simpleidserver / SimpleIdServer

OpenID, OAuth 2.0, SCIM2.0, UMA2.0, FAPI, CIBA & OPENBANKING Framework for ASP.NET Core
https://simpleidserver.com/
Apache License 2.0
686 stars 90 forks source link

Removing children attributes #665

Closed alexander-durovich-swi closed 3 months ago

alexander-durovich-swi commented 6 months ago

Hello,

We see the following problem for the PATCH request with remove operations:

  1. If parrent attribute has 2 children and we send the PATCH request with remove operation for only one child attribute, then all is ok, response contains the only one left attribute. Example: Parent "name" attribute has 2 children: "name.familyName" and "name.givenName". We send request with:
        {
            "op": "remove",
            "path": "name.familyName"
        }

Response contains only one left attribute (all is ok here):

    "name": {
        "givenName": "GName123"
    },
  1. We send request with 2 remove operations for bouth children:
        {
            "op": "remove",
            "path": "name.givenName"
        },
        {
            "op": "remove",
            "path": "name.familyName"
        }

Response in this case contains both children with old values:

    "name": {
        "givenName": "GName123",
        "familyName": "FName456"
    },

Perhaps problem is in the SCIMRepresentation.BuildHierarchicalAttributes method. We see that when all children attributes removed, then Children and CachedChildren properties are not recalculated at parent node. This Children and CachedChildren properties are recalculated at parent only if at least one child atrribute exist.

Are you agree to fix this problem, please?

Best Regard, Alex

simpleidserver commented 6 months ago

Hello,

I attempted to reproduce the issue on my local machine without success. By default, the attributes of the representation are not returned by HTTP PATCH and HTTP PUT operations. We avoid overloading the memory by loading only the attributes targeted by the HTTP operations into memory.

Have you implemented a custom class and implemented the ISCIMRepresentationCommandRepository ? Hello,If so, have you updated the Get operation as follows? You must explicitly load the FlatAttributes property.

        public async Task<SCIMRepresentation> Get(string id, CancellationToken token = default)
        {
            var query = _scimDbContext.SCIMRepresentationLst
                .Include(r => r.Schemas).ThenInclude(s => s.Attributes)
                .Include(r => r.FlatAttributes);
            return await query.FirstOrDefaultAsync(r => r.Id == id, token);
        }

I tested these changes on my local machine, and the attributes are correctly updated.

alexander-durovich-swi commented 6 months ago

Hello,

Thank you for yuor answer. Yes, we have our own implementation of ISCIMRepresentationCommandRepository. Our implementation doesn't work direct with DB, instead it calls our another internal service for getting/updating representation with FlatAttributes. The Get method gets our internal user model from our internal service and converts into representation with filled FlatAttributes. Also it caches the got representation object in memory. The Update method converts representation object with FlatAttributes into internal user model and sends into our internal service. But at first the Update method merges new attribute changes collected via BulkXX methods with current cached representation.FlatAttributes. For this we restore SCIMRepresentationPatchResult object with data got from BulkXX methods and then reuse your SCIMRepresentationExtensions.Apply method. We see that SCIMRepresentationExtensions.Apply method is called then also by your code in PatchRepresentationCommandHandler.UpdateRepresentation method after calling _scimRepresentationCommandRepository.Update, and as we see, this method is idempotent (can be called several times with the same result), so our calling of Apply method should not effect anyhow the result behaviour. As we see, the Children and CachedChildren properties are recalculated in the SCIMRepresentation.BuildHierarchicalAttributes method that is called for example form the BaseApiController.InternalPatch method via call to SCIMRepresentationExtensions.ToResponse method and then calling representation.HierarchicalAttributes getter. And we see the following results after calling BuildHierarchicalAttributes method:

  1. Remove 2 attributes. Before calling Apply method we have: image After calling Apply and BuildHierarchicalAttributes we have: image I.e. Children properties are not recalcualted.

  2. Remove one attribute: Before calling Apply method we have: image After calling Apply and BuildHierarchicalAttributes we have: image I.e. Children properties are reclaculated.

Best Regards, Alex

simpleidserver commented 6 months ago

There is indeed an issue in the BuildHierarchicalAttributes algorithm; the Children and CachedChildren properties were not cleared. The problem has been resolved in the master branch. You can view the changes here: https://github.com/simpleidserver/SimpleIdServer/commit/cd6482bb798468bea26dee262e034c7c91424217

alexander-durovich-swi commented 6 months ago

Ok, thank you very much!

Best Regards, Alex

alexander-durovich-swi commented 5 months ago

Hello,

Could you say when are you planning the release of v.4.0.7, please?

Best Regards, Alex

simpleidserver commented 5 months ago

Hello,

The Nuget package 4.0.7-rc1 https://www.nuget.org/packages/SimpleIdServer.Scim is available :)

KR,

SID

alexander-durovich-swi commented 5 months ago

Ok, thank you very much!

Best Regards, Alex