-
Notifications
You must be signed in to change notification settings - Fork 546
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
Move SAML SP validation to SAMLSSOServiceProviderManager layer #6263
Conversation
Quality Gate passedIssues Measures |
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.
Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.
Files not reviewed (1)
- components/identity-core/org.wso2.carbon.identity.core/src/test/resources/testng.xml: Language not supported
The following imports are unused and can be removed: In
In
|
} | ||
|
||
if (StringUtils.isNotBlank(serviceProviderDO.getIssuerQualifier()) && | ||
!serviceProviderDO.getIssuer().contains(IdentityRegistryResources.QUALIFIER_ID)) { |
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.
&& !serviceProviderDO.getIssuer().contains(IdentityRegistryResources.QUALIFIER_ID)
This part is not available in the addServiceProvider()
validation in the RegistrySAMLSSOServiceProviderDAOImpl
.. Is that expected?
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.
Same concern is there for uploadServiceProvider()
as well.
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.
We added the check to ensure that the qualifier wont be appended again to the issuer value and set in the DO
private void validateServiceProvider(SAMLSSOServiceProviderDO serviceProviderDO) throws IdentityException { | ||
|
||
if (serviceProviderDO == null || serviceProviderDO.getIssuer() == null || | ||
StringUtils.isBlank(serviceProviderDO.getIssuer())) { |
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.
StringUtils.isBlank(serviceProviderDO.getIssuer()
part is not available in the uploadServiceProvider()
validation in the RegistrySAMLSSOServiceProviderDAOImpl.
Is that expected?
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.
Added the check to ensure the issuer isnt white space
Proposed changes in this pull request
[List all changes you want to add here. If you fixed an issue, please
add a reference to that issue as well.]
When should this PR be merged
[Please describe any preconditions that need to be addressed before we
can merge this pull request.]
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