Skip to content

feat: endpoint to logout app subdomain URLs #5428

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
Merged
merged 5 commits into from
Dec 20, 2022
Merged

feat: endpoint to logout app subdomain URLs #5428

merged 5 commits into from
Dec 20, 2022

Conversation

deansheather
Copy link
Member

@deansheather deansheather commented Dec 15, 2022 •
edited
Loading

Closes #4394

Adds a "special" app URL coder-logout (which is not a valid app URL so cannot be conflicted by real apps) which will delete the API key from the database and the cookie. Very lenient towards invalid tokens and will delete the cookie and redirect without failing even if it's an invalid token in most cases.

Accepts a redirect_uri query param which must have a hostname that matches the access URL or any app URL. Defaults to the access URL.

E.g. https://coder-logout.dev.coder.com would log you out of all app URLs and return you to the login page.

TODO:

  • Change the frontend to redirect here (instead of login page) on logout

@deansheather deansheather requested a review from a team as a code owner December 15, 2022 14:34
@deansheather deansheather requested review from code-asher and Kira-Pilot and removed request for a team and code-asher December 15, 2022 14:34
@deansheather
Copy link
Member Author

Regarding the frontend, the implementation causes the login screen to flash before redirecting, but I'm unsure how to do it better in xstate.

@deansheather deansheather requested a review from f0ssel December 15, 2022 20:13
f0ssel
f0ssel approved these changes Dec 15, 2022
View reviewed changes
Copy link
Contributor

@f0ssel f0ssel left a comment

Choose a reason for hiding this comment

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

good tests 👍

@deansheather deansheather requested a review from coadler December 15, 2022 20:57
@deansheather
Copy link
Member Author

@Kira-Pilot is there a better way to redirect to the page without flashing the login screen for a split second?

@deansheather
Copy link
Member Author

This is blocked while we figure out a better way to avoid the double login page flash. Kira, Joe and I did some research into it in Discord on Friday and we found that the flash is caused by the RequireAuth component reacting to the state being "signedOut" and navigating to the login screen before we've finished navigating to the new logout endpoint.

The fix seems to be adding a new state to the auth state provider called "signedOutNavigatingAway" or something similar and having the RequireAuth component do nothing if it sees that state. Since Kira is out next week I might need some help from @BrunoQuaresma or @presleyp next week to get those changes finalized.

@BrunoQuaresma BrunoQuaresma self-assigned this Dec 19, 2022
deansheather
deansheather commented Dec 20, 2022
View reviewed changes
@deansheather deansheather enabled auto-merge (squash) December 20, 2022 18:39
@deansheather deansheather merged commit 50dfc20 into main Dec 20, 2022
@deansheather deansheather deleted the dean/app-url-logout branch December 20, 2022 18:45
@github-actions github-actions bot locked and limited conversation to collaborators Dec 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kylecarbs kylecarbs kylecarbs approved these changes

@Kira-Pilot Kira-Pilot Kira-Pilot approved these changes

@f0ssel f0ssel f0ssel approved these changes

@coadler coadler Awaiting requested review from coadler

Assignees

@deansheather deansheather

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Logging out in the dashboard should also logout of subdomain app URLs
5 participants
@deansheather @kylecarbs @Kira-Pilot @f0ssel @BrunoQuaresma