-
Notifications
You must be signed in to change notification settings - Fork 2.3k
[#953] - Allow CORS preflight requests to bypass authentication #2372
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
Conversation
lprimak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably != null check is not sufficient, as it could be blank
|
Thank you for your contribution! We really appreciate it. Couple of issues: |
|
Also, since you removed the issue template, there is no way to know of your copyright attribution to Apache. Thank you |
lprimak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few minor comments
|
I am not sure why this PR is showing differences from main when those changes are already in main... I have never seen that before, but it's not correct. Are you by any chance cherry-picking latest commits into main into your branch? |
No, I'm not doing that; I'm just following the instructions in the
|
|
@lprimak I used |
lprimak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there...
Need to correct the checkstyle errors.
Also, do you know how to squash the commits?
There should be only one commit in the PR.
[INFO] --- checkstyle:3.6.0:checkstyle (default) @ shiro-web ---
[INFO] Rendering content with org.apache.maven.skins:maven-fluido-skin:jar:2.1.0 skin
[INFO] Starting audit...
[ERROR] web/src/main/java/org/apache/shiro/web/filter/authc/HttpAuthenticationFilter.java:82:21: Variable 'allowPreFlightRequests' explicitly initialized to 'false' (default value for its type). [ExplicitInitialization]
[ERROR] web/src/main/java/org/apache/shiro/web/util/CorsUtils.java:65:53: '&&' should be on a new line. [OperatorWrap]
[ERROR] web/src/main/java/org/apache/shiro/web/util/CorsUtils.java:66:64: '&&' should be on a new line. [OperatorWrap]
Audit done.
web/src/test/groovy/org/apache/shiro/web/util/CorsUtilsTest.groovy
Outdated
Show resolved
Hide resolved
web/src/test/groovy/org/apache/shiro/web/util/CorsUtilsTest.groovy
Outdated
Show resolved
Hide resolved
web/src/test/groovy/org/apache/shiro/web/util/CorsUtilsTest.groovy
Outdated
Show resolved
Hide resolved
Yes I can handle this, I will also fix your checkstyle problems. Note: I merged all commits into the one. |
9dfaef8 to
20ad921
Compare
20ad921 to
852cc7f
Compare
|
Approved. Tests were failing due to CloudFlare outage |
|
I'll take that as a no...sounds good
|
fpapon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as we don't enable it by default for now. May be in next major version as a breaking change.
|
and @celikfatih thank you for your contribution! |
|
See #2376 for 3.x to change the default behavior |
bmarwell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use mockito + assertj even in the one other test case
web/src/test/groovy/org/apache/shiro/web/filter/authc/BearerHttpFilterAuthenticationTest.groovy
Show resolved
Hide resolved
web/src/test/java/org/apache/shiro/web/filter/authc/BasicHttpFilterAuthenticationTest.java
Show resolved
Hide resolved
67f563f to
416c776
Compare
416c776 to
3af59b5
Compare
You are doing great. Thank you! Love the persistence! |
bmarwell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the CorsUtils should be a public final utility class instead, but I do not want to decide that alone if you are okay with it.
However, I want to menteion: I love all the javadoc you added as well as the corrections you did! Thank you so much! :D
| this.authcScheme = authcScheme; | ||
| } | ||
|
|
||
| public void setAllowPreFlightRequests(boolean allowPreFlightRequests) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This (might) be missing isAllowPreflightRequests() getter - but I am not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is missing. Should we add it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please
This PR implements the enhancement proposed in #953, allowing CORS preflight (OPTIONS) requests to bypass authentication across supported authentication filters.
Browsers perform CORS preflight requests before sending actual cross-origin requests, and these preflight requests must not be forced through authentication in order for the CORS handshake to complete successfully.
This change updates the access-control logic to detect preflight requests via
CorsUtils.isPreFlightRequest(...)and immediately allow them whenallowPreflightRequestsis enabled.This behavior applies generically and is not limited to Basic authentication.
Key Changes
Added a preflight request check in
isAccessAllowed(...)within the relevant filter.Ensured that OPTIONS requests with valid CORS headers bypass authentication.
Updated Javadoc explaining the new behavior.
Added unit tests for
CorsUtils.isPreFlightRequest(...).Fixes #953
Following this checklist to help us incorporate your contribution quickly and easily:
for the change (usually before you start working on it). Trivial changes like typos do not
require a GitHub issue. Your pull request should address just this issue, without pulling in other changes.
[#XXX] - Fixes bug in SessionManager,where you replace
#XXXwith the appropriate GitHub issue. Best practiceis to use the GitHub issue title in the pull request title and in the first line of the commit message.
fixes #XXXif merging the PR should close a related issue.mvn verifyto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.git rebase -i.Trivial changes like typos do not require a GitHub issue (javadoc, comments...).
In this case, just format the pull request title like
[DOC] - Add javadoc in SessionManager.If this is your first contribution, you have to read the Contribution Guidelines
If your pull request is about ~20 lines of code you don't need to sign an Individual Contributor License Agreement
if you are unsure please ask on the developers list.
To make clear that you license your contribution under the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.