Add support for Spring Security's @Secured #5225

aureamunoz · 2019-11-05T17:31:56Z

This PR contains a new extension spring-security implementing @Secured annotation.
It's based on #5074 and is a draft.
I let you, @geoand and @michalszynkiewicz , take a look and forward to others if needed.
Thank you

gsmet · 2019-11-05T17:37:59Z

@aureamunoz quick question, I haven't looked at the implementation: does it support the EL Spring Security has (things such as hasRole() and so on)? I suppose not. I wonder what would be necessary for us to support that because it's very useful in practice.

gsmet · 2019-11-05T17:39:07Z

BTW, I should have started by that: it's an exciting new addition.

geoand · 2019-11-05T21:11:06Z

Nice work!

To answer your question @gsmet, the stuff you describe I am pretty sure is covered by Spring's @PreAuthorize which I'll probably be picking up soon

netodevel · 2019-11-08T17:15:53Z

@aureamunoz,

Suggestion:

I don't know if it's possible, but it would be nice to have security configuration support.
Something like:

http
.authorizeRequests()
.antMatchers("/hello/**").allowAll()

geoand · 2019-11-08T17:19:13Z

@netodevel we aren't planning to support that style of configuration at this time. That doesn't mean that we won't in the future, but just that it's not in the immediate plans.

netodevel · 2019-11-08T20:59:47Z

@geoand,
Ok, I'll follow this issue to understand and try to contribute in the future.
Thanks for answering me.

geoand · 2019-11-08T21:04:04Z

You are welcome!

geoand · 2019-11-09T14:57:51Z

@michalszynkiewicz @aureamunoz what is the status of #5074?

Is there anything I can do to help?

michalszynkiewicz · 2019-11-09T18:49:47Z

@geoand imo done, Stuart asked if I can use index instead of looping throug all classes but AFAIK I can't

geoand · 2019-11-09T19:16:39Z

Which part of your PR is the code in question @michalszynkiewicz ?

I can take a look on Monday if you like

michalszynkiewicz · 2019-11-09T19:43:33Z

https://github.com/quarkusio/quarkus/pull/5074/files#diff-36e991a830b10f10b19a4360212eebcaR194

geoand · 2019-11-11T06:38:25Z

I'll be looking at #5074 today

geoand · 2019-11-11T10:32:56Z

@michalszynkiewicz Nice work!

I was thinking of implementing this in a slightly different manner however. The basic premise that I have in mind is that methods need to be "mapped" to a SecurityCheck.
With that in mind things like Spring's @Secured would not need to use the annotation transformer, the extension could just map methods to the appropriate RolesAllowed check.

There are probably a few mechanics involved to make this work properly but I will start from your PR and build on it to incorporate the solution I have in mind which I think should turn out to be simpler and avoid the need to go over all classes in the index.

geoand · 2019-11-11T12:09:24Z

I'm going to try to come up with a way to do this that will also allow for the types of SecurityChecks that will be needed for Spring's @PreAuthorize

This is done by allowing the registration of arbitrary SecurityCheck implementations. Furthermore the use of the AnnotationStore is hidden so as to not give the impression that it's a feature that magically makes security annotations work This change paves the way for quarkusio#5225 and more related improvements in the future

geoand · 2019-11-18T09:37:59Z

I just updated the PR to align with the changes made in the core Quarkus security

geoand · 2019-11-18T09:50:25Z

One thing that is missing is an integration test that will ensure this works in native mode

michalszynkiewicz · 2019-11-18T11:28:46Z

@geoand maybe add it to the main module of the integration tests? It already has security enabled

geoand · 2019-11-18T11:35:06Z

@michalszynkiewicz we don't want to add Spring Security to the main module because that one should not depend on any Spring things.
We could probably just add it to the Spring Web integration test in order to avoid having a completely new integration test that will take up a lot of time.
The reason I want an integration test for Spring security is that there could perhaps be problems with the Spring Security artifact that will only show up in native mode - and that might require maven exclusions or GraalVM substitutions.

michalszynkiewicz · 2019-11-18T19:21:13Z

