-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add OAuth2 support #502
Add OAuth2 support #502
Conversation
Fixed a nil panic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome.
I have som clarifying questions.
For us to release this gradually, then we should allow both authentication strategies. We could control this via a flag to avoid coupling a release here with out own deployment.
A release would follow these steps:
- Deploy release manger with support for IDP
- Move beta tests to use the IDP while the rest uses the old authentication flow
- Move all developers to IDP flow via new hamctl version
- Remove support for old authentication flow.
Can should be able to do the above steps after this PR is merged.
That is exactly how we also think the release procedure should be. We do allow both mechanisms so it is doable. I will address the other comments and resolve when they are done. Next step is to figure out the IDP configuration, we do have a test setup that we have used to verify this implementation, but we should have something production ready set up. I will let you know when we have resolved all comments 🚀 |
I have nothing more to add. Would you mind giving it another look? 🙏 |
This PR contains the addition of the login command to hamctl which will initiate an oauth2 device authorization flow.
The access token is persisted to disk and when it is expired the user must do a login again.
The release daemon and artifact executables has been moved to client credentials flow to have oauth2 on all the calls.
We support both the old auth token that is shared as well as the new jwt. The server must be configured with an IdP but this version is kept backwards compatible such that old clients can still use the server. When users have updated their cli's then we can remove the support for the static token.
The changes include that:
Question for the reviewers:
Should we initiate a login if the token is invalid or should we just fail and tell the user to do a login instead?
Doing automatic logins will hopefully make for at better user experience but that might break e.g. scripts that cannot pop a browser.
@AndersSoee suggested that we could add an additional flag that indicates that we should not auto-login if the token is invalid.
Currently we just fail if the token is invalid and tell the user to log in.
We have been holding back on documentation as we would like to agree to the changes before writing it 🚀