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

Support repeated wildcards in the HTTP permissions path matching patterns #37192

Merged

Conversation

michalvavrik
Copy link
Member

closes: #14047

Until now it was only possible to use wildcard at the end of the path, now you can use it anywhere. Inner wildcard matches exactly one path segment. Behavior for path patterns with ending wildcard or exact path matches didn't change at all.

@quarkus-bot quarkus-bot bot added area/docstyle issues related for manual docstyle review area/documentation area/vertx labels Nov 18, 2023
@michalvavrik
Copy link
Member Author

@stuartwdouglas I'd be truly grateful if you could review this PR when the time is right for you. It's still your path matching algorithm, I just added bit of recursion. So it's still prefix path matching, but /one/*/two/* is divided to /one and the path match contains path matcher for prefix paths behind /*/, here /two.

Copy link

github-actions bot commented Nov 18, 2023

🙈 The PR is closed and the preview is expired.

This comment has been minimized.

Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

All looks precise to me, lets wait for Stuart @stuartwdouglas to have a quick look too

@sberyozkin
Copy link
Member

@michalvavrik Michal, I wonder if it would make sense to add an Important message in the docs, highlighting the importance of testing in general but especially when more than one wildcard is used - to avoid users introducing accidental bypasses.
By the way, I think it can make sense to update the unit test where you check // cases with a few cases of multiple wildcards, I believe by the time this new matcher code is done the path has already been normalized, so a few basic tests will do

@michalvavrik
Copy link
Member Author

michalvavrik commented Nov 19, 2023

@michalvavrik Michal, I wonder if it would make sense to add an Important message in the docs, highlighting the importance of testing in general but especially when more than one wildcard is used - to avoid users introducing accidental bypasses.

That sounds very reasonable. I'll do that.

By the way, I think it can make sense to update the unit test where you check // cases with a few cases of multiple wildcards, I believe by the time this new matcher code is done the path has already been normalized, so a few basic tests will do

@sberyozkin unit test I added to the PathMatchingHttpSecurityPolicyTest is just a smoke test. As long as PathMatcherTest is passing and path is normalized, there is no difference. And the PathMatcherTest tests everything I could think of. So I don't think it is necessary, but it is easy to do and I'll add few other // test cases with multiple wildcards.

@michalvavrik michalvavrik force-pushed the feature/path-matching-wildcard-latest branch from 34ef0d6 to 1e40a7d Compare November 19, 2023 22:19

This comment has been minimized.

@michalvavrik
Copy link
Member Author

Considering I just changed test in different module and docs and previously Hibernate didn't fail, I think integration-tests/hibernate-orm-tenancy/connection-resolver-legacy-qualifiers are unrelated.

@michalvavrik michalvavrik force-pushed the feature/path-matching-wildcard-latest branch from 1e40a7d to d7296a7 Compare November 21, 2023 23:47
@michalvavrik michalvavrik force-pushed the feature/path-matching-wildcard-latest branch from d7296a7 to bae0f03 Compare November 21, 2023 23:50
@michalvavrik
Copy link
Member Author

Thank you Stuart @stuartwdouglas and Sergey @sberyozkin .

Copy link

quarkus-bot bot commented Nov 22, 2023

Failing Jobs - Building bae0f03

Status Name Step Failures Logs Raw logs Build scan
✔️ JVM Tests - JDK 11 🔍
✔️ JVM Tests - JDK 17 🔍
JVM Tests - JDK 21 Build ⚠️ Check → Logs Raw logs 🔍

You can also consult the Develocity build scans.

@sberyozkin
Copy link
Member

Thanks, JVM21 failure is not related

@sberyozkin sberyozkin merged commit 1323552 into quarkusio:main Nov 22, 2023
53 of 54 checks passed
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Nov 22, 2023
@quarkus-bot quarkus-bot bot added this to the 3.7 - main milestone Nov 22, 2023
@michalvavrik michalvavrik deleted the feature/path-matching-wildcard-latest branch November 22, 2023 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docstyle issues related for manual docstyle review area/documentation area/vertx kind/enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

Support complex wildcard path matching for authorization of web endpoints
3 participants