Skip to content

fix-expiration-of-token-from-actual-idtoken#1040

Merged
peternied merged 29 commits intoopensearch-project:mainfrom
spartan2015:fix-expiration-of-token-from-actual-idtoken
Aug 31, 2022
Merged

fix-expiration-of-token-from-actual-idtoken#1040
peternied merged 29 commits intoopensearch-project:mainfrom
spartan2015:fix-expiration-of-token-from-actual-idtoken

Conversation

@spartan2015
Copy link
Contributor

@spartan2015 spartan2015 commented Jul 23, 2022 •
edited
Loading

Description

Seem like oidc token response from Gitlab expiresIn property is not the actual expiration of the idToken. idToken expires before the access_token does. Which makes it security plugin unusable when integrating with Gitlab

Category

Bug fix

Why these changes are required?

to expire the jwt token when the jwt token says is going to expire which is not the expiresIn fields from tokenResponse but the idToken claims exp field

What is the old behavior before changes and new behavior after changes?

Issues Resolved

[List any issues this PR will resolve (Is this a backport? If so, please add backport PR # and/or commits #)]

Testing

unit testing was done in helper.test.ts for the actual expiration extraction

Check List

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@spartan2015 spartan2015 requested a review from a team July 23, 2022 14:44
@spartan2015 spartan2015 changed the base branch from main to 2.1 July 23, 2022 17:12
@spartan2015 spartan2015 changed the base branch from 2.1 to main July 25, 2022 12:54
@DarshitChanpura
Copy link
Member

DarshitChanpura commented Jul 25, 2022

Thank you for submitting this PR @spartan2015! Can you please sign-off all your commits so DCO can pass?

@cliu123
Copy link
Member

cliu123 commented Jul 27, 2022

@spartan2015 Thanks for contributing! Unit tests failed. Would you please take a look?

@spartan2015
Copy link
Contributor Author

spartan2015 commented Jul 28, 2022

@spartan2015 Thanks for contributing! Unit tests failed. Would you please take a look?

fixed

@spartan2015 spartan2015 force-pushed the fix-expiration-of-token-from-actual-idtoken branch from 9f7708f to 9d9785f Compare July 28, 2022 11:01
@spartan2015
Copy link
Contributor Author

spartan2015 commented Jul 28, 2022

Thank you for submitting this PR @spartan2015! Can you please sign-off all your commits so DCO can pass?

fixed

@spartan2015 spartan2015 reopened this Jul 28, 2022
peternied
peternied requested changes Jul 28, 2022
View reviewed changes
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution, I'd like to see all the data management logic moved into helper.ts.

AMoo-Miki and others added 19 commits August 8, 2022 13:13
@AMoo-Miki
Clear the sessionStorage when logging out (opensearch-project#1003)
c66d980 
Signed-off-by: Miki <miki@amazon.com>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
@cliu123
Bump version to 2.1.0.0 (opensearch-project#1004)
1c58c32 
Signed-off-by: cliu123 <lc12251109@gmail.com>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
@jaycci @peternied @cliu123
Dynamically compute OpenID redirectUri from proxy HTTP headers (opens…
32eec67 
…earch-project#929)

Signed-off-by: Jean-Christian Simonetti <github@elysiria.fr>

Co-authored-by: Peter Nied <petern@amazon.com>
Co-authored-by: Chang Liu <lc12251109@gmail.com>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
@hsiang9431-amzn
Disable private tenant for read only users (opensearch-project#868)
40478f5 
* Disable private tenant for read only users

Signed-off-by: Gio Collina <gio.collina@eliatra.com>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
@aoguan1990
Fix OIDC looping issue - too many redriects (opensearch-project#1014)
08092a4 
Signed-off-by: Aozixuan Priscilla Guan <aoguan@amazon.com>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
@downsrob
Adds snapshot management cluster permissions (opensearch-project#1016)
46980c8 
Signed-off-by: Clay Downs <downsrob@amazon.com>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
@DarshitChanpura
Adds release notes for 2.1.0 (opensearch-project#1015)
d66016e 
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
@DarshitChanpura
Updates Dev guide (opensearch-project#897)
a53eab0 
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
@cwperks
Add tests for account-nav-button when multitenancy is disabled (opens…
6182907 
…earch-project#1020)

Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
@cliu123
Get security_tenant search param from URL (opensearch-project#1024)
520d9cd 
Signed-off-by: cliu123 <lc12251109@gmail.com>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
@cwperks
Fix bug in support for jwt.url_param customization (opensearch-projec…
546dd81 
…t#1025)

Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
@DarshitChanpura
Revert "Replace _opendistro route with _plugins (opensearch-project#895…
0d51611 
…)" (opensearch-project#1035)

This reverts commit e4e4032.

Signed-off-by: Vasile Negru <vasile@eosfintek.com>
fix-expiration-of-token-from-actual-idtoken
a6bf095 
Signed-off-by: Vasile Negru <pro@ChooseExcellenc.localdomain>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
fix-expiration-of-token-from-actual-idtoken
cccfd75 
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
Signed-off-by: Vasile Negru <pro@ChooseExcellenc.localdomain>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
fix-expiration-of-token-from-actual-idtoken
04ffaeb 
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
Signed-off-by: Vasile Negru <pro@ChooseExcellenc.localdomain>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
fix-expiration-of-token-from-actual-idtoken
5adca5b 
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
Signed-off-by: Vasile Negru <pro@ChooseExcellenc.localdomain>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
fix-expiration-of-token-from-actual-idtoken
d4aba7a 
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
Signed-off-by: Vasile Negru <pro@ChooseExcellenc.localdomain>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
fix-expiration-of-token-from-actual-idtoken
913f917 
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
Signed-off-by: Vasile Negru <pro@ChooseExcellenc.localdomain>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
fix-expiration-of-token-from-actual-idtoken
766a07e 
Signed-off-by: Vasile Negru <irimia.vasile@gmail.com>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
Signed-off-by: Vasile Negru <pro@ChooseExcellenc.localdomain>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
Vasile Negru and others added 6 commits August 8, 2022 13:13
Revert "Revert "Replace _opendistro route with _plugins (opensearch-p…
257d9ea 
…roject#895)" (opensearch-project#1035)"

This reverts commit c456883362610c61fcc5d54b2974d7a5c6327c1d.

Signed-off-by: Vasile Negru <pro@ChooseExcellenc.localdomain>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
fix-expiration-of-token-from-actual-idtoken
27488bd 
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
Signed-off-by: Vasile Negru <pro@ChooseExcellenc.localdomain>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
@DarshitChanpura
Revert "Replace _opendistro route with _plugins (opensearch-project#895…
1e6a952 
…)" (opensearch-project#1035)

This reverts commit e4e4032.

Signed-off-by: Vasile Negru <pro@ChooseExcellenc.localdomain>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
Revert "Revert "Replace _opendistro route with _plugins (opensearch-p…
e45afa4 
…roject#895)" (opensearch-project#1035)"

This reverts commit c456883362610c61fcc5d54b2974d7a5c6327c1d.

Signed-off-by: Vasile Negru <vasile@eosfintek.com>
fix-expiration-of-token-from-actual-idtoken
fdc1d0b 
Signed-off-by: Vasile Negru <pro@ChooseExcellenc.localdomain>
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
fix-expiration-of-token-from-actual-idtoken
b26ccea 
Signed-off-by: Vasile Negru <vasile@eosfintek.com>
@spartan2015 spartan2015 force-pushed the fix-expiration-of-token-from-actual-idtoken branch from 7e17a0f to b26ccea Compare August 8, 2022 10:16
Merge branch 'main' into fix-expiration-of-token-from-actual-idtoken
e0bacde 
# Conflicts:
#	server/auth/types/openid/helper.test.ts
peternied
peternied approved these changes Aug 8, 2022
View reviewed changes
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

So much cleaner, thanks @spartan2015

DarshitChanpura
DarshitChanpura approved these changes Aug 23, 2022
View reviewed changes
Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

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

thank you @spartan2015 !

@codecov-commenter
Copy link

codecov-commenter commented Aug 23, 2022 •
edited
Loading

Codecov Report

Merging #1040 (326c154) into main (964e804) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1040   +/-   ##
=======================================
  Coverage   72.27%   72.27%           
=======================================
  Files          87       87           
  Lines        1915     1915           
  Branches      244      244           
=======================================
  Hits         1384     1384           
  Misses        478      478           
  Partials       53       53

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@peternied peternied merged commit 057cbe4 into opensearch-project:main Aug 31, 2022
@peternied peternied added the backport 2.x backport to 2.x branch label Aug 31, 2022
@peternied
Copy link
Member

peternied commented Aug 31, 2022

@spartan2015 Thanks for sticking with this change, it's been merged!

@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Aug 31, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@peternied peternied peternied approved these changes

@DarshitChanpura DarshitChanpura DarshitChanpura approved these changes

Assignees

No one assigned

Labels

backport 2.x backport to 2.x branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.