In-Memory Claim Management #6007

amanda-ariyaratne · 2024-10-09T11:52:28Z

Proposed changes in this pull request

This PR brings the in-memory implementation for default dialects and claims.
The default dialect URIs and claim URIs cannot be changed but other metadata can be updated.
The changes done on the default data will be saved in the database. At runtime, the output will be resolved based on both the data in memory and the database.

Class Diagram

Note: When adding updates to the database, parent entities need to be added as pre-requisites. This is intentionally handled in a non-transactional manner since it will not cause any data inconsistencies or produce stale data.
eg: When adding a claim, its dialect need to be added to the database if it is not already present. This is not handled as a single transaction.

Related Issue

https://github.com/wso2-enterprise/asgardeo-product/issues/26702

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

Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
Is the PR labeled correctly?

Functionality

Are all requirements met? Compare implemented functionality with the requirements specification.
Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.

Code

Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.

Tests

Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
If this is a bug fix, are tests for the issue in place? There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.
If this is a new feature or a significant change to an existing feature? has the manual testing spreadsheet been updated with instructions for manual testing?

Security

Confirm this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.
Are all UI and API inputs run through forms or serializers?
Are all external inputs validated and sanitized appropriately?
Does all branching logic have a default case?
Does this solution handle outliers and edge cases gracefully?
Are all external communications secured and restricted to SSL?

Documentation

Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes should be documented.
Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented.
Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.

codecov · 2024-10-09T12:17:47Z

Codecov Report

Attention: Patch coverage is 89.97773% with 45 lines in your changes missing coverage. Please review.

Project coverage is 40.61%. Comparing base (dcc6780) to head (47fbd24).
Report is 64 commits behind head on master.

Files with missing lines	Patch %	Lines
...etadata/mgt/SystemDefaultClaimMetadataManager.java	84.92%	10 Missing and 9 partials ⚠️
.../claim/metadata/mgt/DefaultClaimMetadataStore.java	0.00%	17 Missing ⚠️
...tadata/mgt/ClaimMetadataManagementServiceImpl.java	93.93%	3 Missing and 1 partial ⚠️
...identity/claim/metadata/mgt/dao/LocalClaimDAO.java	82.60%	3 Missing and 1 partial ⚠️
...laim/metadata/mgt/UnifiedClaimMetadataManager.java	99.34%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #6007      +/-   ##
============================================
+ Coverage     40.23%   40.61%   +0.38%     
- Complexity    14223    14748     +525     
============================================
  Files          1734     1743       +9     
  Lines        117149   122670    +5521     
  Branches      20155    21981    +1826     
============================================
+ Hits          47134    49827    +2693     
- Misses        62761    65317    +2556     
- Partials       7254     7526     +272

Flag	Coverage Δ
unit	`24.39% <89.97%> (+0.45%)`	⬆️

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.

jenkins-is-staging · 2024-10-14T04:23:27Z

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/11320953119

...data.mgt/src/main/java/org/wso2/carbon/identity/claim/metadata/mgt/ClaimMetadataManager.java

jenkins-is-staging · 2024-10-14T06:42:17Z

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/11320953119
Status: failure

jenkins-is-staging · 2024-10-16T05:47:50Z

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/11359610537

jenkins-is-staging · 2024-10-16T08:11:01Z

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/11359610537
Status: failure

...adata.mgt/src/main/java/org/wso2/carbon/identity/claim/metadata/mgt/ClaimMetadataReader.java

chamathns · 2024-10-18T06:19:03Z

...adata.mgt/src/main/java/org/wso2/carbon/identity/claim/metadata/mgt/ClaimMetadataReader.java

+
+import java.util.List;
+
+public interface ClaimMetadataReader {


add class comment

...adata.mgt/src/main/java/org/wso2/carbon/identity/claim/metadata/mgt/ClaimMetadataWriter.java

chamathns · 2024-10-18T06:20:07Z

...adata.mgt/src/main/java/org/wso2/carbon/identity/claim/metadata/mgt/ClaimMetadataWriter.java

+    void addExternalClaim(ExternalClaim externalClaim, int tenantId)
+            throws ClaimMetadataException;


check formatting

chamathns · 2024-10-18T06:24:14Z

...t/src/main/java/org/wso2/carbon/identity/claim/metadata/mgt/DBBasedClaimMetadataManager.java

+        if (claimURI == null || StringUtils.isBlank(claimURI)) {
+            return null;
+        }
+
+        return this.externalClaimDAO.getExternalClaims(externalClaimDialectURI, tenantId).stream()
+                .filter(externalClaim -> claimURI.equals(externalClaim.getClaimURI()))
+                .findFirst().orElse(null);
+    }


check the previous comment

...main/java/org/wso2/carbon/identity/claim/metadata/mgt/SystemDefaultClaimMetadataManager.java

chamathns · 2024-10-18T06:26:22Z

...main/java/org/wso2/carbon/identity/claim/metadata/mgt/SystemDefaultClaimMetadataManager.java

+        claims = new HashMap<>();
+
+        ClaimConfig claimConfig = IdentityClaimManagementServiceDataHolder.getInstance().getClaimConfig();
+        if (claimConfig != null && claimConfig.getClaimMap() != null) {


can use MapUtils

chamathns · 2024-10-18T06:28:52Z

...main/java/org/wso2/carbon/identity/claim/metadata/mgt/SystemDefaultClaimMetadataManager.java

+            for (Map.Entry<ClaimKey, ClaimMapping> entry : claimConfig.getClaimMap().entrySet()) {
+                ClaimKey claimKey = entry.getKey();
+                ClaimMapping claimMapping = entry.getValue();
+                String claimDialectURI = claimKey.getDialectUri();
+                String claimURI = claimKey.getClaimUri();
+                Claim claim;
+
+                boolean dialectExists = claimDialects.stream()
+                        .anyMatch(claimDialect -> claimDialect.getClaimDialectURI().equals(claimDialectURI));
+                if (!dialectExists) {
+                    claimDialects.add(new ClaimDialect(claimDialectURI));
+                }
+
+                if (claimDialectURI.equals(LOCAL_CLAIM_DIALECT_URI)) {
+                    List<AttributeMapping> mappedAttributes = new ArrayList<>();
+                    if (StringUtils.isNotBlank(claimMapping.getMappedAttribute())) {
+                        mappedAttributes
+                                .add(new AttributeMapping(primaryDomainName, claimMapping.getMappedAttribute()));
+                    }
+
+                    if (claimMapping.getMappedAttributes() != null) {
+                        for (Map.Entry<String, String> claimMappingEntry : claimMapping.getMappedAttributes()
+                                .entrySet()) {
+                            mappedAttributes.add(new AttributeMapping(claimMappingEntry.getKey(),
+                                    claimMappingEntry.getValue()));
+                        }
+                    }
+
+                    Map<String, String> claimProperties = filterClaimProperties(claimConfig.getPropertyHolderMap()
+                            .get(claimKey));
+                    claim = new LocalClaim(claimURI, mappedAttributes, claimProperties);
+                } else {
+                    String mappedLocalClaimURI = claimConfig.getPropertyHolderMap().get(claimKey).get(ClaimConstants
+                            .MAPPED_LOCAL_CLAIM_PROPERTY);
+                    Map<String, String> claimProperties = filterClaimProperties(claimConfig.getPropertyHolderMap()
+                            .get(claimKey));
+                    claim = new ExternalClaim(claimDialectURI, claimURI, mappedLocalClaimURI, claimProperties);
+                }
+                claims.computeIfAbsent(claimDialectURI, k -> new ArrayList<>());
+                claims.get(claimDialectURI).add(claim);
+            }
+        }
+    }


let's refactor this. can simplify the nesting / extract out the finding mapped attributes part to a helper method to improve readability.

chamathns · 2024-10-18T06:29:12Z

...main/java/org/wso2/carbon/identity/claim/metadata/mgt/SystemDefaultClaimMetadataManager.java

+        if (claimDialectURI == null || StringUtils.isBlank(claimDialectURI)) {
+            return null;
+        }
+
+        return claimDialects.stream()
+                .filter(claimDialect -> claimDialectURI.equals(claimDialect.getClaimDialectURI()))
+                .findFirst()
+                .orElse(null);
+    }


refer to previous comments about returning null

sonarcloud · 2024-10-18T06:40:34Z

Quality Gate passed

Issues
64 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.8% Duplication on New Code

See analysis details on SonarCloud

chamathns · 2024-10-18T09:46:28Z

...t/src/main/java/org/wso2/carbon/identity/claim/metadata/mgt/UnifiedClaimMetadataManager.java

+    public void updateLocalClaimMappings(List<LocalClaim> localClaimList, int tenantId, String userStoreDomain)
+            throws ClaimMetadataException {
+
+        if (!localClaimList.isEmpty() && !isClaimDialectInDB(ClaimConstants.LOCAL_CLAIM_DIALECT_URI, tenantId)) {


prone to NPEs. will need to check if localClaimList is not null, before calling isEmpty() on it.

chamathns · 2024-10-18T09:58:29Z

...t/src/main/java/org/wso2/carbon/identity/claim/metadata/mgt/UnifiedClaimMetadataManager.java

+
+        List<ClaimDialect> claimDialectsInDB = this.dbBasedClaimMetadataManager.getClaimDialects(tenantId);
+        List<ClaimDialect> claimDialectsInSystem = this.systemDefaultClaimMetadataManager.getClaimDialects(tenantId);
+        Set<String> claimDialectURIsInDB = claimDialectsInDB.stream()


are claimDialectsInDB and claimDialectsInSystem null safe? if not get them as Optional.OfNullable before streaming

chamathns · 2024-10-18T12:10:34Z

...main/java/org/wso2/carbon/identity/claim/metadata/mgt/SystemDefaultClaimMetadataManager.java

+    @Override
+    public boolean isMappedLocalClaim(String localClaimURI, int tenantId) throws ClaimMetadataException {
+
+        if (localClaimURI == null || StringUtils.isBlank(localClaimURI)) {


explicit null check is redundant

chamathns · 2024-10-18T12:33:50Z

...main/java/org/wso2/carbon/identity/claim/metadata/mgt/SystemDefaultClaimMetadataManager.java

+                continue;
+            }
+            List<Claim> externalClaims = entry.getValue().stream()
+                    .map(ExternalClaim.class::cast)


to avoid potential class cast exceptions, we can use a instanceof filter before casting

.filter(claim -> claim instanceof ExternalClaim) .map(claim -> (ExternalClaim) claim)

chamathns · 2024-10-18T12:35:00Z

...main/java/org/wso2/carbon/identity/claim/metadata/mgt/SystemDefaultClaimMetadataManager.java

+                    .map(ExternalClaim.class::cast)
+                    .filter(claim -> localClaimURI.equals(claim.getMappedLocalClaim()))
+                    .collect(Collectors.toList());
+            if (!externalClaims.isEmpty()) {


instead of collecting and checking if it's empty, can't we use anyMatch() and terminate early?

chamathns · 2024-10-18T12:36:14Z

...main/java/org/wso2/carbon/identity/claim/metadata/mgt/SystemDefaultClaimMetadataManager.java

+    @Override
+    public boolean isLocalClaimMappedWithinDialect(String mappedLocalClaim, String externalClaimDialectURI, int tenantId) throws ClaimMetadataException {
+
+        if (externalClaimDialectURI == null || StringUtils.isBlank(externalClaimDialectURI)


explicit null check is redundant. StringUtils.isBlank does this

chamathns · 2024-10-18T12:38:15Z

...main/java/org/wso2/carbon/identity/claim/metadata/mgt/SystemDefaultClaimMetadataManager.java

+                || !claims.containsKey(externalClaimDialectURI)) {
+            return false;
+        }
+        if (mappedLocalClaim == null || StringUtils.isBlank(mappedLocalClaim)) {


refer to previous comment

chamathns · 2024-10-18T12:39:58Z

...main/java/org/wso2/carbon/identity/claim/metadata/mgt/SystemDefaultClaimMetadataManager.java

+            claimProperties.put(ClaimConstants.DISPLAY_NAME_PROPERTY, "0");
+        }
+
+        if (claimProperties.containsKey(ClaimConstants.SUPPORTED_BY_DEFAULT_PROPERTY)) {


can use computeIfAbsent

chamathns · 2024-10-18T12:40:11Z

...main/java/org/wso2/carbon/identity/claim/metadata/mgt/SystemDefaultClaimMetadataManager.java

+            }
+        }
+
+        if (claimProperties.containsKey(ClaimConstants.READ_ONLY_PROPERTY)) {


refer to previous comment

amanda-ariyaratne marked this pull request as ready for review October 9, 2024 11:53

amanda-ariyaratne marked this pull request as draft October 9, 2024 11:55

add in-memory claim management

90268d2

amanda-ariyaratne force-pushed the issue-26702 branch from 78bc0c4 to 90268d2 Compare October 13, 2024 08:39

amanda-ariyaratne marked this pull request as ready for review October 14, 2024 04:21

ashensw reviewed Oct 14, 2024

View reviewed changes

...data.mgt/src/main/java/org/wso2/carbon/identity/claim/metadata/mgt/ClaimMetadataManager.java Outdated Show resolved Hide resolved

add copyright license to new class files

7c1f3db

add in-memory implementation for DefaultClaimMetadataStore class

9c7bfd4

add new validation for update claim dialect

7ebaaf7

amanda-ariyaratne added 5 commits October 16, 2024 14:50

remove unused imports

48955e6

fix bug in get external claim method

266783a

improve unit tests of SystemDefaultClaimMetadataManager

713c5f8

add more unit tests for ClaimMetadataManagementServiceImpl

95312a1

separate claim metadata manager interface to reader and writer

e7495a7

chamathns reviewed Oct 18, 2024

View reviewed changes

add missing copyright license and javadoc comments

47fbd24

chamathns reviewed Oct 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In-Memory Claim Management #6007

In-Memory Claim Management #6007

amanda-ariyaratne commented Oct 9, 2024 •

edited

Loading

codecov bot commented Oct 9, 2024 •

edited

Loading

jenkins-is-staging commented Oct 14, 2024

jenkins-is-staging commented Oct 14, 2024

jenkins-is-staging commented Oct 16, 2024

jenkins-is-staging commented Oct 16, 2024

chamathns Oct 18, 2024

chamathns Oct 18, 2024

chamathns Oct 18, 2024

chamathns Oct 18, 2024

chamathns Oct 18, 2024

chamathns Oct 18, 2024

sonarcloud bot commented Oct 18, 2024

chamathns Oct 18, 2024

chamathns Oct 18, 2024

chamathns Oct 18, 2024

chamathns Oct 18, 2024

chamathns Oct 18, 2024

chamathns Oct 18, 2024

chamathns Oct 18, 2024

chamathns Oct 18, 2024

chamathns Oct 18, 2024


		import java.util.List;

		public interface ClaimMetadataReader {

		void addExternalClaim(ExternalClaim externalClaim, int tenantId)
		throws ClaimMetadataException;

In-Memory Claim Management #6007

Are you sure you want to change the base?

In-Memory Claim Management #6007

Conversation

amanda-ariyaratne commented Oct 9, 2024 • edited Loading

Proposed changes in this pull request

Related Issue

When should this PR be merged

Follow up actions

Checklist (for reviewing)

General

Functionality

Code

Tests

Security

Documentation

codecov bot commented Oct 9, 2024 • edited Loading

Codecov Report

jenkins-is-staging commented Oct 14, 2024

jenkins-is-staging commented Oct 14, 2024

jenkins-is-staging commented Oct 16, 2024

jenkins-is-staging commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Oct 18, 2024

Quality Gate passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanda-ariyaratne commented Oct 9, 2024 •

edited

Loading

codecov bot commented Oct 9, 2024 •

edited

Loading