Closed leshalv closed 1 week ago
At present, the Spring Authorization Server supports OpenID Connect Authentication using the authorization_code
flow only. The sid
claim is optional, and adding it to non-authorization code requests might seem unnecessary at first glance. However, there are several compelling reasons to consider including the sid
claim even in the current setup:
sid
ClaimFuture-proofing for Additional Flows:
sid
claim now sets the stage for future support of other OpenID Connect flows, such as implicit
or hybrid
flows. This proactive step can simplify future updates and reduce the need for extensive modifications when additional flows are introduced.Enhanced Session Management:
authorization_code
flow, the sid
claim can be beneficial for session management. It can help in tracking user sessions more effectively, providing a clearer picture of user activity and aiding in session-related operations such as logout and session revocation.Consistency Across Implementations:
sid
claim, the Spring Authorization Server can align more closely with the OpenID Connect specification and other implementations that support multiple flows. This consistency can make it easier for developers familiar with OpenID Connect to work with the Spring Authorization Server.Improved User Experience:
Security Considerations:
sid
claim can also contribute to security enhancements. By clearly identifying user sessions, it becomes easier to detect and manage session hijacking attempts or other malicious activities. This additional layer of security can be valuable in protecting user data and ensuring secure authentication flows.@leshalv
Even within the
authorization_code
flow, thesid
claim can be beneficial for session management. It can help in tracking user sessions more effectively, providing a clearer picture of user activity and aiding in session-related operations such as logout and session revocation
I don't understand why this is listed since the sid
claim is already included in the authorization_code
flow.
By including the
sid
claim, the Spring Authorization Server can align more closely with the OpenID Connect specification and other implementations that support multiple flows. This consistency can make it easier for developers familiar with OpenID Connect to work with the Spring Authorization Server.
This comment is too general. What part of the current implementation does not align with the spec? Please be specific and reference the part of the spec that the current implementation does not align with.
The comments under "Improved User Experience" and "Security Considerations" are also too general and speculative. Again, if you see room for improvement, please be very specific and demonstrate there is an issue with the current implementation.
The inclusion of the
sid
claim now sets the stage for future support of other OpenID Connect flows, such asimplicit
orhybrid
flows
FYI, there are no plans to support the implicit
or hybrid
flows.
Since the sid
claim is used only in the authorization_code
flow in the current implementation, I'm going to close this PR. If we decide to support additional flows, we can add the sid
claim at that point.
When using it, if you want to extend other authentication methods, such as implicit authorization, you also need sid and nonce, I don't think you have to judge whether it is authorization_code here, because the id_token itself can contain sid and nonce, which is convenient for better expansion and reduces duplicate boilerplate code. @jgrandja
if you want to extend other authentication methods, such as implicit authorization, you also need sid and nonce
You can register a custom OAuth2TokenCustomizer<JwtEncodingContext>
@Bean
for adding the sid
or nonce
claim
@leshalv I'm trying to understand the reason for this change.
The
sid
claim is specified as follows:At the moment, Spring Authorization Server supports OpenID Connect Authentication using
authorization_code
flow only. So it doesn't make sense to add thesid
claim to non-authorization code requests.As an FYI, before submitting a PR, please log an issue first describing the proposed enhancement so we can discuss beforehand.
Please provide your reasoning on this proposed change.