MicrosoftDocs / azure-docs

Open source documentation of Microsoft Azure
https://docs.microsoft.com/azure
Creative Commons Attribution 4.0 International
10.31k stars 21.49k forks source link

PATCH on microsoft.systemForCrossDomainIdentityManagement nuget package fails #42220

Closed ashkansirous closed 5 years ago

ashkansirous commented 5 years ago

I have created a SCIM integration using microsoft.systemForCrossDomainIdentityManagement nuget package but some of the PATCH requests fails when calling the API based on the RFC7644.

Also, I've tried to call the API using Azure Ad- Enterprise app as it has been described in the documentation, but that also fails on PATCH.

Looking at the logs and narrowing them down I've figured that the request is not in the same format as what microsoft.systemForCrossDomainIdentityManagement expects.

One patch request from AD is like below (which will fail):

{ "schemas":["urn:ietf:params:scim:api:messages:2.0:PatchOp"], "Operations": [ {"op":"Replace","path":"displayName","value":" User X"} ]}

While the request that works is like this:

{"schemas":["urn:ietf:params:scim:api:messages:2.0:PatchOp"] ,
"Operations":[ {"op":"Replace","path":"displayName","value":
[ {"$ref":null,"value":"User x"}]}]
}

I could make this work by implementing a middleware and overwriting the request to value:[{value:"xxx"}] like below, but it is just opening up to bugs. My replace format is as below:

public class PatchRequestUpdaterMiddleware : OwinMiddleware
{
     private const string OperationValueFinderRegex = "({[\\s\\w\":,.\\[\\]\\\\]*op[\\s\\w\":,.\\[\\]\\\\]*\"value\"\\s*:\\s*)(\"[\\w\\s\\-,.@?!*;\'\\(\\)]+\")"; //{"op":"x","value":"Andrew1"}

public override async Task Invoke(IOwinContext context)
    {
        if (context.Request.Method.ToLower() != "patch")
        {
           await Next.Invoke(context);
            return;
        }
        var streamReader = new StreamReader(context.Request.Body);
        string body = streamReader.ReadToEnd();
        body = Regex.Replace(body, OperationValueFinderRegex, m => $"{m.Groups[1].Value}[{{\"value\":{m.Groups[2].Value}}}]"); //{"op":"x","value":"Ashkan"} ==>> {"op":"x","value":[{"value":"Ashkan"}]}
        context.Request.Body = new MemoryStream(Encoding.UTF8.GetBytes(body));
        await Next.Invoke(context);
    }
 }

Furthermore, I've findout that Azure Ad was doing it wrong until Dec 2018 and then it has been fixed but there is no update on the nuget package since 2017.


Document Details

Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

MarileeTurscak-MSFT commented 5 years ago

@ashkansirous Thanks for your feedback! We will investigate and update as appropriate.

frankhu-2021 commented 5 years ago

hey @ashkansirous the Nuget package link you linked is to the 1.0.5 version. Are you using the most up to date version https://www.nuget.org/packages/Microsoft.SystemForCrossDomainIdentityManagement/2.0.5.95?

And in addition to that, you're saying that when you use the patch :

{ "schemas": [ "urn:ietf:params:scim:api:messages:2.0:PatchOp"], "Operations": [ { "op":"Add", "path":"manager", "value": [ { "$ref":"http://.../scim/Users/2819c223-7f76-453a-919d-413861904646", "value":"2819c223-7f76-453a-919d-413861904646"}]}]}

The v2.0.5 SystemForCrossDomainIdentityManagement library is throwing an error?

frankhu-2021 commented 5 years ago

@ashkansirous there may be some issues with the current library, but there are plans for fixes in the near future, however there are no hard deadlines for when this will happen. It should be within the next couple of months.

However, the library should be working for the patch scenario,

frankhu-2021 commented 5 years ago

Can you try setting up this sample to see if it works in your environment? It could be a configuration/code issue, https://github.com/Azure/AzureAD-BYOA-Provisioning-Samples

If it doesn't work with the provisioning sample, I suggest filing an issue against the azuread-byoa provisioning sample for further help on this.

ashkansirous commented 5 years ago

@FrankHu-MSFT Sorry, the version was added to the link unintentionaly. I'm on the latest version for sure :). I downloaded the solution and ran it so I could see for example create works as it should. Then tried to run my PATCH which failed with 400 bad request. https://share.getcloudapp.com/JruwedOX Then I've changed the Operation to "value":[ {"$ref":null,"value":"User x"}] and it workes fine. I can see that the request doesn't even hit their code as it doesn't hit my code and I can say for sure that it is in the library and not in their sample. Do you still suggest that I open another ticket for them and close this? :o

-- P.S: https://github.com/Azure/AzureAD-BYOA-Provisioning-Samples is an aweful sample for starting to use SCIM library! It has all the complications from Semaphores to Factories when it is not needed and then the code runs as a console application and not a web API. If the purpose of the solution is to help developers start using the library, it could have been so simple with 4-5 files ( an owin startup, a provider, a monitor and a simple user and group service that write directly to a text file as JSON) There is no need to do any design patterns or load data from a database with their Id, because no one cares about it in a sample project. :)

frankhu-2021 commented 5 years ago

Hey @ashkansirous I see. I'm sorry that there are some current issues with this and thank you for letting us know about this. This is currently in our backlog and plan on resolving this issue as soon as possible.

That being said, I'm currently looking in to this further to determine next steps. Thanks again,

ashkansirous commented 5 years ago

@FrankHu-MSFT Thanks a lot and I'm happy to help :). :+1:

frankhu-2021 commented 5 years ago

Hey @ashkansirous currently this is in our backlog right now. As there are currently no actionable items to resolve the doc issues as it's an issue with the library currently we will be closing out this doc issue and continue tracking this internally, there should be updates on this coming soon in the next couple of months.

I'm sorry for the inconvenience.

Thanks again, and the docs should be correct once the library has been fixed to handle this scenario accordingly, thanks for providing your workaround solution in the meanwhile.

ashkansirous commented 5 years ago

Hi @FrankHu-MSFT Thanks for reply. Of course what I care most is to fix the bug, but doesn't it make more sense to keep the issue open until you solve it and then close it? To my knowledge, when an issue is closed it means that it has been fix/ or it was irrelevant :) which is not our case. Other poeple would follow the same thread and try to get update on this. Also, how do I know that you have fixed it and apply the latest if you close the thread now? :)

frankhu-2021 commented 5 years ago

@ashkansirous the github issues here are meant for azure docs, not for product bugs. The official place to track this is the azure ad byoa sample github repo issues location.

ashkansirous commented 5 years ago

@FrankHu-MSFT I have an open issue for this exact issue there from Jan 18, 2019 (11 month ago) but there was no reply to it. Would it be the right place to track it? :) https://github.com/Azure/AzureAD-BYOA-Provisioning-Samples/issues/22

frankhu-2021 commented 5 years ago

yes @ashkansirous there will be updates there, as you can see craigmcm has provided an update on that issue accordingly.