Refactor: Create Login State Values Once

[strehle]

If IdP selection is not active, but all IdP are listed in login page, then the state and PKCE values are generated with each request and stored in the current HTTP session. If there are parallel action, e.g. different browser tabs, then this can lead to login issues.

Refactor so that these states are created once per Http session and re-use them in other requests, e.g. browser tabs

[Copilot]

Pull request overview

This pull request refactors the OAuth state and PKCE parameter generation to address race conditions when multiple browser tabs access IdP login simultaneously. Instead of generating new state and PKCE values on each request, the code now caches and reuses these values per HTTP session, ensuring consistency across parallel requests to the same IdP.

Key changes:

• Modified SessionUtils.setStateParam() to return and reuse cached values instead of only storing them
• Added session caching for PKCE code challenge values alongside existing code verifier caching
• Updated ExternalOAuthProviderConfigurator.getIdpAuthenticationUrl() to utilize the new caching behavior

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File	Description
SessionUtils.java	Changed setStateParam to return cached values and added code challenge caching support
ExternalOAuthProviderConfigurator.java	Updated to use returned values from setStateParam for state and PKCE parameters

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The method signature has been changed from void to String without updating the method's documentation or JavaDoc. This is a breaking API change that affects how the method is used. Callers now need to use the return value instead of relying on side effects alone.

Consider adding JavaDoc to document:

1. The return value represents the state parameter value (either newly generated or retrieved from session)
2. The behavior of reusing cached values when they exist in the session
3. That null may be returned if the cached value is not a String (though this should be addressed separately)

[Copilot]

The naming convention for the new constant EXTERNAL_OAUTH_CODE_CHALLENGE_ATTRIBUTE_PREFIX follows the established pattern of similar constants (EXTERNAL_OAUTH_STATE_ATTRIBUTE_PREFIX and EXTERNAL_OAUTH_CODE_VERIFIER_ATTRIBUTE_PREFIX). However, consider whether storing the code challenge in the session is necessary.

In PKCE, the code challenge is derived from the code verifier and sent to the authorization endpoint, while only the code verifier needs to be retrieved from the session later to send to the token endpoint. The code challenge is not used after the authorization request, so caching it provides no functional benefit and only increases session size.

If the code challenge cache is kept for consistency with the refactoring approach, this is acceptable but should be documented as to why it's cached.

[Copilot]

The code challenge is being computed from a potentially cached code verifier, but the code challenge itself is also being cached. This creates an inconsistency problem: if the code verifier already exists in the session, the code will compute a new challenge from that existing verifier but then attempt to cache this newly computed challenge. However, if a code challenge already exists in the session, it will be returned instead of the newly computed one, leading to a mismatch between the cached verifier and cached challenge.

The issue arises because pkceVerifier.compute() always computes a fresh challenge, but setStateParam may return a previously cached challenge that was computed from a different execution path or even a different verifier value.

To fix this, the code challenge should be computed once when the code verifier is first generated, and both should be cached together atomically. If the code verifier already exists in the session, the cached code challenge should be retrieved directly without recomputation.

Suggested change

	var codeVerifier = SessionUtils.setStateParam(request.getSession(), SessionUtils.codeVerifierParameterAttributeKeyForIdp(idpOriginKey), generateCodeVerifier());
	var codeChallenge = SessionUtils.setStateParam(request.getSession(), SessionUtils.codeChallengeParameterAttributeKeyForIdp(idpOriginKey), pkceVerifier.compute(codeVerifier));
	var session = request.getSession();
	var codeVerifierKey = SessionUtils.codeVerifierParameterAttributeKeyForIdp(idpOriginKey);
	var codeChallengeKey = SessionUtils.codeChallengeParameterAttributeKeyForIdp(idpOriginKey);
	String codeVerifier = (String) session.getAttribute(codeVerifierKey);
	String codeChallenge = (String) session.getAttribute(codeChallengeKey);
	if (codeVerifier == null) {
	// First-time generation: create verifier and challenge and cache both atomically
	codeVerifier = generateCodeVerifier();
	codeChallenge = pkceVerifier.compute(codeVerifier);
	session.setAttribute(codeVerifierKey, codeVerifier);
	session.setAttribute(codeChallengeKey, codeChallenge);
	} else if (codeChallenge == null) {
	// Verifier exists but challenge does not: compute and cache from existing verifier
	codeChallenge = pkceVerifier.compute(codeVerifier);
	session.setAttribute(codeChallengeKey, codeChallenge);
	}

[Copilot]

The setStateParam method returns null when the session attribute exists but is not a String instance. This could lead to a null state value being used in the OAuth flow, which would cause authentication failures. The calling code in ExternalOAuthProviderConfigurator does not check for null return values.

While type safety should prevent non-String values from being stored, defensive error handling or logging would be more robust than silently returning null. Consider throwing an IllegalStateException or logging a warning if the cached value is of an unexpected type.

Suggested change

	if (session.isNew() || session.getAttribute(stateParamKey) == null) {
	session.setAttribute(stateParamKey, state);
	return state;
	} else {
	return session.getAttribute(stateParamKey) instanceof String existingState ? existingState : null;
	Object existingAttribute = session.getAttribute(stateParamKey);
	if (session.isNew() || existingAttribute == null) {
	session.setAttribute(stateParamKey, state);
	return state;
	} else if (existingAttribute instanceof String) {
	return (String) existingAttribute;
	} else {
	throw new IllegalStateException(
	"Expected session attribute '" + stateParamKey + "' to be a String but found: " +
	existingAttribute.getClass().getName()
	);

[Copilot]

The condition 'session.isNew()' may not correctly identify all cases where state parameters should be regenerated. According to the Servlet specification, isNew() returns true only for newly created sessions where the client hasn't yet acknowledged the session (i.e., hasn't sent back the session ID). This means:

1. If a session exists but the state attribute is null (e.g., after session timeout and recreation), isNew() could be false, and a new state would be generated correctly via the second condition.
2. However, if isNew() is true but the attribute already exists (which shouldn't normally happen), the existing value would be overwritten.

The logic appears functional but could be clearer. Consider simplifying to just check if the attribute is null, which is the actual condition that matters for determining whether to generate or reuse state values.

Suggested change

	if (session.isNew() || session.getAttribute(stateParamKey) == null) {
	session.setAttribute(stateParamKey, state);
	return state;
	} else {
	return session.getAttribute(stateParamKey) instanceof String existingState ? existingState : null;
	Object existing = session.getAttribute(stateParamKey);
	if (existing == null) {
	session.setAttribute(stateParamKey, state);
	return state;
	} else {
	return existing instanceof String existingState ? existingState : null;

[adrianhoelzl-sap]

Can we add a comment or change the method's name to represent this behavior? If a value already exists, we do not update it.

[adrianhoelzl-sap]

Can we replace var with the concrete types here?