OIDC session expires with ID token expiry and refresh token is never used.

[Alankarsharma]

Description

In OIDC flow ID token expiry time was used to set cookie expiry time. Because of this cookie was getting expired at same time as ID token and alternate flow in isValidCookie function to fetch new ID token using refresh token is not getting triggered as cookie itself is expired.

Category

Bug fix

Why these changes are required?

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

This code was correct in version 2.11.1, issue is only in 2.13.0 version. I haven't verified 2.12.0 version.

Issues Resolved

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

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

Check List

• [ ] New functionality includes testing
• [ ] New functionality has been documented
• [x] 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.

[derek-ho]

Hi @Alankarsharma thanks for the PR! Just providing some initial feedback here - I was the person who made the PR that changed the OIDC cookie expiry - initial PR is here. I am not an OIDC expert, so maybe I got some things wrong, but we can discuss here and get to the bottom of this (I still have seen some complaints about auto logouts in the public slack, and I am actually trying to replicate and dive deep on this myself - see conversation here: https://opensearch.slack.com/archives/C051Y637FKK/p1712521239951639).

Some basic background is that there is actually two checks that go into seeing if the cookie is still valid - the place you modified in this PR, but also here. In my initial PR in which I consolidated the expiryTime variables, and used the expiry from the token response is because from my understanding for external IDP login setups we should not be blindly giving a session length of TTL, and should instead rely on the timeout from the credentials given by the IDP. Is that not correct/can you explain a little more the reasoning behind why you think this PR would fix the issues?

[Alankarsharma]

OIDC response has two JWT token, ID Token and Refresh token. You are taking expiry time of ID token and setting it as expiry of cookie, ID token are short lived token and once they are expired we can get new token using Refresh token, code for this is present in isValidCookie. In login flow, I have set ID token expiry value in 'cookie.credentials.expiry' and checking if ID token is still valid or not. If it is valid then true is returned, if it is not valid a new ID token will be fetched using Refresh token and value of this token are updated in cookie again. User is logged-out only if both ID token and refresh token are expired or cookie is invalidated.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 70.61%. Comparing base (9abc57d) to head (d6c989f).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #1924 +/- ## ======================================= Coverage 70.61% 70.61% ======================================= Files 97 97 Lines 2600 2600 Branches 387 380 -7 ======================================= Hits 1836 1836 Misses 668 668 Partials 96 96 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[Alankarsharma]

@derek-ho I checked two code location that you mentioned. No code change is required there.

[derek-ho]

@Alankarsharma I think this looks good - one quick question, maybe we can take this on as a separate follow up item: how can we add a test case around this refresh token flow? Do you have any ideas? I think a simple unit test may not suffice since it seems that the issue was that the cookie was invalidated prior to reaching the isValidCookie function?

[Alankarsharma]

@derek-ho I will check test case for refresh token, will create a separate pull request for that.

[cwperks]

@Alankarsharma Thank you for this PR! I branched off of this and added a test to verify the refreshToken workflow with this fix included: https://github.com/opensearch-project/security-dashboards-plugin/pull/1990

I believe there's one more area where the expiryTime of the credentials needs to be updated instead of the cookie here: https://github.com/opensearch-project/security-dashboards-plugin/blob/9abc57d415e8eedfc96d1bb399cfe118500a4d21/server/auth/types/openid/openid_auth.ts#L296-L300

The changes in this PR will work for the first refresh, but would fail on subsequent refresh since it would be setting the expiry time of the cookie to the expiry time of the access token after first refresh.

[derek-ho]

@Alankarsharma will be closing this out since the change was merged with @cwperks PR. Thank you for your contribution! I hope to see more from you in the future :)