Skip to content
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

Merged
merged 2 commits into from
Mar 29, 2019

Conversation

jzheaux
Copy link
Contributor

@jzheaux jzheaux commented Feb 26, 2019

No description provided.

@jzheaux jzheaux added the wip label Feb 26, 2019
@jzheaux jzheaux requested a review from rwinch February 26, 2019 15:39
@jzheaux
Copy link
Contributor Author

jzheaux commented Feb 26, 2019

@rwinch I'm using only AuthenticationManager to support multi-tenancy. The reason is because it's possible for Authentication to contain all the necessary information to resolve tenant differences.

Originally, I tried out an AuthenticationManagerResolver.resolve(HttpServletRequest), though I'm of the mind that there's value in this interface not being tied to the request. In my experimentation with alternatives, it was simpler (no new interfaces) to simply allow the user to supply a custom AuthenticationManager and a custom AuthenticationDetailsSource.

This has a nice cognate on the reactive side, injecting a custom ServerAuthenticationConverter to get an Authentication, and then send that Authentication to a custom ReactiveAuthenticationManager.

I'm open to discussion here, of course.

@jgrandja
Copy link
Contributor

Related #5385

}

AuthenticationDetailsSource<HttpServletRequest, ?> authenticationDetailsSource() {
return request -> request.getPathInfo().split("/")[1];
Copy link
Contributor

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.

Copy link
Contributor Author

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 on HttpServletRequest

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.

Copy link
Contributor

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 on HttpServletRequest 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?

@jzheaux jzheaux removed the wip label Mar 29, 2019
@jzheaux jzheaux marked this pull request as ready for review March 29, 2019 17:49
Copy link
Member

@rwinch rwinch left a 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.

jzheaux added 2 commits March 29, 2019 13:16
Suitable for multi-tenant applications needing to branch
authentication strategies based on request details.

Fixes: spring-projectsgh-6722
@jzheaux jzheaux added status: duplicate A duplicate of another issue New Feature in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels Mar 29, 2019
@jzheaux jzheaux added this to the 5.2.0.M2 milestone Mar 29, 2019
@jzheaux jzheaux merged commit 7e8aade into spring-projects:master Mar 29, 2019
@jzheaux jzheaux deleted the multi-tenancy branch March 29, 2019 21:09
@jzheaux jzheaux self-assigned this Apr 14, 2019
@rwinch rwinch added the type: enhancement A general enhancement label May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants