Skip to content

Add Multi-tenancy support for Reactive Resource Server#6861

Merged
jzheaux merged 1 commit intospring-projects:masterfrom
rhamedy:gh-6727
Jun 13, 2019
Merged

Add Multi-tenancy support for Reactive Resource Server#6861
jzheaux merged 1 commit intospring-projects:masterfrom
rhamedy:gh-6727

Conversation

@rhamedy
Copy link
Copy Markdown
Contributor

@rhamedy rhamedy commented May 11, 2019

Fixes: gh-6727
Fixes: gh-6723

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 11, 2019
@rhamedy rhamedy changed the title Gh 6727 Add Multi-tenancy support for Reactive Resource Server May 11, 2019
@rhamedy
Copy link
Copy Markdown
Contributor Author

rhamedy commented May 11, 2019

@jzheaux I have decided to create a draft pull request to get some feedback before proceeding with updating the javadoc and adding tests.

Seems like draft pull requests does not trigger build, I wish it did that way we would have figured whether the changes added break any tests or not.

@rwinch rwinch requested a review from jzheaux May 13, 2019 19:44
@jzheaux jzheaux self-assigned this May 13, 2019
@jzheaux jzheaux removed the status: waiting-for-triage An issue we've not yet triaged label May 13, 2019
@jzheaux jzheaux added this to the 5.2.x milestone May 13, 2019
jzheaux
jzheaux requested changes May 13, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @rhamedy! I've left some feedback inline.

Also, I figure that you are waiting to add unit tests and updating javadocs until we are further along, so I've left most of that feedback out of this review.

Comment thread ...springframework/security/config/annotation/web/reactive/ServerHttpSecurityConfiguration.java Outdated
Comment thread config/src/main/java/org/springframework/security/config/web/server/ServerHttpSecurity.java Outdated
Comment thread config/src/main/java/org/springframework/security/config/web/server/ServerHttpSecurity.java Outdated
Comment thread ...ain/java/org/springframework/security/web/server/authentication/AuthenticationWebFilter.java Outdated
Comment thread .../java/org/springframework/security/authentication/ReactiveAuthenticationManagerResolver.java Outdated
Comment thread .../java/org/springframework/security/authentication/ReactiveAuthenticationManagerResolver.java Outdated
Comment thread ...ain/java/org/springframework/security/web/server/authentication/AuthenticationWebFilter.java Outdated
@jzheaux jzheaux added 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 labels May 13, 2019
@rhamedy rhamedy marked this pull request as ready for review May 17, 2019 03:53
@rhamedy
Copy link
Copy Markdown
Contributor Author

rhamedy commented May 17, 2019

@jzheaux thank you for the initial review. I think draft was a good idea as I wasn't sure if I have understood the scope of work or not. I have addressed most of your comments and pushed my changes up for review.
I have not written test for all cases and changes, I only added 1 test for the AuthenticationWebFilterTests class for now.
I am curious to what we can and cannot test with regards to changes in this PR?

jzheaux
jzheaux requested changes May 20, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again, @rhamedy. Yes, I think a draft PR was a good idea in the beginning - if you think it's ready, go ahead and had any remaining tests and java docs and the convert into a formal PR.

Comment thread ...ain/java/org/springframework/security/web/server/authentication/AuthenticationWebFilter.java Outdated
Comment thread config/src/main/java/org/springframework/security/config/web/server/ServerHttpSecurity.java Outdated
@rhamedy
Copy link
Copy Markdown
Contributor Author

rhamedy commented Jun 2, 2019

Hi @jzheaux

The switch to flatMap was what I needed. Pushed my changes as well as added some unit tests to the AuthenticationWebFilter.
Not sure if we have to write tests for changes in OAuth2ResourceServerSpec? If yes, where can I find existing tests? The ServerHttpSecurityTest does not seem to have any for OAuth2ResourceServerSpec 😕

Also, please let me know if any changes need to be made in method names (naming is not my strongest skill set), javadoc, and remaining tests (if any).

Thanks for your feedback.

jzheaux
jzheaux requested changes Jun 4, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@jzheaux jzheaux left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhamedy, I answered your question about tests in the PR. Thanks!

Also, it appears that the branch has some conflicts, let me know if you have any questions about resolving those.

Comment thread config/src/main/java/org/springframework/security/config/web/server/ServerHttpSecurity.java Outdated
Comment thread config/src/main/java/org/springframework/security/config/web/server/ServerHttpSecurity.java Outdated
@rhamedy rhamedy force-pushed the gh-6727 branch 2 times, most recently from 15ce1f8 to 6226e6c Compare June 11, 2019 12:55
@rhamedy
Copy link
Copy Markdown
Contributor Author

rhamedy commented Jun 11, 2019 •
edited
Loading

Hey @jzheaux sorry, I had some git hiccup and had to do a few hard resets before seeing my changes re-appear magically. I usually do rebase but, in this case rebasing wanted me to resolve changes not related to my code so I fallback to merging master to my branch and before this PR is in good shape to be merged then I will squash my commits 👍 For now hopefully, I have carried over the master changes to my branch without breaking anything 🙏

I have pushed my changes that contains the test that is getWhenUsingCustomAuthenticationManagerResolverThenUsesItAccordingly using CustomAuthenticationManagerResolverConfig in OAuth2ResourceServerSpecTests and when running the test I get the follwoing error:

java.lang.AssertionError: 
Expected: a string starting with "Bearer error=\"mock-failure\""
     but: was "Bearer"

> GET 
> WebTestClient-Request-Id: [1]
> Authorization: [Bearer eyJhbGciOiJSUzI1NiJ9.eyJzdWIiOiJtb2NrLXN1YmplY3QiLCJzY29wZSI6Im1lc3NhZ2U6cmVhZCIsImV4cCI6NDY4ODY0MTQxM30.cRl1bv_dDYcAN5U4NlIVKj8uu4mLMwjABF93P4dShiq-GQ-owzaqTSlB4YarNFgV3PKQvT9wxN1jBpGribvISljakoC0E8wDV-saDi8WxN-qvImYsn1zLzYFiZXCfRIxCmonJpydeiAPRxMTPtwnYDS9Ib0T_iA80TBGd-INhyxUUfrwRW5sqKRbjUciRJhpp7fW2ZYXmi9iPt3HDjRQA4IloJZ7f4-spt5Q9wl5HcQTv1t4XrX4eqhVbE5cCoIkFQnKPOc-jhVM44_eazLU6Xk-CCXP8C_UT5pX0luRS2cJrVFfHp2IR_AWxC-shItg6LNEmNFD4Zc-JLZcr0Q86Q]

No content

< 401 UNAUTHORIZED Unauthorized
< Pragma: [no-cache]
< Expires: [0]
< X-Frame-Options: [DENY]
< X-Content-Type-Options: [nosniff]
< Referrer-Policy: [no-referrer]
< WWW-Authenticate: [Bearer]
< Cache-Control: [no-cache, no-store, max-age=0, must-revalidate]
< X-XSS-Protection: [1 ; mode=block]

0 bytes of content (unknown content-type).

	at org.springframework.test.web.reactive.server.ExchangeResult.assertWithDiagnostics(ExchangeResult.java:200)
	at org.springframework.test.web.reactive.server.HeaderAssertions.value(HeaderAssertions.java:81)
	at org.springframework.security.config.web.server.OAuth2ResourceServerSpecTests.getWhenUsingCustomAuthenticationManagerResolverThenUsesItAccordingly(OAuth2ResourceServerSpecTests.java:252)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.springframework.test.context.junit4.statements.RunBeforeTestExecutionCallbacks.evaluate(RunBeforeTestExecutionCallbacks.java:74)
	at org.springframework.test.context.junit4.statements.RunAfterTestExecutionCallbacks.evaluate(RunAfterTestExecutionCallbacks.java:84)
	at org.springframework.test.context.junit4.statements.RunBeforeTestMethodCallbacks.evaluate(RunBeforeTestMethodCallbacks.java:75)
	at org.springframework.test.context.junit4.statements.RunAfterTestMethodCallbacks.evaluate(RunAfterTestMethodCallbacks.java:86)
	at org.springframework.security.config.test.SpringTestRule$1.evaluate(SpringTestRule.java:36)
	at org.springframework.test.context.junit4.statements.SpringRepeat.evaluate(SpringRepeat.java:84)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.runChild(SpringJUnit4ClassRunner.java:251)
	at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.runChild(SpringJUnit4ClassRunner.java:97)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.springframework.test.context.junit4.statements.RunBeforeTestClassCallbacks.evaluate(RunBeforeTestClassCallbacks.java:61)
	at org.springframework.test.context.junit4.statements.RunAfterTestClassCallbacks.evaluate(RunAfterTestClassCallbacks.java:70)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.run(SpringJUnit4ClassRunner.java:190)
	at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:86)
	at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:538)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:760)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:460)
	at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:206)