@geoand if you ask me, we should have native tests for all the stuff. OTOH, the native test modules are really costly.
+1 from me for adding it to spring web integration tests if we have a module for them.

geoand · 2019-11-18T20:19:37Z

We do have a Spring Web integration test module so I propose @aureamunoz add them there

aureamunoz · 2019-11-18T20:40:21Z

We do have a Spring Web integration test module so I propose @aureamunoz add them there

Yes, of course. It's what I'm doing, sorry for not saying nothing until now 😏

This is done by allowing the registration of arbitrary SecurityCheck implementations. Furthermore the use of the AnnotationStore is hidden so as to not give the impression that it's a feature that magically makes security annotations work This change paves the way for quarkusio#5225 and more related improvements in the future

geoand · 2019-11-19T21:11:51Z

@aureamunoz can you please squash the second commit?
I don't think that in this case it makes sense to have a second one.
If you add a guide however, then that would make an excellent second commit

geoand · 2019-11-19T21:13:04Z

...ment/src/main/java/io/quarkus/spring/security/deployment/SpringSecurityTransformerUtils.java

+import org.jboss.jandex.MethodInfo;
+import org.springframework.security.access.annotation.Secured;
+
+public class SpringSecurityTransformerUtils {


Is this class needed? I don't think it's used, correct?

...ployment/src/test/java/io/quarkus/spring/security/deployment/SpringSecuredAnnotatedTest.java

Co-authored-by: geoand <geoand@gmail.com>

Haven't checked the logic itself but the things I noted were addressed.

michalszynkiewicz · 2019-11-21T08:43:19Z

It wouldn't hurt to have a test for the newly added check but that's not a big issue.
LGTM

geoand · 2019-11-21T08:45:08Z

It wouldn't hurt to have a test for the newly added check but that's not a big issue.
LGTM

There is one test, indeed there could have been more :)

geoand · 2019-11-21T08:45:24Z

Thanks for all the work @aureamunoz and for all the support @michalszynkiewicz !

michalszynkiewicz · 2019-11-21T09:00:25Z

Thanks for all the work @aureamunoz !

👍 good job!

It wouldn't hurt to have a test for the newly added check but that's not a big issue.
LGTM

There is one test, indeed there could have been more :)

sorry, I missed that.

geoand · 2019-11-21T09:04:44Z

sorry, I missed that.

No worries!
Thanks again for your help!

This is done by allowing the registration of arbitrary SecurityCheck implementations. Furthermore the use of the AnnotationStore is hidden so as to not give the impression that it's a feature that magically makes security annotations work This change paves the way for quarkusio#5225 and more related improvements in the future

Related to quarkusio#5225

This is done by allowing the registration of arbitrary SecurityCheck implementations. Furthermore the use of the AnnotationStore is hidden so as to not give the impression that it's a feature that magically makes security annotations work This change paves the way for quarkusio#5225 and more related improvements in the future

Related to quarkusio#5225

geoand mentioned this pull request Nov 13, 2019

CDI security determined at build time #5074

Merged

geoand mentioned this pull request Nov 14, 2019

Make build registration of security interceptors more flexible #5483

Merged

geoand force-pushed the spring-secured-extension branch from f60a495 to 6a2b5f7 Compare November 18, 2019 09:23

aureamunoz force-pushed the spring-secured-extension branch from 6a2b5f7 to 5c90300 Compare November 19, 2019 20:54

geoand reviewed Nov 19, 2019

View reviewed changes

aureamunoz force-pushed the spring-secured-extension branch from 5c90300 to e5ac403 Compare November 20, 2019 08:55

Add Spring Security extension for @secured implementation

6e7f5e3

Co-authored-by: geoand <geoand@gmail.com>

geoand force-pushed the spring-secured-extension branch from 760f6ad to 6e7f5e3 Compare November 20, 2019 21:56

michalszynkiewicz approved these changes Nov 21, 2019

View reviewed changes

geoand merged commit 5de59a5 into quarkusio:master Nov 21, 2019

gsmet added this to the 1.1.0 milestone Nov 24, 2019

aureamunoz added a commit to aureamunoz/quarkus that referenced this pull request Nov 25, 2019

fix: add spring-security extension metadata

9942575

Related to quarkusio#5225

aureamunoz mentioned this pull request Nov 25, 2019

fix: add spring-security extension metadata #5738

Merged

gsmet pushed a commit to aureamunoz/quarkus that referenced this pull request Nov 25, 2019

fix: add spring-security extension metadata

b5ac769

Related to quarkusio#5225

paulrobinson mentioned this pull request Nov 26, 2019

Spring Security Support #5663

Closed

2 tasks

geoand mentioned this pull request Nov 26, 2019

Introduce support for Spring Security #4037

Closed

2 tasks

gsmet added the release/noteworthy-feature label Dec 9, 2019

mmusgrov pushed a commit to mmusgrov/quarkus that referenced this pull request Dec 13, 2019

fix: add spring-security extension metadata

cc31138

Related to quarkusio#5225

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Spring Security's @Secured #5225

Add support for Spring Security's @Secured #5225

aureamunoz commented Nov 5, 2019

gsmet commented Nov 5, 2019

gsmet commented Nov 5, 2019

geoand commented Nov 5, 2019

netodevel commented Nov 8, 2019

geoand commented Nov 8, 2019

netodevel commented Nov 8, 2019

geoand commented Nov 8, 2019

geoand commented Nov 9, 2019

michalszynkiewicz commented Nov 9, 2019

geoand commented Nov 9, 2019

michalszynkiewicz commented Nov 9, 2019

geoand commented Nov 11, 2019

geoand commented Nov 11, 2019 •

edited

Loading

geoand commented Nov 11, 2019

geoand commented Nov 18, 2019

geoand commented Nov 18, 2019

michalszynkiewicz commented Nov 18, 2019

geoand commented Nov 18, 2019

michalszynkiewicz commented Nov 18, 2019 •

edited

Loading

geoand commented Nov 18, 2019 •

edited

Loading

aureamunoz commented Nov 18, 2019

geoand commented Nov 19, 2019

geoand Nov 19, 2019

michalszynkiewicz commented Nov 21, 2019

geoand commented Nov 21, 2019

geoand commented Nov 21, 2019 •

edited

Loading

michalszynkiewicz commented Nov 21, 2019 •

edited

Loading

geoand commented Nov 21, 2019

Add support for Spring Security's @Secured #5225

Add support for Spring Security's @Secured #5225

Conversation

aureamunoz commented Nov 5, 2019

gsmet commented Nov 5, 2019

gsmet commented Nov 5, 2019

geoand commented Nov 5, 2019

netodevel commented Nov 8, 2019

geoand commented Nov 8, 2019

netodevel commented Nov 8, 2019

geoand commented Nov 8, 2019

geoand commented Nov 9, 2019

michalszynkiewicz commented Nov 9, 2019

geoand commented Nov 9, 2019

michalszynkiewicz commented Nov 9, 2019

geoand commented Nov 11, 2019

geoand commented Nov 11, 2019 • edited Loading

geoand commented Nov 11, 2019

geoand commented Nov 18, 2019

geoand commented Nov 18, 2019

michalszynkiewicz commented Nov 18, 2019

geoand commented Nov 18, 2019

michalszynkiewicz commented Nov 18, 2019 • edited Loading

geoand commented Nov 18, 2019 • edited Loading

aureamunoz commented Nov 18, 2019

geoand commented Nov 19, 2019

geoand Nov 19, 2019

Choose a reason for hiding this comment

michalszynkiewicz commented Nov 21, 2019

geoand commented Nov 21, 2019

geoand commented Nov 21, 2019 • edited Loading

michalszynkiewicz commented Nov 21, 2019 • edited Loading

geoand commented Nov 21, 2019

geoand commented Nov 11, 2019 •

edited

Loading

michalszynkiewicz commented Nov 18, 2019 •

edited

Loading

geoand commented Nov 18, 2019 •

edited

Loading

geoand commented Nov 21, 2019 •

edited

Loading

michalszynkiewicz commented Nov 21, 2019 •

edited

Loading