Fix refresh token flow in OIDC accounting for cookie split

[cwperks]

Description

This PR fixes an issue with the refresh token workflow for OIDC after cookie splitting was introduced in 2.7.0. (Link to PR where cookie splitting was introduced: #1352)

The reason the flow was not working was because in the OIDC flow it would refresh the token in the isValidCookie call here. The implementation of isValidCookie in oidc_auth.ts calls on setExtraAuthStorage to save the new access token and split it into multiple cookies.

setExtraAuthStorage(
  request,
  `Bearer ${refreshTokenResponse.idToken}`,
  this.getExtraAuthStorageOptions()
);

Later on in authentication_type.ts it calls on buildAuthHeaderFromCookie which calls on getExtraAuthStorageValue to unsplit the cookies.

extraValue = getExtraAuthStorageValue(request, this.getExtraAuthStorageOptions());

When operated on the same request object, the getter to get the new access token is not fetching the updated value. This PR makes it so that it does not rely on cookie splitting on the request where the access token is refreshed.

Category

[Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation]

Bug fix

Issues Resolved

#1522

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
• 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.

[codecov]

Codecov Report

Merging #1569 (dbf64a5) into main (ecb15d5) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1569   +/-   ##
=======================================
  Coverage   66.18%   66.18%           
=======================================
  Files          93       93           
  Lines        2339     2339           
  Branches      312      312           
=======================================
  Hits         1548     1548           
  Misses        722      722           
  Partials       69       69           

[peternied]

With the check on 243, drop the coalescing operator? I'm not sure what this check would resolve to if it wasn't set (for whatever crazy reason). Could you also add unit tests?

Suggested change

	if (cookie.credentials?.expires_at > Date.now()) {
	if (cookie.credentials.expires_at > Date.now()) {

[peternied]

Provide a default implementation that returns empty string?

[jochen-kressin]

Hi @cwperks! First of all, my apologies for this regression. I'm quite surprised I haven't been bitten by this before - as you describe, when accessing the cookie within the same request, it still holds the previous value.
Thanks for looking into this!

I did come up with an alternative solution that I'd like to run by you. It does unfortunately include accessing the raw Hapi objects, but we already do that within the cookie splitting code, so I guess one more place doesn't hurt.

To my understanding, the Hapi request objects holds the cookie values in two different objects:

• state
• _states

state is what you get after the cookie header is parsed and I believe this is used when you get the cookie value via the sessionStorageFactory
_states is what is used internally by Hapi whenever a cookie is set

That said - when you set the cookie via the sessionStorageFactory it seems like the state object isn't updated with the new value and, like you noticed, you would still get the old cookie value even if you use sessionStorageFactory.asScoped(request).get() again to retrieve the cookie.
Instead, the new value resides in the _states object (and for the auth cookie also in request.auth.artifacts).

Then, upon the next request, the _states object is "flushed" and state is updated with the values from _states.

I haven't gone through all Hapi code yet, and I'm not sure when/how _states is flushed, so feel free to correct me if I'm wrong :)

Anyhow, with that in mind, we can change these lines https://github.com/opensearch-project/security-dashboards-plugin/blob/main/server/session/cookie_splitter.ts#L142-L144
into

if (rawRequest._states[cookieName]) {
  // Set if the value has been updated within the same request
  fullCookieValue = fullCookieValue + rawRequest._states[cookieName].value;
} else if (rawRequest.state[cookieName]) {
     fullCookieValue = fullCookieValue + rawRequest.state[cookieName];
}

The .value comes from the _states object looking like this as opposed to state's key: value:

cookie_name_abc: {
    name: 'cookie_name_abc',
    value: 'eJyrblablasomething'
  },

With this in place, all other changes can be dropped/reverted, which I'd say is the main advantage.

I've tested this locally, and I've also tested to make sure it doesn't affect any existing cookies (re: backwards compatibility).

I would appreciate your input on this - do you think this approach is worth pursuing?
If yes, we could discuss adding some more error handling and also how to test this properly so that regressions like this one doesn't happen again. And again, I'm very sorry for this. I did test the refresh flow, but must have done something wrong.

[cwperks]

@jochen-kressin Thank you very much for the detailed explanation and proposed patch! I would be in favor of referencing the value from _states given that this repo is already relying on the raw request, but I'd also like input from others as well.

@peternied @davidlago @RyanL1997 @scrawfor99 @cliu123 Any thoughts on relying on the raw _states (internal to hapi) to get the most update to date value after token refresh?

[peternied]

@jochen-kressin thanks for the detailed response, I'm sold

With this in place, all other changes can be dropped/reverted, which I'd say is the main advantage.

With this 💯 % on board

[DarshitChanpura]

@cwperks I'm onboard this too. Should we close this in lieu of @jochen-kressin 's PR?

[cwperks]

Closing in favor of #1580