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

Redirect after token expiration #10165

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jacob-nv
Copy link
Contributor

Description

Trying to navigate to specific link with token being expired will force redirect to /files/spaces/personal which overrides original redirect.

Found one solution which is setting redirect in UserManager constructor, while checking for existence of stored redirect in combination with /files/spaces/personal to prevent override of this storage. Other part of condition is to check for url not being oidc redirect or callback, since this would be stored (in this case setting url from constructor) and the app would get stuck on callback pages.

Related Issue

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@jacob-nv jacob-nv self-assigned this Dec 13, 2023
Copy link

update-docs bot commented Dec 13, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

Copy link

sonarcloud bot commented Dec 13, 2023

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

3 New issues
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Collaborator

@JammingBen JammingBen left a comment

Choose a reason for hiding this comment

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

Tested it and it works great. I'm not 100% sold on the way how it's implemented though 😅 Initializing the userManager with a redirect URL feels a bit weird. Also the edge case for /files/spaces/personal.

After a token has expired and the user opens a page, they will get logged out: https://github.com/owncloud/web/blob/master/packages/web-runtime/src/services/auth/authService.ts#L262C1-L262C1. After the logout the user is automatically being redirected to the login page again, but it loses every context such as the redirectUri during that process.

One way to possibly solve this would be to do a silent sign out instead and then manually redirect to the login page while also passing the correct redirectUri. I couldn't find a way to make a silent logout work in the past though... It would be also beneficial for this issue: #4677. Maybe you could check if you can make it work? We basically want what signoutRedirect does but without the redirect. The lib has a method signoutSilent (although not exported to the outside for some reason), maybe that would work somehow. Our userManager is extending the one from the lib.

Or maybe @kulmann / @dschmidt have an idea (or are fine with the current approach).

@jacob-nv
Copy link
Contributor Author

Tested it and it works great. I'm not 100% sold on the way how it's implemented though 😅 Initializing the userManager with a redirect URL feels a bit weird. Also the edge case for /files/spaces/personal.

After a token has expired and the user opens a page, they will get logged out: https://github.com/owncloud/web/blob/master/packages/web-runtime/src/services/auth/authService.ts#L262C1-L262C1. After the logout the user is automatically being redirected to the login page again, but it loses every context such as the redirectUri during that process.

One way to possibly solve this would be to do a silent sign out instead and then manually redirect to the login page while also passing the correct redirectUri. I couldn't find a way to make a silent logout work in the past though... It would be also beneficial for this issue: #4677. Maybe you could check if you can make it work? We basically want what signoutRedirect does but without the redirect. The lib has a method signoutSilent (although not exported to the outside for some reason), maybe that would work somehow. Our userManager is extending the one from the lib.

Or maybe @kulmann / @dschmidt have an idea (or are fine with the current approach).

signoutSilent method is exported but in 2.3 version, current version is 2.1
this way it seems to work, cannot await signoutSilent method, it throws error Refused to frame 'https://host.docker.internal:9200/' because an ancestor violates the following Content Security Policy directive: "frame-ancestors 'none'"

But somehow this seems to work well, it signouts user, sets the proper redirect url, redirects to login and from login you get to stored redirect url

@JammingBen
Copy link
Collaborator

Needs to be tested and refined (probably), hence I converted it back to a draft. The last time I checked it worked, however, it broke #10063.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Login when coming from the "Logged out" page doesn't redirect correctly
2 participants