imulab / go-scim

Building blocks for servers implementing Simple Cloud Identity Management v2
MIT License
145 stars 56 forks source link

Support the enterprise extension by default #72

Open imulab opened 4 years ago

imulab commented 4 years ago

As use cases of Microsoft AD seems popular with this library, it will be beneficial to support enterprise extension in the showcase project out-of-box so users can test it out.

imulab commented 4 years ago

@plamenGo I added the enterprise extension schema and updated the default User resource type in the showcase project. Did a simple create and get User on it, seems fine. Do you mind giving it a run on feature/72 branch? (You will need to make docker to build the image first).

plamenGo commented 4 years ago

OK, I can check it out -- I did the same a forked repo, so I will check for any differences. Thanks @imulab!

plamenGo commented 4 years ago

Looks good, works fine.

Originally, I used this schema (with a .json ext):

user_enterprise_extension_schema.txt

It has different indices specified, and the manager schema is not called out. No idea whether that makes a difference. What you have seems good, the Microsoft tests are happy.

imulab commented 4 years ago

Great, let me merge this in with your racing fix.

plamenGo commented 4 years ago

looks like the enterprise schema does not work with PATCH calls, keep getting "error invalid path"

imulab commented 4 years ago

can you post your request here?

plamenGo commented 4 years ago

PATCH /Users/f020ebd9-8a5f-49de-b4d1-b47f755b7747

either one of these bodies fails

{"schemas":["urn:ietf:params:scim:api:messages:2.0:PatchOp"],"Operations":[{"op":"Add","path":"urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:manager","value":"45619541-95de-4d5e-9872-571b5d2c5577"}]}"
{"schemas":["urn:ietf:params:scim:api:messages:2.0:PatchOp"],"Operations":[{"op":"Add","path":"urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:employeeNumber","value":"6546579"}]}

this issue is same as https://github.com/imulab/go-scim/issues/75

imulab commented 4 years ago

@plamenGo

Located the issue! I forgot to call the critical crud.Register method so the compilers didn't learn about the URNs at all.

I added the calls during the context initialization for the user and group resource types and it seems to be working now. Also fixed a bunch of test cases affected by adding the schema extension definition in the public resource type definition (they are all crying about the extension being unregistered).

This was fixed in the latest pkg/v2.1.3 release.

Thanks for providing the failing request! Closing this one for now.

plamenGo commented 4 years ago

Hi, thank you for looking into this!

I tested, and this is what I'm finding:

It works fine for the add op.

Specifically, it works for this request:

{"schemas":["urn:ietf:params:scim:api:messages:2.0:PatchOp"],"Operations":[{"op":"Add","path":"urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:employeeNumber","value":"6546579"}]}

It fails for the manager request from my prev, but that might be poorly formatted by Azure. This version works:

{"schemas":["urn:ietf:params:scim:api:messages:2.0:PatchOp"],"Operations": [{"op":"Add","path":"urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:manager","value":{"value":"6546579"}}]}

I believe this is an issue where Azure does not respect that manager should be a complex type, and just sends a simple value, which we are not expecting as per the schema.

However when the 'op' is replace I am still getting the invalid path error consistently. Is there something else we need to do to handle 'replace' ops correctly?

{
    "schemas": [
        "urn:ietf:params:scim:api:messages:2.0:Error"
    ],
    "status": 400,
    "scimType": "invalidPath",
    "detail": "invalidPath: error compiling path"
}

My request payload is this, but I can pretty much repro with with any PATCH call that updates the extension schema (replace op):

{"schemas":["urn:ietf:params:scim:api:messages:2.0:PatchOp"],

"Operations":[{"op":"replace","path":"urn:ietf:params:scim:schemas:extension:enterprise:2.0:User","value":{"department":"bob"}}]}

PATCH to http://localhost:5000/Users/id

imulab commented 4 years ago

@plamenGo let me look into this...

plamenGo commented 4 years ago

Hi, wondering if there is any advice on this.

imulab commented 4 years ago

@plamenGo Sorry, been busy with another project, haven't checked back on this one. Will do so on Tuesday.

plamenGo commented 4 years ago

@imulab do you have any advice if I wanted to attempt to fix it?

christiannicola commented 1 year ago

@plamenGo I did some testing on my end and it seems that replace operations in PATCH calls are working fine as long as you register the extension schema with crud.Register. I was running into the same issues as you were, but that fixed it.

You can also change the extension schema to make manager a simple attribute if Azure requires this - all you need to do is to change the json schema files accordingly.