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.

Sign up for GitHub

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

Jump to bottom

feat(auth): Add OAuth logout #786

Merged
merged 17 commits into from
Jan 18, 2022
Merged

feat(auth): Add OAuth logout #786

merged 17 commits into from
Jan 18, 2022

Conversation

jan-law
Copy link
Contributor

@jan-law jan-law commented Dec 16, 2021 •
edited
Loading

Fixes #717
Depends on cryostatio/cryostat-web#355
Depends on cryostatio/cryostat-operator#319
Depends on cryostatio/cryostat-operator#320

Please see the -web PR for more information about the OAuth server logout behaviour.

This PR also updates the web client and caches the logout redirect url.

@jan-law jan-law added the feat New feature or request label Dec 16, 2021
@jan-law jan-law requested a review from andrewazores December 16, 2021 18:59
andrewazores
andrewazores reviewed Dec 16, 2021
View reviewed changes
Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

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

Code looks pretty reasonable. I'll take a look at the OpenShift auth docs next.

@jan-law jan-law mentioned this pull request Dec 22, 2021
Closed
@andrewazores
Copy link
Member

This and cryostatio/cryostat-web#355 are looking pretty good. I'll wait until cryostatio/cryostat-operator#320 is resolved so I can also test that out - right now I'm seeing the (currently expected) CORS failure when trying to log out from the admin account.

@jan-law
Copy link
Contributor Author

jan-law commented Jan 11, 2022

Note: The "Token Authentication" section in https://cryostat.io/get-started/ should also be updated when this OAuth feature becomes available in the next Cryostat release.

@andrewazores andrewazores mentioned this pull request Jan 11, 2022
Closed
@jan-law jan-law mentioned this pull request Jan 13, 2022
Merged
@jan-law
Copy link
Contributor Author

jan-law commented Jan 17, 2022 •
edited
Loading

Rebased just now. The only conflict was fixed by rebasing the web-client with the latest web-client commits on the main branch.

andrewazores
andrewazores reviewed Jan 18, 2022
View reviewed changes
Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

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

LGTM otherwise. I would be okay with deferring my one suggestion above to a follow-up PR as well if you'd prefer, since that sort of cleanup also might change exception handling and the exceptions that are reported to the web-client.

@github-actions
Copy link
Contributor

This PR/issue depends on:

@andrewazores andrewazores merged commit 6c4ab05 into cryostatio:main Jan 18, 2022
@jan-law jan-law deleted the oauth-logout branch January 18, 2022 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebaron ebaron ebaron left review comments

@andrewazores andrewazores andrewazores approved these changes

Assignees
No one assigned
Labels
feat New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OAuth Redirect Authentication flow
3 participants
@jan-law @andrewazores @ebaron