-
Notifications
You must be signed in to change notification settings - Fork 163
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
[v2.9] Azuread support #346
Conversation
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.
One thought I had about the --prompt option that I figured I'd capture here before I forget. While it certainly works well, it feels like a bit of a non-standard approach to this sort of operation (copy/paste a URL from a blank page). We should definitely make sure that this UX is reasonable, and if it is, we should definitely add some extra text in the cli output to make it very clear to the user what the expected flow is in this scenario.
@crobby that's a legit concern. What about having a Rancher UI page that is not doing anything in the backend, but just showing you the URL, or maybe the query params? That will be pretty cool, and actually useful also to show the errors. The user will be prompted to input the code, and/or the other parameters. It need a bit of work on the UI side, but not much. |
0965aa1
to
4a26aff
Compare
6c514fd
to
5d70023
Compare
3bbfa67
to
0a27e78
Compare
In the final implementation, implementing the RFC there is no update on the UI. |
664b214
to
8ae800c
Compare
bump rancher added dashboard implementation
updated login request removed unused import removed prompt flag
go mod fix
1cb0854
to
3553f75
Compare
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.
I think this looks pretty good.
Any chance of adding some unit tests? Might be reasonable to add a few at least for the new functionality.
- added getClient func to create once the HTTP client with the same TLS configuration. - added tests for the getAuthProviders func
Thanks @crobby. I've added just a couple of tests to the With the latest commit I've added a /cc FYI @samjustus @anupama2501 |
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.
The main issue with the local references is done.
Regarding the cert pool I defer to @pjbgf 's final assessment
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.
I think this lgtm pending the feedback/resolution on the cert pool.
As discussed internally we can keep the changes as-is (reverting a func name) to keep the existing behaviour. We will address the issues in a next version of the cli. |
Ref:
SURE-4275
This PR adds the support for the AzureAD provider. When configured the
rancher token
command will prompt the user to select it, or use the local login.The CLI will use the Device Authorization Grant, so it will ask the user to enter a code in the URL, as discussed in the RFC.
The PR adds also some refinements and refactor:
The
getAuthProviders
func now returns a[]TypedProvider
. With rancher/rancher#44285 theGetType
method was added, so that every provider automatically implements any interface with the same method.With this we can actually do a type assertion on the returned interface and get the concrete underlying type.
Note: before merging the import for the struct will need to be updated.