NASA-AMMOS / aerie-cli

An unofficial CLI for interacting with Aerie planning software
MIT License
3 stars 4 forks source link

Add Hasura auth option #38

Closed Navarro-Jonathan closed 1 year ago

Navarro-Jonathan commented 1 year ago
cartermak commented 1 year ago

@Mythicaeda

Minor: Usage of the flag is non-intuitive.

We agree, but this is the default functionality of Typer when adding "global" (i.e., applying to all commands) arguments. I think we could start hacking around with the underlying library to make it behave like you suggested (which we like as well), but I don't think it's a worthwhile endeavor right now.

Major: This doesn't work with multiple deployments.

I assume this is based on the assumption that the admin secret is persistent across multiple invokations of aerie-cli, which it isn't (or, at least, is not intended to be).

if the admin secret is provided, then the user shouldn't still need to log in, as, as far as I'm aware, the point of using the Admin Secret is to avoid needing to Authenticate with the Gateway.

We can talk through this; my thought is that we'd lose the ability to do any gateway interactions (currently, only uploading a mission model) if we let the admin secret be alternate to, instead of in addition to, the primary auth method. Maybe that's a worthwhile cost (and maybe that's more in alignment with what you need from this feature).

Mythicaeda commented 1 year ago

@cartermak

We agree, but this is the default functionality of Typer when adding "global" (i.e., applying to all commands) arguments. I think we could start hacking around with the underlying library to make it behave like you suggested (which we like as well), but I don't think it's a worthwhile endeavor right now.

Understood. I agree that that's not worthwhile currently.

I assume this is based on the assumption that the admin secret is persistent across multiple invokations of aerie-cli, which it isn't (or, at least, is not intended to be).

It was based on my experience while testing this branch.

We can talk through this; my thought is that we'd lose the ability to do any gateway interactions (currently, only uploading a mission model) if we let the admin secret be alternate to, instead of in addition to, the primary auth method. Maybe that's a worthwhile cost (and maybe that's more in alignment with what you need from this feature).

That's true and I'd forgotten about it. For the sake of discussion, a solution for this that would work with the alternate to option would be to "put off" having the user login in admin mode until they try to upload a mission model, since that's the only command that requires gateway interaction.

cartermak commented 1 year ago

@Mythicaeda

It was based on my experience while testing this branch.

Ope, we best fix that. @Navarro-Jonathan Can you please test & adapt so that we don't store the header as part of the persistent session? If it's not trivial, we can talk it through.

That's true and I'd forgotten about it. For the sake of discussion, a solution for this that would work with the alternate to option would be to "put off" having the user login in admin mode until they try to upload a mission model, since that's the only command that requires gateway interaction.

I think this would be very difficult to implement...OK to leave it at "for the sake of discussion"? Or do you see any real use cases where we'd need to use the command-line option without being able to authenticate a user?

Mythicaeda commented 1 year ago

I think this would be very difficult to implement...OK to leave it at "for the sake of discussion"? Or do you see any real use cases where we'd need to use the command-line option without being able to authenticate a user?

@cartermak I'm good with leaving at "for the sake of discussion" unless we have a real use case (since the one I'm thinking of with application accounts use the CLI as a Python library and can just provide a custom HostSession)

Navarro-Jonathan commented 1 year ago

@cartermak

Ope, we best fix that. @Navarro-Jonathan Can you please test & adapt so that we don't store the header as part of the persistent session? If it's not trivial, we can talk it through.

Sure thing. It may have just been reading the environment variable, but I'll look into that now.

cartermak commented 1 year ago

I'm good with leaving at "for the sake of discussion" unless we have a real use case (since the one I'm thinking of with application accounts use the CLI as a Python library and can just provide a custom HostSession)

Excellent 🙂

Navarro-Jonathan commented 1 year ago

Sure thing. It may have just been reading the environment variable, but I'll look into that now.

I believe the header isn't being stored persistently anywhere. When testing by running one command with the --hasura-admin-secret option followed by one without, the header isn't retained.

I looked through the codebase and didn't see how it would be written persistently at any point. The header's only being injected into the session during the session's equivalent of a get method.

Mythicaeda commented 1 year ago

I believe the header isn't being stored persistently anywhere. When testing by running one command with the --hasura-admin-secret option followed by one without, the header isn't retained.

Retesting locally and can confirm 👍 no more storage issues. My issue might've just been me forgetting to reset my env when I changed from testing the env option to testing the flag option

Navarro-Jonathan commented 1 year ago

Retesting locally and can confirm 👍 no more storage issues. My issue might've just been me forgetting to reset my env when I changed from testing the env option to testing the flag option

Great! I'm happy to see it seems to be working properly.