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.

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 84.91736% with 73 lines in your changes missing coverage. Please review.

Project coverage is 40.94%. Comparing base (a1adff7) to head (3f9afca).
Report is 52 commits behind head on master.

Files with missing lines	Patch %	Lines
...laim/metadata/mgt/UnifiedClaimMetadataManager.java	88.64%	12 Missing and 9 partials ⚠️
.../claim/metadata/mgt/DefaultClaimMetadataStore.java	0.00%	17 Missing ⚠️
...tadata/mgt/ClaimMetadataManagementServiceImpl.java	74.60%	12 Missing and 4 partials ⚠️
...ty/claim/metadata/mgt/util/ClaimMetadataUtils.java	75.00%	6 Missing and 4 partials ⚠️
...identity/claim/metadata/mgt/dao/LocalClaimDAO.java	80.95%	3 Missing and 1 partial ⚠️
...etadata/mgt/SystemDefaultClaimMetadataManager.java	97.33%	1 Missing and 1 partial ⚠️
...rbon/identity/claim/metadata/mgt/dao/ClaimDAO.java	88.23%	2 Missing ⚠️
...laim/metadata/mgt/DBBasedClaimMetadataManager.java	98.30%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #6007      +/-   ##
============================================
+ Coverage     40.73%   40.94%   +0.21%     
- Complexity    14459    14607     +148     
============================================
  Files          1764     1767       +3     
  Lines        117634   118050     +416     
  Branches      19106    19169      +63     
============================================
+ Hits          47917    48339     +422     
+ Misses        62443    62426      -17     
- Partials       7274     7285      +11

Flag	Coverage Δ
unit	`25.39% <84.91%> (+0.40%)`	⬆️

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

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

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

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

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

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

components/claim-mgt/org.wso2.carbon.identity.claim.metadata.mgt/pom.xml

...ain/java/org/wso2/carbon/identity/claim/metadata/mgt/ClaimMetadataManagementServiceImpl.java

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

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

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

...etadata.mgt/src/main/java/org/wso2/carbon/identity/claim/metadata/mgt/dao/LocalClaimDAO.java

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

...c/test/java/org/wso2/carbon/identity/claim/metadata/mgt/UnifiedClaimMetadataManagerTest.java

...a.mgt/src/main/java/org/wso2/carbon/identity/claim/metadata/mgt/util/ClaimMetadataUtils.java

nilasini · 2024-11-01T09:11:50Z

Can you please add sample API requests and responses before and after your fix?

nilasini · 2024-11-01T09:49:35Z

Did we consideer store procedure based DAO mentioned in [1]. Seems we need to deprecate it as we are having in memory concept, this[1] may not be needed anymore. Please verify and do the needful
[1] https://github.com/wso2/carbon-identity-framework/pull/3717/files
[2] wso2/product-is#12535

amanda-ariyaratne · 2024-11-01T10:49:39Z

Can you please add sample API requests and responses before and after your fix?

These are the changes to API responses with the changes in this PR and the supporting PRs.

Rename a default dialect

Before : Allowed. HTTP Status 200 OK.

After: 403 Forbidden

{
    "code": "CMT-60007",
    "message": "Cannot rename dialect <dialect_URI> as it is a system dialect",
    "description": "Cannot rename dialect <dialect_URI> as it is a system dialect",
    "traceId": ""
}

Rename a custom dialect to an existing dialect URI.

Before: 500 Internal Server Error - CMT-50013

{
    "code": "CMT-50013",
    "message": "Unable to update claim dialect.",
    "description": "Unable to update claim dialect.",
    "traceId": ""
}

After: 409 Conflict CMT-60002 Claim dialect: %s already exists

{
    "code": "CMT-60002",
    "message": "Claim dialect: dialect_URI already exists",
    "description": "Claim dialect: dialect_URI already exists",
    "traceId": ""
}

Remove a default dialect.

Before: 204 No Content

After: 403 Forbidden

{
    "code": "CMT-60008",
    "message": "Cannot delete dialect <dailect_URI> as it is a system dialect",
    "description": "Cannot delete dialect <dailect_URI> as it is a system dialect",
    "traceId": ""
}

Remove a default claim

Before: 204 No Content

After: 403 Forbidden

{
    "code": "CMT-60006",
    "message": "Cannot delete claim <claim_URI> as it is a system claim",
    "description": "Cannot delete claim <claim_URI> as it is a system claim",
    "traceId": ""
}

Get External Claim

Before: Did not have a property list.

{
        "id": "dXJuOmlldGY6cGFyYW1zOnNjaW06c2NoZW1hczpjb3JlOjIuMDpVc2VyOmVtYWlscw",
        "claimURI": "urn:ietf:params:scim:schemas:core:2.0:User:emails",
        "claimDialectURI": "urn:ietf:params:scim:schemas:core:2.0:User",
        "mappedLocalClaimURI": "http://wso2.org/claims/emailaddress",
 }

After: Has property list.

{
        "id": "dXJuOmlldGY6cGFyYW1zOnNjaW06c2NoZW1hczpjb3JlOjIuMDpVc2VyOmVtYWlscw",
        "claimURI": "urn:ietf:params:scim:schemas:core:2.0:User:emails",
        "claimDialectURI": "urn:ietf:params:scim:schemas:core:2.0:User",
        "mappedLocalClaimURI": "http://wso2.org/claims/emailaddress",
        "properties": [
            {
                "key": "Description",
                "value": "Email Addresses"
            },
            {
                "key": "DisplayOrder",
                "value": "5"
            },
            {
                "key": "RegEx",
                "value": "^([a-zA-Z0-9_\\.\\-])+\\@(([a-zA-Z0-9\\-])+\\.)+([a-zA-Z0-9]{2,4})+$"
            },
            {
                "key": "SupportedByDefault",
                "value": "true"
            },
            {
                "key": "MappedLocalClaim",
                "value": "http://wso2.org/claims/emailaddress"
            },
            {
                "key": "isSystemClaim",
                "value": "true"
            },
            {
                "key": "DisplayName",
                "value": "Emails"
            }
        ]
}

Update an external claim to an already mapped local claim within the same dialect.

Before: 200 OK (Bug)

After: 400 Bad Request

{
    "code": "CMT-60004",
    "message": "Local claim URI : <claim_URI> is already mapped in claim dialect: <dialect_URI>”,
    "description": "Local claim URI : <claim_URI> is already mapped in claim dialect: <dialect_URI>",
    "traceId": ""
}

Remove a default external claim

Before: 204 No Content

After: 403 Forbidden

{
    "code": "CMT-60006",
    "message": "Cannot delete claim <claim_URI> as it is a system claim",
    "description": "Cannot delete claim <claim_URI> as it is a system claim",
    "traceId": ""
}

amanda-ariyaratne · 2024-11-01T10:52:59Z

Did we consideer store procedure based DAO mentioned in [1]. Seems we need to deprecate it as we are having in memory concept, this[1] may not be needed anymore. Please verify and do the needful [1] https://github.com/wso2/carbon-identity-framework/pull/3717/files [2] wso2/product-is#12535

Thanks for pointing this out. Since scope to claim mapping stored procedure is only run if the init claim config stored procedure is run earlier, there shouldn't be an issue. New claims will continue to be supported through the in-memory implementation.

Let's deprecate the stored procedures since they are not needed with this improvement.

jenkins-is-staging · 2024-11-01T13:33:08Z

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

jenkins-is-staging · 2024-11-01T16:03:34Z

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

nilasini · 2024-11-04T04:34:31Z

Did we consideer store procedure based DAO mentioned in [1]. Seems we need to deprecate it as we are having in memory concept, this[1] may not be needed anymore. Please verify and do the needful [1] https://github.com/wso2/carbon-identity-framework/pull/3717/files [2] wso2/product-is#12535

Thanks for pointing this out. Since scope to claim mapping stored procedure is only run if the init claim config stored procedure is run earlier, there shouldn't be an issue. New claims will continue to be supported through the in-memory implementation.

Let's deprecate the stored procedures since they are not needed with this improvement.

Can you please create a git issue with the detail to deprecate this.

nilasini · 2024-11-04T04:36:37Z

Can you please add sample API requests and responses before and after your fix?

These are the changes to API responses with the changes in this PR and the supporting PRs.

Rename a default dialect

Before : Allowed. HTTP Status 200 OK.

After: 400 Bad Request

{
    "code": "CMT-60007",
    "message": "Cannot rename dialect <dialect_URI> as it is a system dialect",
    "description": "Cannot rename dialect <dialect_URI> as it is a system dialect",
    "traceId": ""
}

Rename a custom dialect to an existing dialect URI.

Before: 500 Internal Server Error - CMT-50013

{
    "code": "CMT-50013",
    "message": "Unable to update claim dialect.",
    "description": "Unable to update claim dialect.",
    "traceId": ""
}

After: 409 Conflict CMT-60002 Claim dialect: %s already exists

{
    "code": "CMT-60002",
    "message": "Claim dialect: dialect_URI already exists",
    "description": "Claim dialect: dialect_URI already exists",
    "traceId": ""
}

Remove a default dialect.

Before: 204 No Content

After: 400 Bad Request

{
    "code": "CMT-60008",
    "message": "Cannot delete dialect <dailect_URI> as it is a system dialect",
    "description": "Cannot delete dialect <dailect_URI> as it is a system dialect",
    "traceId": ""
}

Remove a default claim

Before: 204 No Content

After: 400 Bad Request

{
    "code": "CMT-60006",
    "message": "Cannot delete claim <claim_URI> as it is a system claim",
    "description": "Cannot delete claim <claim_URI> as it is a system claim",
    "traceId": ""
}

Get External Claim

Before: Did not have a property list.

{
        "id": "dXJuOmlldGY6cGFyYW1zOnNjaW06c2NoZW1hczpjb3JlOjIuMDpVc2VyOmVtYWlscw",
        "claimURI": "urn:ietf:params:scim:schemas:core:2.0:User:emails",
        "claimDialectURI": "urn:ietf:params:scim:schemas:core:2.0:User",
        "mappedLocalClaimURI": "http://wso2.org/claims/emailaddress",
 }

After: Has property list.

{
        "id": "dXJuOmlldGY6cGFyYW1zOnNjaW06c2NoZW1hczpjb3JlOjIuMDpVc2VyOmVtYWlscw",
        "claimURI": "urn:ietf:params:scim:schemas:core:2.0:User:emails",
        "claimDialectURI": "urn:ietf:params:scim:schemas:core:2.0:User",
        "mappedLocalClaimURI": "http://wso2.org/claims/emailaddress",
        "properties": [
            {
                "key": "Description",
                "value": "Email Addresses"
            },
            {
                "key": "DisplayOrder",
                "value": "5"
            },
            {
                "key": "RegEx",
                "value": "^([a-zA-Z0-9_\\.\\-])+\\@(([a-zA-Z0-9\\-])+\\.)+([a-zA-Z0-9]{2,4})+$"
            },
            {
                "key": "SupportedByDefault",
                "value": "true"
            },
            {
                "key": "MappedLocalClaim",
                "value": "http://wso2.org/claims/emailaddress"
            },
            {
                "key": "isSystemClaim",
                "value": "true"
            },
            {
                "key": "DisplayName",
                "value": "Emails"
            }
        ]
}

Add claim mapped to a non existing local claim

Before: 400 Bad Request

{
    "code": "CMT-50036",
    "message": "Invalid mapped local claim URI",
    "description": "Invalid mapped local claim URI",
    "traceId": ""
}

After: 400 Bad Request

{
    "code": "CMT-600011",
    "message": "Local claim URI: <claim_URI>  does not exist.",
    "description": "Local claim URI: <claim_URI>  does not exist.",
    "traceId": ""
}

Any reason to change this error code and error message. Can't we keep the existing one?

Update an external claim to an already mapped local claim within the same dialect.

Before: 200 OK (Bug)

After: 400 Bad Request
{
    "code": "CMT-60004",
    "message": "Local claim URI : <claim_URI> is already mapped in claim dialect: <dialect_URI>”,
    "description": "Local claim URI : <claim_URI> is already mapped in claim dialect: <dialect_URI>",
    "traceId": ""
}
Remove a default external claim

Before: 204 No Content

After: 400 Bad Request
{
    "code": "CMT-60006",
    "message": "Cannot delete claim <claim_URI> as it is a system claim",
    "description": "Cannot delete claim <claim_URI> as it is a system claim",
    "traceId": ""
}

Please update the documentation with the proper responses

amanda-ariyaratne · 2024-11-05T12:33:10Z

Any reason to change this error code and error message. Can't we keep the existing one?

@nilasini No we can keep the existing one. I restored it in the code and updated the original comment.

amanda-ariyaratne · 2024-11-06T10:04:42Z

Can you please create a git issue with the detail to deprecate this.

Created wso2/product-is#21626

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

sonarcloud · 2024-11-08T06:14:50Z

Quality Gate passed

Issues
59 New issues
0 Accepted issues

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

See analysis details on SonarCloud

jenkins-is-staging · 2024-11-11T04:52:05Z

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

jenkins-is-staging · 2024-11-11T07:10:31Z

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

amanda-ariyaratne · 2024-11-12T05:01:09Z

The integration test failures will be fixed with the below 2 PRs

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

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

chamathns reviewed Oct 18, 2024

View reviewed changes