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

Introduce support for the most common features of Spring Security's @PreAuthorize #5787

Merged
merged 4 commits into from Nov 29, 2019

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Nov 26, 2019

Docs will follow and be based on what @aureamunoz is already preparing

Relates to: #4037

@geoand
Copy link
Contributor Author

geoand commented Nov 26, 2019

@michalszynkiewicz can you please review the CDI related logic?

@gsmet would you like to go over the basics to see if I have missed anything obvious? I don't expect you to go over the actual code since it's quite involved :)

The good thing I think about this work is that there is a large amount of tests so all the features should be covered nicely.

@geoand geoand added this to the 1.1.0 milestone Nov 27, 2019
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I added a few minor comment/questions.

Tests look good so it should be pretty safe.

@gsmet
Copy link
Member

gsmet commented Nov 28, 2019

By the way, additional question: do you support precedence via parenthesis, IIRC, that's something we used in our conditions.

@geoand
Copy link
Contributor Author

geoand commented Nov 28, 2019

In this iteration there is precedence support I am afraid. I really wanted to get the basics out there and see what people think and then try to work out what people would like and use the most.
That said, if you have a good and easy way to deal with parentheses and precedence that doesn't require me to learn ANTLR, I'm all ears 😁.
Of course writing a proper parser will probably be necessary at some point...

@gsmet
Copy link
Member

gsmet commented Nov 28, 2019

Couldn't we use an EL implementation somehow instead of doing dark magic? That would be taken care of I suppose.

That being said, I'm for merging this. But you can be sure you will need proper precedence support as that's very important for conditions.

I'm totally positive we used that in our applications at $previousJob.

@geoand
Copy link
Contributor Author

geoand commented Nov 28, 2019

I really don't want to pull in SpEL because who knows what it will take to get it working on native and how many classes it depends on.
I agree that we will definitely need proper precedence and similar features.
I'll ask @mkouba how he implemented those in Qute 😉

@geoand geoand force-pushed the spring-preauthorize branch 3 times, most recently from 6b4c7b9 to 0ec0b8b Compare November 28, 2019 20:59
@gsmet gsmet merged commit adb87d9 into quarkusio:master Nov 29, 2019
@gsmet
Copy link
Member

gsmet commented Nov 29, 2019

@geoand I merged this one, thanks. I also merged @aureamunoz 's work on the Spring Security guide so you should be able to work on the documentation.

@gsmet
Copy link
Member

gsmet commented Nov 29, 2019

BTW, will your parser fail if there are some precedence rules defined in an expression? I would rather have it fail than having it do incorrect things.

@geoand
Copy link
Contributor Author

geoand commented Nov 29, 2019

@gsmet Thanks!

Yes it will currently fail instead of providing incorrect results.

I will work on the documentation next week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants