-
Notifications
You must be signed in to change notification settings - Fork 6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Multi tenancy for Resource Server #6563
Conversation
@rwinch I'm using only Originally, I tried out an This has a nice cognate on the reactive side, injecting a custom I'm open to discussion here, of course. |
Related #5385 |
} | ||
|
||
AuthenticationDetailsSource<HttpServletRequest, ?> authenticationDetailsSource() { | ||
return request -> request.getPathInfo().split("/")[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jzheaux I don't think it makes sense to use AuthenticationDetailsSource
for extracting the tenant info. In this example, AuthenticationManager
indirectly relies on HttpServletRequest
which is not the intended design for AuthenticationManager
as it's not meant to be tied to the web-tier - it should work on a desktop application for example.
Also, the tenant info may come from a request parameter so I'm not sure how this would work with the current setup.
Either way, I feel we need to extract the tenant info in the web-tier and populate it with a new impl of Authentication
that is passed into AuthenticationManager
. We may even consider a new type that is populated in a AuthenticationDetailsSource
? I just think we should leave the extracting of the tenant info in the web-tier - Filter
or something similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this example,
AuthenticationManager
indirectly relies onHttpServletRequest
What do you mean that it is indirectly reliant? By that same logic, isn't it also indirectly reliant on HttpServletRequest
because of the bearer token resolver? No web-ness makes it down into the authentication manager. We just have a tenantId that so happens to be the path.
This also happens on the reactive side. The AuthenticationWebFilter
calls a ServerAuthenticationConverter
, which takes a request
and returns an Authentication
, which is then passed to a manager.
We may even consider a new type that is populated in a `AuthenticationDetailsSource
A stronger type may be in order, as you said, though. Calling setDetails(Object)
doesn't offer any type safety, nor any direction on what ought to go there. However, I don't think that I'd want to enforce it at the setter level. For example, I wouldn't want to do this:
public void setAuthenticationDetailsSource
(AuthenticationDetailsSource<HttpServletRequest, Tenant>)
Since there are certainly other scenarios where the user would like to provide details in the Authentication
object.
I just think we should leave the extracting of the tenant info in the web-tier -
Filter
or something similar.
Just to be clear, this is already technically the case since the filter calls source.buildDetails
before invoking the authentication manager. But, I'm thinking you are calling out something more nuanced than that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jzheaux Regarding my comment
AuthenticationManager
indirectly relies onHttpServletRequest
which is not the intended design...
Please ignore as I misinterpreted the implementation on my initial look. All seems fine. Actually, I like the fact that you're leveraging Authentication.details
to store the tenant info. This allows for an easy integration.
I'm wondering if it makes sense to leverage (or extend) WebAuthenticationDetailsSource
and store the tenant info in WebAuthenticationDetails
or subclass of it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how this turned out. It is a simple and yet very powerful change.
Suitable for multi-tenant applications needing to branch authentication strategies based on request details. Fixes: spring-projectsgh-6722
No description provided.