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.

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

fix: allow logout on open file #1744

Closed
wants to merge 1 commit into from
Closed

fix: allow logout on open file #1744

wants to merge 1 commit into from

Conversation

wusteven815
Copy link
Contributor

  • Fixes Open file blocks logout #1685
    • Moved storeCookieToken from AuthPluginPsk.tsx to AuthPlugin.ts, so it can be exported
    • Logout button now uses that to remove the cookies

@wusteven815 wusteven815 self-assigned this Jan 23, 2024
Copy link

codecov bot commented Jan 23, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (45fa929) 46.04% compared to head (122ef83) 46.03%.

Files Patch % Lines
packages/app-utils/src/components/AppBootstrap.tsx 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1744      +/-   ##
==========================================
- Coverage   46.04%   46.03%   -0.01%     
==========================================
  Files         621      621              
  Lines       37454    37457       +3     
  Branches     9413     9413              
==========================================
+ Hits        17244    17245       +1     
- Misses      20156    20158       +2     
  Partials       54       54              
Flag Coverage Δ
unit 46.03% <80.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wusteven815 wusteven815 marked this pull request as ready for review January 24, 2024 18:54
Copy link
Collaborator

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

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

I think this only needs to be 1 line in AuthPluginPsk in onLogout add storeRefreshToken(null).

Since we use plugins for auth, any clearing of stored data on logout should be handled by the plugin. We emit an event on a BroadcastChannel on logout so that any 3rd party plugin can respond to a logout request

With any of the plugin architecture, the mindset we use typically is "could I achieve this if I were a 3rd party writing this plugin". What that means is basically no app specific code (anything in app-utils or code-studio) to handle these things as a 3rd party writing a plugin cannot modify any code except their own plugin code. If it can't be achieved, then that's a shortcoming of the APIs we have for plugins

As an example, here's a plugin for KeyCloak which handles many different types of authentication https://github.com/deephaven/deephaven-plugins/blob/main/plugins/auth-keycloak/src/js/src/AuthPluginKeycloak.tsx#L67

@wusteven815
Copy link
Contributor Author

I think this only needs to be 1 line in AuthPluginPsk in onLogout add storeRefreshToken(null).

When I was debugging this, I noticed that of the 3 listeners (AppBootstrap, AuthPluginPsk, RefreshTokenBootstrap), only AppBootstrap's onLogout was being run, and that was why I added those lines to it. Is the real issue that the other listeners are not running, instead of AppBootstrap not running the logout functions?

@mattrunyon
Copy link
Collaborator

If they aren't being run, then that's the real issue. I added a line to the refresh token onLogout and it seemed to run when I logged out for me

@wusteven815 wusteven815 marked this pull request as draft February 5, 2024 19:25
@github-actions github-actions bot locked and limited conversation to collaborators Feb 12, 2024
@wusteven815 wusteven815 deleted the fix-1685-blocked-logout branch February 20, 2024 15:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open file blocks logout
2 participants