-
Notifications
You must be signed in to change notification settings - Fork 137
Issue 1483 #1531
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
Issue 1483 #1531
Conversation
…g the refresh token. Signed-off-by: habeck <[email protected]>
Signed-off-by: habeck <[email protected]>
…or (i.e.. either "uui" or "cli"). Cli can only authenticate using PKCE providers (public client), and UI uses std OIDC providers with client id and client secret. Signed-off-by: habeck <[email protected]>
Signed-off-by: habeck <[email protected]>
Signed-off-by: habeck <[email protected]>
Signed-off-by: habeck <[email protected]>
Signed-off-by: habeck <[email protected]>
pilartomas
left a comment
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.
Please, only focus on the changes in auth_manager. That's all what is needed.
The remaining changes should be reverted, they introduce bugs and regressions.
…PR review. Signed-off-by: habeck <[email protected]>
…tected-resource" and updated server command to use it. Signed-off-by: habeck <[email protected]>
Signed-off-by: habeck <[email protected]>
araujof
left a comment
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.
Overall, this PR makes good changes to address issues with CLI auth:
- Token refresh implementation for handling expired tokens
- PKCE implementation for the CLI
- Separation of authentication concerns
However, a few changes may require discussion (and could be split into separate PRs):
- Issues related to commented-out DCR code (
# registration_endpoint = oidc["registration_endpoint"]) - Issues related to manual client input (
# if not client_id: # client_id = await inquirer.text()): - The
appfield
Signed-off-by: habeck <[email protected]>
…ation of non public client) Signed-off-by: habeck <[email protected]>
Signed-off-by: habeck <[email protected]>
Signed-off-by: habeck <[email protected]>
Signed-off-by: habeck <[email protected]>
Signed-off-by: habeck <[email protected]>
Signed-off-by: habeck <[email protected]>
Signed-off-by: habeck <[email protected]>
pilartomas
left a comment
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.
LGTM
| "client_data": [ | ||
| {"server": str(p.issuer), "client_id": p.client_id, "name": p.name} | ||
| for p in self._config.auth.oidc.providers | ||
| if p.issuer is not None | ||
| ], |
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 believe this can be also cleaned up.
By the way, I noticed Client ID Metadata RFC Draft which tries to approach this problem from a different angle but also replaces DCR in a sense. It allows client_id to be an arbitrary URL, previously unknown to the authorization server. The authorization server is then responsible for grabbing the client information dynamically during the flow itself (but making no persistent client, I believe).
Please slack me directly if you need a working PKCE configuration to test this setup.
Automatic token refresh example:
cli login example: