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

use token to login #1201

Merged
merged 1 commit into from
Mar 7, 2023
Merged

use token to login #1201

merged 1 commit into from
Mar 7, 2023

Conversation

rajatjindal
Copy link
Contributor

@bacongobbler
Copy link
Member

I’d recommend calling the environment variable FERMYON_TOKEN. Hippo has no concept of a personal access token at this time.

@rajatjindal rajatjindal force-pushed the use-token branch 3 times, most recently from 3106e5e to 0887fc3 Compare February 27, 2023 07:44
src/commands/login.rs Outdated Show resolved Hide resolved
src/opts.rs Outdated Show resolved Hide resolved
src/commands/login.rs Outdated Show resolved Hide resolved
src/commands/login.rs Outdated Show resolved Hide resolved
@rajatjindal
Copy link
Contributor Author

this is ready for review again @lann @itowlson @bacongobbler, thanks

src/commands/login.rs Show resolved Hide resolved
src/commands/deploy.rs Outdated Show resolved Hide resolved
src/commands/deploy.rs Outdated Show resolved Hide resolved
src/commands/login.rs Outdated Show resolved Hide resolved
src/commands/login.rs Show resolved Hide resolved
src/commands/deploy.rs Outdated Show resolved Hide resolved
match &login_connection.expiration {
Some(expiration) => match DateTime::parse_from_rfc3339(expiration) {
Ok(time) => Utc::now() > time,
Err(_) => true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I'm reading this correctly, if the expiration date cannot be parsed from the token string, we consider it to be expired.

Should we log an error message about the expiration date failing to be parsed? Shouldn't we inform the user that the token contains an invalid expiration date?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently if the token is expired (or it has failed during parsing), we initialize the login process automatically. after adding this log msg, this is how it would look like to user

$) spin deploy
Failed to parse token expiration time 'abc'
Error: premature end of input

Copy your one-time code:

2geg5Xf9

...and open the authorization page in your browser:

https://cloud.fermyon.com/device-authorization

Waiting for device authorization...

does this look ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in a CI scenario, if this were to happen, the step will be stuck forever. should we just exit instead of restarting auth if expiration parsing has failed.

here is what we currently do when token json file is invalid:

$) spin deploy                    
Error: expected `,` or `}` at line 3 column 3

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should exit when we've read invalid data. There's no guarantee that attempting a second login will give us valid data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed it to print following msg, and exit if parsing of expiration time fails.

spin deploy
Failed to parse token expiration time 'abc'. Error: premature end of input

Run `spin login` to log in again

src/opts.rs Outdated Show resolved Hide resolved
@rajatjindal rajatjindal force-pushed the use-token branch 2 times, most recently from 5a36df8 to 211ddd9 Compare March 2, 2023 04:05
Copy link
Member

@radu-matei radu-matei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manually tested, things look great!

LGTM

})
.list_apps()
.await
.context("token validation failed")?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Entered a wrong token and this was the result:

$ spin cloud login
Error: token validation failed

Caused by:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@itowlson earlier suggested:

"Login using the provided token failed.  Run spin login or create a new token using the Fermyon Cloud user interface."

does this look ok?

(this will change token validation failed with above message. Caused by: will remain as is because it seems to be coming from anyhow crate)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is how it looks like with latest changes:

➜  spin login --token abc
Error: Login using the provided token failed. Run `spin login` or create a new token using the Fermyon Cloud user interface.

Caused by:

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>
Copy link
Member

@michelleN michelleN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. had a question about expiration details but not a blocker. Thanks!


let token_info = TokenInfo {
token: self.token.clone(),
expiration: None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the plan to add expiration details in the future?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Eventually personal access tokens can be created with a timed expiration, but it's not part of the MVP. You can still revoke access by deleting the token, however.

@rajatjindal rajatjindal merged commit 98e198e into fermyon:main Mar 7, 2023
@rajatjindal
Copy link
Contributor Author

thank you for the review/approval Michelle (and everyone)

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.

None yet

6 participants