-
Notifications
You must be signed in to change notification settings - Fork 145
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
Disable authentication for DCR create via tenant-wise config #269
Disable authentication for DCR create via tenant-wise config #269
Conversation
# Conflicts: # pom.xml
...entity.auth.valve/src/main/java/org/wso2/carbon/identity/auth/valve/AuthenticationValve.java
Outdated
Show resolved
Hide resolved
...entity.auth.valve/src/main/java/org/wso2/carbon/identity/auth/valve/AuthenticationValve.java
Show resolved
Hide resolved
...alve/src/main/java/org/wso2/carbon/identity/auth/valve/factory/DCRMgtOGSiServiceFactory.java
Show resolved
Hide resolved
...alve/src/main/java/org/wso2/carbon/identity/auth/valve/factory/DCRMgtOGSiServiceFactory.java
Outdated
Show resolved
Hide resolved
...alve/src/main/java/org/wso2/carbon/identity/auth/valve/factory/DCRMgtOGSiServiceFactory.java
Outdated
Show resolved
Hide resolved
...entity.auth.valve/src/main/java/org/wso2/carbon/identity/auth/valve/AuthenticationValve.java
Show resolved
Hide resolved
...entity.auth.valve/src/main/java/org/wso2/carbon/identity/auth/valve/AuthenticationValve.java
Outdated
Show resolved
Hide resolved
|
||
DCRConfiguration dcrConfiguration = DCRMgtOGSiServiceFactory.getInstance().getDCRConfiguration(); | ||
Boolean isClientAuthenticationRequired = dcrConfiguration.isAuthenticationRequired(); | ||
if ((Boolean.TRUE).equals(isClientAuthenticationRequired)) { |
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.
can simplify the if , else
securedResource.setIsSecured(Boolean.TRUE.equals(isClientAuthenticationRequired));
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 was not done because then if isClientAuthenticationRequired
is null
(isClientAuthenticationRequired
is not set), Boolean.TRUE.equals(isClientAuthenticationRequired)
value will be False, which is incorrect.
If isClientAuthenticationRequired
is not set, it should be the current default behaviour, where securedResource.isSecured will be True.
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.
In that case, we can specially handle using the clause if(isClientAuthenticationRequired ==null)
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 we should not handle the scenario where isClientAuthenticationRequired
is not set, because it can change the current flow.
This is a tenant-level config and this overrides the server level configs. If tenant-level config is not set, the server level config should be there.
IS has a config in deployment.toml to configure api security for any endpoint using the endpoint url pattern. For DCR if isClientAuthenticationRequired
is not set, that config value (if configured) will change the securedResource.isSecured
value.
...alve/src/main/java/org/wso2/carbon/identity/auth/valve/factory/DCRMgtOGSiServiceFactory.java
Outdated
Show resolved
Hide resolved
...alve/src/main/java/org/wso2/carbon/identity/auth/valve/factory/DCRMgtOGSiServiceFactory.java
Outdated
Show resolved
Hide resolved
PR builder started |
PR builder completed |
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.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/9026820218
Proposed Changes
With this PR, authentication can be disabled via the authenticationRequired tenant-wise configuration for DCR create endpoint.
Git Issue: wso2/product-is#19274
When should this PR be merged
After merging wso2-extensions/identity-inbound-auth-oauth#2344
Follow up actions
[List any possible follow-up actions here; for instance, testing data
migrations, software that we need to install on staging and production
environments.]
Checklist (for reviewing)
General
Functionality
Code
Tests
Security
Documentation