fabbricadigitale / scimd

SCIM v2 golang implementation
MIT License
5 stars 1 forks source link

Server #46

Closed leodido closed 6 years ago

leodido commented 6 years ago

Needs a way to specify configs affecting the service command, but not in the service provider config.

Eg.,

We also must implement one (or more) way to provision configuration. Better if using a layer capable of abstracting the type of configuration source.

Eg.,

Please list here other things, editing this message.

cc @alelb, @leogr, @samechelon.

leodido commented 6 years ago

This message is about choices.

Choices we have to do.

Please list here other choices, editing this message.

cc @alelb, @leogr, @samechelon.

leodido commented 6 years ago

The subattributes meta.resourceType is enforced to be always returned. Thus it is now an element of the minimal set of attributes returned.

This has required also some bug fixing, in particular to the Contexts() function that now correctly executes filtering also on subattributes which parent has already been filtered out.

cc @leogr

leogr commented 6 years ago

@leodido As per RFC, service provider SHALL ignore extraneous attributes for interoperability purpose because they may exists in other schemas implemented by 3rd-party but not by the current service provider, thus I suggest to just filter out those attributes which cannot be contextualised (ie. Projection should not return them).

Also, I think we have to do not check for Undefined() paths here, because contextualised path cannot be undefined (if that was possible, it would mean that schema was invalid and it's not a Projection responsibility ensuring that).

Finally, I think also that this kind of API should never panic.

leodido commented 6 years ago

I finally fix'd (almost) everything we need to fix to make the GET /<resource>/:id to work.

Anyway there are already some gotchas we need to discuss.

A. Inclusions

1. Existing attribute(s)

GET localhost:8787/v2/Users/2819c223-7f76-453a-919d-ab1234567891?attributes=displayname,name.givenname

{
    "id": "2819c223-7f76-453a-919d-ab1234567891",
    "meta": {
        "resourceType": "User"
    },
    "schemas": [
        "urn:ietf:params:scim:schemas:core:2.0:User",
        "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"
    ],
    "urn:ietf:params:scim:schemas:core:2.0:User": {
        "displayName": "Tiffany Fork",
        "name": {
            "givenName": "Tiffany"
        }
    }
}

Notice the values of meta.location, meta.created and so on (ie., zero values since not grabbed from storage) ...

Imho we should fix this ... cc @leogr, @alelb

2. All unknown attributes

GET localhost:8787/v2/Users/2819c223-7f76-453a-919d-ab1234567891?attributes=unk,ciao,pippo

{
    "id": "2819c223-7f76-453a-919d-ab1234567891",
    "meta": {
        "location": "https://example.com/v2/Users/2819c223-7f76-453a-919d-ab1234567890",
        "resourceType": "User",
        "created": "2010-01-23T04:56:22Z",
        "lastModified": "2011-05-13T04:42:34Z",
        "version": "W/\"a330bc54f0671c9\""
    },
    "schemas": [
        "urn:ietf:params:scim:schemas:core:2.0:User",
        "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"
    ],
    "urn:ietf:params:scim:schemas:core:2.0:User": {
        "active": true,
        "addresses": [
            {
                "...": "...",
            },
            {
                "...": "...",
            }
        ],
        "displayName": "Tiffany Fork",
        "...": "..."
}

Notice the strange (null) value of lastModified ... cc @alelb

3. No inclusions

GET localhost:8787/v2/Users/2819c223-7f76-453a-919d-ab1234567891

JSON as (A.2), (B.2).

4. Existing extension attribute(s)

GET localhost:8787/v2/Users/2819c223-7f76-453a-919d-ab1234567891?attributes=urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:costCenter,urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:manager.$ref,displayname

{
    "id": "2819c223-7f76-453a-919d-ab1234567891",
    "meta": {
        "resourceType": "User"
    },
    "schemas": [
        "urn:ietf:params:scim:schemas:core:2.0:User",
        "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"
    ],
    "urn:ietf:params:scim:schemas:core:2.0:User": {
        "displayName": "Tiffany Fork"
    },
    "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User": {
        "costCenter": "4130",
        "manager": {
            "$ref": "../Users/26118915-6090-4610-87e4-49d8ca9f808d"
        }
    }
}

5. Existing reference attribute(s)

GET localhost:8787/v2/Users/2819c223-7f76-453a-919d-ab1234567891?attributes=displayName,groups.$ref

{
    "id": "2819c223-7f76-453a-919d-ab1234567891",
    "meta": {
        "resourceType": "User"
    },
    "schemas": [
        "urn:ietf:params:scim:schemas:core:2.0:User",
        "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"
    ],
    "urn:ietf:params:scim:schemas:core:2.0:User": {
        "displayName": "Tiffany Fork",
        "groups": [
            {
                "$ref": "https://example.com/v2/Groups/fc348aa8-3835-40eb-a20b-c726e15c55b5"
            }
        ]
    }
}

B. Exclusions

1. Existing attribute(s)

GET localhost:8787/v2/Users/2819c223-7f76-453a-919d-ab1234567891?excludedAttributes=displayName,locale,nickName,profileurl,title,timezone,username,usertype,preferredlanguage,active,x509certificates.value,photos.value,photos.type,phonenumbers.type,phonenumbers.value,ims.type,ims.value,name.familyname,name.formatted,name.honorificsuffix,name.honorificprefix,emails.type,emails.value,groups.value,groups.display,addresses.type,addresses.streetaddress,addresses.type,addresses.region,addresses.country,addresses.formatted,addresses.postalcode,addresses.locality

{
    "id": "2819c223-7f76-453a-919d-ab1234567891",
    "meta": {
        "location": "https://example.com/v2/Users/2819c223-7f76-453a-919d-ab1234567890",
        "resourceType": "User",
        "created": "2010-01-23T04:56:22Z",
        "lastModified": "2011-05-13T04:42:34Z",
        "version": "W/\"a330bc54f0671c9\""
    },
    "schemas": [
        "urn:ietf:params:scim:schemas:core:2.0:User",
        "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"
    ],
    "urn:ietf:params:scim:schemas:core:2.0:User": {
        "addresses": [
            {
                "primary": true
            },
            {}
        ],
        "emails": [
            {
                "primary": true
            },
            {}
        ],
        "groups": [
            {
                "$ref": "https://example.com/v2/Groups/fc348aa8-3835-40eb-a20b-c726e15c55b5"
            }
        ],
        "ims": [
            {}
        ],
        "name": {
            "givenName": "Tiffany",
            "middleName": "Geraldine"
        },
        "phoneNumbers": [
            {},
            {}
        ],
        "photos": [
            {},
            {}
        ],
        "x509Certificates": [
            {}
        ]
    },
    "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User": {
        "costCenter": "4130",
        "department": "Tour Operations",
        "division": "Theme Park",
        "employeeNumber": "701984",
        "manager": {
            "$ref": "../Users/26118915-6090-4610-87e4-49d8ca9f808d",
            "displayName": "John Smith",
            "value": "26118915-6090-4610-87e4-49d8ca9f808d"
        },
        "organization": "Universal Studios"
    }
}

Notice the complex attributes with their zero values (eg. [], {}) ...

Similar to the case A.1.

We could think to a layer responsible to eliminate zero values from responses. cc @leogr, @alelb

And also pay attention to do not eliminate attributes for which the nil value is possible.

2. All unknown exclusions

GET localhost:8787/v2/Users/2819c223-7f76-453a-919d-ab1234567891?excludeAttributes=pippo,pluto

{
    "id": "2819c223-7f76-453a-919d-ab1234567891",
    "meta": {
        "location": "https://example.com/v2/Users/2819c223-7f76-453a-919d-ab1234567890",
        "resourceType": "User",
        "created": "2010-01-23T04:56:22Z",
        "lastModified": "2011-05-13T04:42:34Z",
        "version": "W/\"a330bc54f0671c9\""
    },
    "schemas": [
        "urn:ietf:params:scim:schemas:core:2.0:User",
        "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"
    ],
    "urn:ietf:params:scim:schemas:core:2.0:User": {
        "active": true,
        "...": "..."
    },
    "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User": {
        "costCenter": "4130",
        "...": "..."
    }
}

3. No exclusions

GET localhost:8787/v2/Users/2819c223-7f76-453a-919d-ab1234567891

JSON as (A.2), (B.2).

4. Existing extension attribute(s)

GET localhost:8787/v2/Users/2819c223-7f76-453a-919d-ab1234567891?excludedAttributes=urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:costcenter,urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:department

{
    "id": "2819c223-7f76-453a-919d-ab1234567891",
    "meta": {
        "location": "https://example.com/v2/Users/2819c223-7f76-453a-919d-ab1234567890",
        "resourceType": "User",
        "created": "2010-01-23T04:56:22Z",
        "lastModified": "2011-05-13T04:42:34Z",
        "version": "W/\"a330bc54f0671c9\""
    },
    "schemas": [
        "urn:ietf:params:scim:schemas:core:2.0:User",
        "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"
    ],
   "urn:ietf:params:scim:schemas:core:2.0:User": {
        "active": true,
        "...": "..."
    },
    "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User": {
        "division": "Theme Park",
        "employeeNumber": "701984",
        "manager": {
            "$ref": "../Users/26118915-6090-4610-87e4-49d8ca9f808d",
            "displayName": "John Smith",
            "value": "26118915-6090-4610-87e4-49d8ca9f808d"
        },
        "organization": "Universal Studios"
    }
}

5. Existing reference attribute(s)

GET localhost:8787/v2/Users/2819c223-7f76-453a-919d-ab1234567891?excludedAttributes=urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:manager.value,urn:ietf:params:scim:schemas:extension:enterprise:2.0:User:manager.$ref,groups.$ref

{
    "id": "2819c223-7f76-453a-919d-ab1234567891",
    "meta": {
        "location": "https://example.com/v2/Users/2819c223-7f76-453a-919d-ab1234567890",
        "resourceType": "User",
        "created": "2010-01-23T04:56:22Z",
        "lastModified": "2011-05-13T04:42:34Z",
        "version": "W/\"a330bc54f0671c9\""
    },
    "schemas": [
        "urn:ietf:params:scim:schemas:core:2.0:User",
        "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"
    ],
   "urn:ietf:params:scim:schemas:core:2.0:User": {
        "active": true,
        "...": "...",
        "groups": [
            {
                "display": "Employees",
                "value": "fc348aa8-3835-40eb-a20b-c726e15c55b5"
            }
        ],
        "...": "..."
    },
    "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User": {
        "costCenter": "4130",
        "department": "Tour Operations",
        "division": "Theme Park",
        "employeeNumber": "701984",
        "manager": {
            "displayName": "John Smith"
        },
        "organization": "Universal Studios"
    }
}

C. Both

TBD

leogr commented 6 years ago

For inclusion With the only exception of meta.resourceType that must be always present in our implementation, there're two things you have to check:

That said I didn't get why externalId is returned, even it has omitempty

leogr commented 6 years ago

For exclusion About complex attributes with their zero values (eg. [], {}), I think it's an issue with mongo projection. I'm not sure, but it may happen when you project a sub-attribute that's not present within the document. Anywau, something like that:

        "phoneNumbers": [
            {},
            {}
        ],

it's not nil nor empty, so cannot be a JSON marshal responsibility to clean up them, event they have omitempty.

leodido commented 6 years ago

About your first comment you were right.

I fixed the typo and setup the other fields of meta as not required and omitempty.

About your last comment investigating I found the following.

Assume a query with excludedAttributes=emails.type,emails.value.

The object returned from the mongo adapter will contain:

emails:[map[primary:true] map[]]

Notice the second field (ie., the empty map).

And then I found that resource.Resource MarshalJSON impl. does not implement omitempty currently.

leogr commented 6 years ago

@leodido note that “externalId" is actually missing from toResource here. Also, it should be better if the "omitempty" check was applied to toDoc too (just to avoid saving the empty field, not mandatory).

leogr commented 6 years ago

About

emails:[map[primary:true] map[]]

IMHO, the storage should not return that, however I'm not sure if we can fix that within the storage layer.

leogr commented 6 years ago

@leodido I noticed that response examples above are wrong. For instance the following:

{
    "id": "2819c223-7f76-453a-919d-ab1234567891",
    "meta": {
        "resourceType": "User"
    },
    "schemas": [
        "urn:ietf:params:scim:schemas:core:2.0:User",
        "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"
    ],
    "urn:ietf:params:scim:schemas:core:2.0:User": {
        "displayName": "Tiffany Fork",
        "name": {
            "givenName": "Tiffany"
        }
    }
}

should be so:

{
    "id": "2819c223-7f76-453a-919d-ab1234567891",
    "meta": {
        "resourceType": "User"
    },
    "schemas": [
        "urn:ietf:params:scim:schemas:core:2.0:User",
        "urn:ietf:params:scim:schemas:extension:enterprise:2.0:User"
    ],
    "displayName": "Tiffany Fork",
    "name": {
         "givenName": "Tiffany"
     }
}

Because only extensions' attributes must be enclosed within base schema's attributes must not be enclosed per namespace within a complex, base schema attributes must not.

See https://tools.ietf.org/html/rfc7644#section-3.4.1

Marshalling/Unmarshalling should be implemented according with those rules.

What's wrong?

leodido commented 6 years ago

@leogr you're right.

Probably the cause is this line.

leogr commented 6 years ago

Hey you, by https://github.com/fabbricadigitale/scimd/pull/46/commits/166b561e2e564149be87de9467e3acf3217d38f0 I've fixed empty {} issue. Double check please, and let me know if it works properly.

alelb commented 6 years ago

@leodido , @leogr Take a look to my last commit please. I moved ValidateRequired in required package to avoid import cycle with .../api/attr package. We can move all functions to perform validation in a shared package. What do you think about this? Any other proposals? Thanks!

leodido commented 6 years ago

@alelb regarding validations, as discussed yersterday.

We could profit from gin validation capabilities for the common attributes struct.

Rather for the data map (complex) of the resource we could write some line of code that will perform structural validation based on the attribute characteristics of the attribute (ie., keys) contained in the data map.

leodido commented 6 years ago

Hey @marcomenoni welcome!

Thanks a lot for the contribution. :trophy:

Only one main observation.

We should move away the logic (password hashing via bcrypt) implemented by your listeners into a dedicated package. So to split the adapter from its listeners: the idea is that they have to be plug-n-play (imagine another way of hashing password implemented via another listener handler) and also storage agnostic.

So i propose to move this part of the code in another package. I propose to call it listeners, within storage/listeners folder.

This way it could be plugged one day also on other kind of storage without duplicating code for example. And the code is well separated.

marcomenoni commented 6 years ago

ok, I am going to move the listeners in a dedicated package

leodido commented 6 years ago

JSON file needs to be embedded within the binary (as I done last week) but this results in a lot of extra space and computation.

Furthermore loading from paths (substituted with virtual ones pointing to the embedded representation of JSON files, at runtime) over complicates the configuration system.

leodido commented 6 years ago

We are almost there guys ...

cc @fabbricadigitale/openinnovation