Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

auth: prompt the user to open the browser #418

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sudomateo
Copy link

@sudomateo sudomateo commented Oct 24, 2023

Previously the oxide auth login command would automatically open the
user's browser which can be a jarring user experience.

This patch updates oxide auth login to prompt the user to open the
browser instead of just opening the browser automatically. Users passing
--no-browser will not receive this prompt and will instead have the
URL printed to standard output.

Closes: #88

@ahl
Copy link
Collaborator

ahl commented Nov 11, 2023

Sorry for the delay on this. Looks good: any thoughts on how we might add a test?

@sudomateo
Copy link
Author

Sorry for the delay on this. Looks good: any thoughts on how we might add a test?

No worries! I can add a unit test to wait_for_enter, but I wasn't entirely sure how to best test the changes to the auth command. I was hoping I could get some suggestions on it.

I see there are tests in cli/tests that I could probably use as inspiration.

@ahl
Copy link
Collaborator

ahl commented Nov 11, 2023

Give it a shot? if nothing seems to work well then we'll merge as-is. Thanks again!

@sudomateo
Copy link
Author

Give it a shot? if nothing seems to work well then we'll merge as-is. Thanks again!

Will do! Busy week this week but I'll work on it sometime next week. I'll ping here when updated.

@sudomateo sudomateo changed the title Prompt the user to open the browser auth: prompt the user to open the browser Dec 23, 2023
@sudomateo
Copy link
Author

Sorry for the delay on this. I finally got some time to work on this. I refactored the function to be generic and added unit tests. If there's time tomorrow I'll take a look at adding an integration test that actually runs oxide auth login with mocked data.

@sudomateo
Copy link
Author

I added integration tests. This is ready for another review. There might be a need to update how the open::that is called since I'd imagine we'd want to skip opening the browser in integration tests, but we can discuss that during review.

Previously the `oxide auth login` command would automatically open the
user's browser which can be a jarring user experience.

This patch updates `oxide auth login` to prompt the user to open the
browser instead of just opening the browser automatically. Users passing
`--no-browser` will not receive this prompt and will instead have the
URL printed to standard output.

Closes: oxidecomputer#88
@sudomateo sudomateo force-pushed the sudomateo/auth-login-updates branch from 0627e10 to 8bf8446 Compare March 26, 2024 15:09
@sudomateo
Copy link
Author

Rebased and updated the branch. cargo test is passing locally. I don't have permission to start CI/CD here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Press enter to open browser for auth
2 participants