Caused by: java.lang.AssertionError: 
Expected: a string starting with "Bearer error=\"mock-failure\""
     but: was "Bearer"
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:8)
	at org.springframework.test.web.reactive.server.HeaderAssertions.lambda$value$1(HeaderAssertions.java:81)
	at org.springframework.test.web.reactive.server.ExchangeResult.assertWithDiagnostics(ExchangeResult.java:197)
	... 34 more

even though I have stubbed the authentication manager to return error on authentication which leads me to think that the mocked authentication manager is not used 🤔

@jzheaux
Copy link
Copy Markdown
Contributor

jzheaux commented Jun 12, 2019

@rhamedy, I think the problem is that when just authenticationManagerResolver() is called, no filter is getting added.

I'd recommend something similar to this:

if (this.authenticationManagerResolver != null) {
    AuthenticationWebFilter oauth2 = new AuthenticationWebFilter(this.authenticationManagerResolver);
    oauth2.setServerAuthenticationConverter(bearerTokenConverter);
    oauth2.setAuthenticationFailureHandler(new 
            ServerAuthenticationEntryPointFailureHandler(entryPoint));
    http.addFilterAt(oauth2, SecurityWebFiltersOrder.AUTHENTICATION);
} else if (this.jwt != null) {
    this.jwt.configure(http);
} else if (this.opaqueToken != null) {
    this.jwt.configure(http);
}

The extra support in jwt.configure and opaqueToken.configure for resolvers is unnecessary, which aligns with the fact that these are mutually exclusive configurations.

@rhamedy rhamedy force-pushed the gh-6727 branch 2 times, most recently from 936941f to 28b3181 Compare June 13, 2019 04:40
@rhamedy
Copy link
Copy Markdown
Contributor Author

rhamedy commented Jun 13, 2019

Hi @jzheaux thanks for hint above, that seemed to be the issue with failing tests. I squashed my commits and pushed my changes. Please let me know if there is need for more tests and/or javadoc.

jzheaux
jzheaux requested changes Jun 13, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhamedy, this looks great, thank you again.

Just one quick change, which I've described inline, and we should be good.

Comment thread ...ain/java/org/springframework/security/web/server/authentication/AuthenticationWebFilter.java Outdated
@rhamedy
Introduced ReactiveAuthenticationManagerResolver
610343c 
Suitable for multi-tenant reactive applications needing to branch
authentication strategies based on request details.
@jzheaux jzheaux merged commit f6ed1db into spring-projects:master Jun 13, 2019
@jzheaux
Copy link
Copy Markdown
Contributor

jzheaux commented Jun 13, 2019

Thanks, @rhamedy, this is now merged into master!

@jzheaux jzheaux modified the milestones: 5.2.x, 5.2.0.M3 Jun 13, 2019
@rhamedy rhamedy deleted the gh-6727 branch June 13, 2019 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jzheaux jzheaux jzheaux approved these changes

Assignees

@jzheaux jzheaux

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

Milestone

5.2.0.M3

Development

Successfully merging this pull request may close these issues.

Multi-tenancy for Reactive Resource Server Introduce ReactiveAuthenticationManagerResolver

3 participants

@rhamedy @jzheaux @spring-projects-issues