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 to login page in case token is expired instead of showing an error page #395

Conversation

eva-mueller-coremedia
Copy link
Contributor

@eva-mueller-coremedia eva-mueller-coremedia commented Sep 12, 2024

Instead of showing an error page, the user should be redirected to the login page.

fixes: #396

Testing done

I have tested this with 4.340.ve70636c6590e and by setting the following

(Cognito) Refresh Configuration:

Refresh token expiration: 60 minutes
Access token expiration: 10 minutes
ID token expiration: 10 minutes

After 60 minutes, I got redirected to the login page instead of getting presented an error page.

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

Instead of showing an error page
@eva-mueller-coremedia
Copy link
Contributor Author

There are a lot of test failures. I will check them

Copy link

codecov bot commented Sep 12, 2024

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.

Project coverage is 72.72%. Comparing base (a5a97f8) to head (89afbcf).
Report is 39 commits behind head on master.

Files with missing lines Patch % Lines
...va/org/jenkinsci/plugins/oic/OicSecurityRealm.java 71.42% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #395      +/-   ##
============================================
+ Coverage     71.88%   72.72%   +0.84%     
- Complexity      239      245       +6     
============================================
  Files            12       12              
  Lines          1010     1023      +13     
  Branches        145      148       +3     
============================================
+ Hits            726      744      +18     
+ Misses          206      199       -7     
- Partials         78       80       +2     

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

@jtnord
Copy link
Member

jtnord commented Oct 2, 2024

Cognito

is this behaving differently to Okta? I tried to find some docs from Amazon on using Cognito as an OP but failed.
Just wondering if there is a different bug hiding under this behaviour.

@eva-mueller-coremedia
Copy link
Contributor Author

eva-mueller-coremedia commented Oct 4, 2024

Cognito

is this behaving differently to Okta? I tried to find some docs from Amazon on using Cognito as an OP but failed. Just wondering if there is a different bug hiding under this behaviour.

Dunno if your statement is for me.

AFAIK, Cognito's Refresh Token will expire after a configurable amount of time. Then, the ID Token can't be refreshed anymore. This is great.

This PR is just about redirecting the user to the login page in case that the user need to re-login instead of showing a plain error page.

@eva-mueller-coremedia
Copy link
Contributor Author

@jtnord @michael-doubez Anything left I should do so that this PR could get merged?

@jtnord
Copy link
Member

jtnord commented Oct 7, 2024

@jtnord @michael-doubez Anything left I should do so that this PR could get merged?

I was being selfish and wanted to land #409 before adapting other changes (rather than the other way around). If you are ok waiting then I will rebase this after #409 lands, otherwise please tell me so

@eva-mueller-coremedia
Copy link
Contributor Author

eva-mueller-coremedia commented Oct 7, 2024

@jtnord @michael-doubez Anything left I should do so that this PR could get merged?

I was being selfish and wanted to land #409 before adapting other changes (rather than the other way around). If you are ok waiting then I will rebase this after #409 lands, otherwise please tell me so

Yes, understandable. I would say: it depends on the waiting timespan 😄 - I you land in less than 1 week, I would say: sure. If it takes (unpredictable) longer, I would appreciate it if you could release it first. If you don't mind being honest 😅

@jtnord
Copy link
Member

jtnord commented Oct 7, 2024

@jtnord @michael-doubez Anything left I should do so that this PR could get merged?

I was being selfish and wanted to land #409 before adapting other changes (rather than the other way around). If you are ok waiting then I will rebase this after #409 lands, otherwise please tell me so

Yes, understandable. I would say: it depends on the waiting timespan 😄 - I you land in less than 1 week, I would say: sure. If it takes (unpredictable) longer, I would appreciate it if you could release it first. If you don't mind being honest 😅

No worries, I am going on holiday, so this will be merged this week whichever way it goes.

@jtnord jtnord added the bug label Oct 8, 2024
@jtnord jtnord merged commit c7c0c06 into jenkinsci:master Oct 8, 2024
19 of 20 checks passed
@eva-mueller-coremedia eva-mueller-coremedia deleted the eva-mueller-coremedia/redirect-on-token-expiry branch October 8, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redirect to login page when token has expired
3 participants