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 JDBC based persistence for SAML #6287

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Osara-B
Copy link
Contributor

@Osara-B Osara-B commented Jan 16, 2025

Introduce JDBC based persistence for SAML. Related to,

@Osara-B Osara-B force-pushed the refactor-saml2 branch 2 times, most recently from 93f9d76 to f25b8dc Compare January 16, 2025 06:46
"An error occurred while retrieving the " + "the service provider with the issuer '%s'", issuer),
e);
}
if (serviceProviderDO == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

lets convert this to,
if (serviceProviderDO != null) {

Copy link

codecov bot commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 81.55080% with 69 lines in your changes missing coverage. Please review.

Project coverage is 46.03%. Comparing base (a08e390) to head (480caf6).
Report is 61 commits behind head on master.

Files with missing lines Patch % Lines
.../identity/core/model/SAMLSSOServiceProviderDO.java 61.53% 3 Missing and 32 partials ⚠️
...ty/core/dao/JDBCSAMLSSOServiceProviderDAOImpl.java 87.54% 26 Missing and 7 partials ⚠️
...ore/dao/RegistrySAMLSSOServiceProviderDAOImpl.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #6287      +/-   ##
============================================
+ Coverage     45.89%   46.03%   +0.13%     
- Complexity    14420    14492      +72     
============================================
  Files          1654     1657       +3     
  Lines        103215   103587     +372     
  Branches      18203    18251      +48     
============================================
+ Hits          47375    47688     +313     
- Misses        49062    49085      +23     
- Partials       6778     6814      +36     
Flag Coverage Δ
unit 29.20% <81.55%> (+0.25%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +170 to +180
try {
String tenantDomain = IdentityTenantUtil.getTenantDomain(tenantId);
// Load the certificate stored in the database, if signature validation is enabled.
if (serviceProviderDO.isDoValidateSignatureInRequests() ||
serviceProviderDO.isDoValidateSignatureInArtifactResolve() ||
serviceProviderDO.isDoEnableEncryptedAssertion()) {

Tenant tenant = IdentityTenantUtil.getTenant(tenantId);
serviceProviderDO.setX509Certificate(getApplicationCertificate(serviceProviderDO, tenant));
}
serviceProviderDO.setTenantDomain(tenantDomain);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we move this logic to the manager?

public boolean isServiceProviderExists(String issuer, int tenantId) throws IdentityException {

try {
return processIsServiceProviderExists(issuer, tenantId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets move processIsServiceProviderExists impl to here.


List<SAMLSSOServiceProviderDO> serviceProvidersList;
try {
serviceProvidersList = processGetServiceProviders(tenantId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets move processGetServiceProviders impl to here.

public boolean removeServiceProvider(String issuer, int tenantId) throws IdentityException {

try {
processDeleteServiceProvider(issuer, tenantId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets move processDeleteServiceProvider impl here

SAMLSSOServiceProviderDO serviceProviderDO = null;

try {
serviceProviderDO = processGetServiceProvider(issuer, tenantId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets move processGetServiceProvider impl here.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
3.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

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

Successfully merging this pull request may close these issues.

2 participants