Make OIDC Back-Channel Logout API Public #13767
Labels
for: team-attention
This ticket should be discussed as a team before proceeding
in: oauth2
An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose)
type: enhancement
A general enhancement
In #12570, it was decided to make the OIDC Back-Channel Logout API private for now to allow the feature to be released at the DSL level while we continue to take time to get the API right.
So that we can come back to it, I'm posting my current understanding here:
JavaDoc
OidcBackChannelLogoutFilter
is a filter that authenticates and then performs a logout (see more here). Here are the truncated JavaDocs forAuthenticationManager
andLogoutHandler
:and
respectively. Because of that, it makes sense that such a filter will have an
AuthenticationManager
and aLogoutHandler
Why Not Everything in AuthenticationManager?
The suggestion to place everything inside
AuthenticationManager
seems to come from a place where more APIs are needless: Why use two APIs when you can use one? It's a fair question and I appreciate @jgrandja asking it.In short, we could extract the needed
HttpServletRequest
elements to perform the back-channel logout; however, the benefits don't seem to outweigh the drawbacks.To move the needed request material through the contract, we'd need to include the request's base URL in the
OidcLogoutAuthenticationToken
. In my view, we should not do this. Such would create at least a token like this:which is a code smell since while
baseUrl
doesn't directly refer to theHttpServletRequest
, it is only needed because of the specific strategy we are currently using for logging out, meaning that it leaks implementation details.Moreover, other customizations to the logout strategy may require the request. If a user needs to include more request material themselves, they would need to customize 1) the authentication token, 2) the authentication converter, and 3) the authentication manager.
This API leakage and the potential extra developer boilerplate are not needed since we already have
LogoutHandler
which takes anHttpServletRequest
directly.For those reasons as well as the ones already stated here and here (option2), I do not think it is wise to do more than verify the credential and populate the principal in
AuthenticationManager
.Here is a branch that folds everything into
AuthenticationManager
.Familiarity and Consistency
One piece of feedback that has been repeated is that we should be consistent with the other OAuth 2.0 filters. This was voiced in #12570 here, here, and here, with very good reason. And because the existing OAuth 2.0 filters are consistent with other common Spring Security filters, this should also be consistent with the rest of Spring Security as well.
NOTE: It's hard, IMO, to prove API consistency. So while this section takes up the most space, I don't see it as more important than the earlier two points.
The following analysis is based on how
OidcBackChannelLogoutFilter
compares toOAuth2LoginAuthenticationFilter
. Note, though, that my same reasoning holds at least for the remaining OAuth 2.0 filters, the SAML 2.0 login/logout filters,AuthenticationFilter
,LogoutFilter
, andUsernamePasswordAuthenticationFilter
. My analysis doesn't extend beyond those and is open for scrutiny at least along those lines.OAuth2LoginAuthenticationFilter
has four main sections to enable its behavior:Each of these sections is a collaboration between various Spring Security components.
RequestMatcher
, theAuthorizationRequestRepository
, and theClientRegistrationRepository
.authorization_code
usingAuthenticationManager
. (the service layer)OAuth2AuthorizedClientRepository
,SecurityContextRepository
,SecurityContextHolderStrategy
, and a set ofSessionAuthenticationStrategy
s.AuthenticationSuccessHandler
andAuthenticationFailureHandler
to affect the response.(For reference, this can be seen a bit more succinctly in the simplest cases,
AuthenticationFilter
andLogoutFilter
)The main reason for this arrangement is that components 1, 2, and 4 require access to the
HttpServletRequest
and/orHttpServletResponse
to operate correctly. While hypothetically Spring Security could have created services for each one of these and required the same kind of request-extraction logic to operate each one, it instead chose a simpler route for these separate components.It's important to observe at this point that each of these components is there for our legitimate use, in accordance with the JavaDoc.
OAuth2LoginAuthenticationFilter
and the other filters listed each enforce important contractual boundaries that are what give each filter a level of familiarity to the community.Before proceeding to
OidcBackChannelLogoutFilter
, I feel it's important to note that this filter is novel in Spring Security OAuth 2.0 Client since it performs both an authentication (to verify the token) and a logout. Indeed, one concern that was brought up offline was that maybe we shouldn't be comparing this toOAuth2LoginAuthenticationFilter
after all.In the absence of any other OAuth 2.0 filters to compare with, I believe that
LogoutFilter
andSaml2LogoutRequestFilter
are good comparisons as well to inform how this filter should be formulated.LogoutFilter
follows this pattern in the following way:CsrfFilter
CsrfFilter
LogoutHandler
sLogoutSuccessHandler
to affect the response.It's certainly true that
LogoutFilter
is so simple, that the first two points are arguably negligible. So, let's look atSaml2LogoutRequestFilter
for something more involved.Saml2LogoutRequestFilter
follows this pattern in the following way:Saml2LogoutRequestParametersResolver
Saml2LogoutRequestValidator
(the service layer)LogoutHandler
sSaml2LogoutResponseResolver
and some private code to affect the HTTP responseIn my attempt to follow this pattern,
OidcBackChannelLogoutFilter
is designed in the following way:AuthenticationConverter
logout_token
usingAuthenticationManager
(the service layer)LogoutHandler
(which ultimately usesOidcSessionStrategy
).LogoutSuccessHandler
to affect the response.Or pictorially, I see it like this:
Some of this has evolved based on @jgrandja and @rwinch's feedback. For example, I originally had new APIs for steps 2 and 3 in this PR. @jgrandja encouraged me to use
AuthenticationManager
for step 2, which the PR now does and @rwinch encouraged me to useLogoutHandler
for step 3, which was also applied. Of course, thanks to these two for helping get this PR this far. That excellent feedback notwithstanding, this four-step pattern was already there in the initial stages, before the refinements to reuse existing APIs. This is important to note since it further demonstrates the pervasiveness of this pattern in spite of the APIs we end up using.What then?
Because
HttpServletRequest
, andThe current arrangement seems like the approach is what will be the most familiar, the least surprising, and the most extendable. So, I'd prefer to make the API public as-is.
The text was updated successfully, but these errors were encountered: