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

[SCIM] Patch Add operation for multiple fields to addresses causes each field to be added separately. #808

Open
JesseDeH opened this issue Nov 8, 2024 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@JesseDeH
Copy link

JesseDeH commented Nov 8, 2024

When performing a Patch with multiple values for a single address object, each value is stored as a separate entry in the addresses list.

See for an example the below Patch operation with the result.
Unfortunately, the example is the way that the SCIM implementation within Entra ID is sending the patches in this case, so I am not able to change the way that the operations are sent to the endpoint.

PatchOp

{
    "schemas": [
        "urn:ietf:params:scim:api:messages:2.0:PatchOp"
    ],
    "Operations": [
        {
            "op": "Add",
            "path": "addresses[type eq \"work\"].streetAddress",
            "value": "Street"
        },
        {
            "op": "Add",
            "path": "addresses[type eq \"work\"].locality",
            "value": "Locality"
        },
        {
            "op": "Add",
            "path": "addresses[type eq \"work\"].region",
            "value": "Region"
        },
        {
            "op": "Add",
            "path": "addresses[type eq \"work\"].postalCode",
            "value": "PostalCode"
        },
        {
            "op": "Add",
            "path": "addresses[type eq \"work\"].country",
            "value": "Country"
        }
    ]
}

Resulting Address object

    "addresses": [
        {
            "streetAddress": "Street",
            "type": "work"
        },
        {
            "locality": "Locality",
            "type": "work"
        },
        {
            "region": "Region",
            "type": "work"
        },
        {
            "postalCode": "PostalCode",
            "type": "work"
        },
        {
            "country": "Country",
            "type": "work"
        }
    ]

I would expect in this case that the operation returns the address as a single object.

    "addresses": [
        {
            "streetAddress": "Street",
            "locality": "Locality",
            "region": "Region",
            "postalCode": "PostalCode",
            "country": "Country",
            "type": "work"
        }
    ]
@simpleidserver simpleidserver self-assigned this Nov 10, 2024
@simpleidserver simpleidserver moved this to In Progress in Release 5.0.3 Nov 10, 2024
@simpleidserver simpleidserver added the bug Something isn't working label Nov 11, 2024
thabart added a commit that referenced this issue Nov 11, 2024
@simpleidserver
Copy link
Owner

Hello,

There is indeed a mistake in the current implementation.

When the addresses property does not exist in the user's SCIM representation, and an HTTP PATCH request is made with multiple operations targeting the same address, multiple addresses are incorrectly inserted.
However, if the addresses property already exists in the user's SCIM representation, the HTTP PATCH request functions as expected.

This issue has been fixed in the Release503 branch. Could you fetch the changes and try again?

KR,
SID

@simpleidserver simpleidserver moved this from In Progress to Done in Release 5.0.3 Nov 11, 2024
@simpleidserver simpleidserver closed this as completed by moving to Done in Release 5.0.3 Nov 11, 2024
@simpleidserver
Copy link
Owner

@JesseDeH : A pre-release package, version 5.0.3-rc1, has been published and is available for use. It includes the bug fix. Could you try it and check if your issue is resolved?

@JesseDeH
Copy link
Author

JesseDeH commented Nov 14, 2024

@simpleidserver Thanks for picking this up so quickly!

I've updated to the rc-1 version, but am getting an issue with the IBusHelper on the Api controllers on startup:

Error while validating the service descriptor 'ServiceType: SimpleIdServer.Scim.Api.BaseApiController
  Lifetime: Transient
  ImplementationType: SimpleIdServer.Scim.Api.GroupsController': 
    Unable to resolve service for type 'MassTransit.IMessageDataRepository' while attempting to activate 'SimpleIdServer.Scim.Helpers.BusHelper'.

I'm adding this without configuring MassTransit, so I am unsure what is causing this at the moment.
services.AddSIDScim(scimHostOptions => { scimHostOptions.IgnoreUnsupportedCanonicalValues = false; })


Edit: By providing a mock implementation of IMessageDataRepository in the services, I was able to run some integration tests, and from there I can confirm the bug is fixed. I'd like to do a further test directly with Entra, but I'll await your response on the above issue.

@JesseDeH JesseDeH reopened this Nov 14, 2024
@simpleidserver
Copy link
Owner

Hello,

I apologize for not mentioning earlier that the Program.cs file in the SCIM project has changed in the Release503 branch.

The following method has been added to configure the repository used by MassTransit for transferring very large messages:

IMessageDataRepository ConfigureMassTransitConfiguration(IServiceCollection services, MassTransitStorageConfiguration conf)
        {
            if (!conf.IsEnabled)
            {
                services.AddSingleton<IMessageDataRepository>(new InMemoryMessageDataRepository());
                return null;
            }

            IMessageDataRepository repository = null;
            switch (conf.Type)
            {
                case MassTransitStorageTypes.INMEMORY:
                    repository = new InMemoryMessageDataRepository();
                    break;
                case MassTransitStorageTypes.DIRECTORY:
                    repository = new FileSystemMessageDataRepository(new DirectoryInfo(AppDomain.CurrentDomain.BaseDirectory));
                    break;
                case MassTransitStorageTypes.AZURESTORAGE:
                    var client = new BlobServiceClient(conf.ConnectionString);
                    repository = client.CreateMessageDataRepository("message-data");
                    break;
            }

            services.AddSingleton(repository);
            return repository;
        }

Please either pull the latest changes to the Program.cs file from here: https://github.com/simpleidserver/SimpleIdServer/blob/Release503/src/Scim/SimpleIdServer.Scim.Startup/Program.cs or
add the missing dependency as follows:

services.AddSingleton<IMessageDataRepository>(new InMemoryMessageDataRepository());

@JesseDeH
Copy link
Author

I've applied your fix and confirmed that the patch also works in combination with Entra.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

No branches or pull requests

2 participants