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

Support the enterprise extension by default #72

Open
imulab opened this issue May 5, 2020 · 14 comments · Fixed by #73
Open

Support the enterprise extension by default #72

imulab opened this issue May 5, 2020 · 14 comments · Fixed by #73
Assignees

Comments

@imulab
Copy link
Owner

imulab commented May 5, 2020

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 imulab self-assigned this May 5, 2020
@imulab
Copy link
Owner Author

imulab commented May 5, 2020

@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
Copy link

plamenGo commented May 5, 2020

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

@plamenGo
Copy link

plamenGo commented May 5, 2020

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
Copy link
Owner Author

imulab commented May 6, 2020

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

@imulab imulab linked a pull request May 6, 2020 that will close this issue
@imulab imulab closed this as completed in #73 May 6, 2020
@plamenGo
Copy link

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

@imulab
Copy link
Owner Author

imulab commented Jun 27, 2020

can you post your request here?

@imulab imulab reopened this Jun 27, 2020
@plamenGo
Copy link

plamenGo commented Jun 27, 2020

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 #75

@imulab
Copy link
Owner Author

imulab commented Jun 29, 2020

@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.

@imulab imulab closed this as completed Jun 29, 2020
@plamenGo
Copy link

plamenGo commented Jun 29, 2020

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
Copy link
Owner Author

imulab commented Jun 30, 2020

@plamenGo let me look into this...

@imulab imulab reopened this Jun 30, 2020
@plamenGo
Copy link

Hi, wondering if there is any advice on this.

@imulab
Copy link
Owner Author

imulab commented Jul 26, 2020

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

@plamenGo
Copy link

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

@christiannicola
Copy link

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants