python-scim / scim2-models

SCIM resources serialization and validation with Pydantic
https://scim2-models.readthedocs.io
Apache License 2.0
11 stars 2 forks source link

RFC7643: Complex attribute 'emails' does not declare subAttribute '$ref' #72

Open fflorent opened 2 months ago

fflorent commented 2 months ago

I use scim2-tester against the scimmy-routers middleware for express

It appears that scimmy-test sends $ref sub attributes for emails. Scimmy does not expect it and rejects the request with an error.

When I take a look at the RFC 7643, section 2.4 states that the $ref attribute is by default added to multi-valued attribute:

If not otherwise defined, the default set of sub-attributes for a multi-valued attribute is as follows:

On pages 53 and 54, the emails attribute does not contain this $ref, so I would say it's SCIMMY / scimmy-routers who are right :) :

{
        "name" : "emails",
        "type" : "complex",
        "multiValued" : true,
        "description" : "Email addresses for the user.  The value SHOULD be canonicalized by the service provider, e.g., 'bjensen@example.com' instead of 'bjensen@EXAMPLE.COM'. Canonical type values of 'work', 'home', and 'other'.",
        "required" : false,
        "subAttributes" : [
          {
            "name" : "value",
            "type" : "string",
            "multiValued" : false,
            "description" : "Email addresses for the user.  The value SHOULD be canonicalized by the service provider, e.g., 'bjensen@example.com' instead of 'bjensen@EXAMPLE.COM'. Canonical type values of 'work', 'home', and 'other'.",
            "required" : false,
            "caseExact" : false,
            "mutability" : "readWrite",
            "returned" : "default",
            "uniqueness" : "none"
          },
          {
            "name" : "display",
            "type" : "string",
            "multiValued" : false,
            "description" : "A human-readable name, primarily used for display purposes.  READ-ONLY.",
            "required" : false,
            "caseExact" : false,
            "mutability" : "readWrite",
            "returned" : "default",
            "uniqueness" : "none"
          },
          {
            "name" : "type",
            "type" : "string",
            "multiValued" : false,
            "description" : "A label indicating the attribute's function, e.g., 'work' or 'home'.",
            "required" : false,
            "caseExact" : false,
            "canonicalValues" : [
              "work",
              "home",
              "other"
            ],
            "mutability" : "readWrite",
            "returned" : "default",
            "uniqueness" : "none"
          },
          {
            "name" : "primary",
            "type" : "boolean",
            "multiValued" : false,
            "description" : "A Boolean value indicating the 'primary' or preferred attribute value for this attribute, e.g., the preferred mailing address or primary email address.  The primary attribute value 'true' MUST appear no more than once.",
            "required" : false,
            "mutability" : "readWrite",
            "returned" : "default"
          }
        ],
        "mutability" : "readWrite",
        "returned" : "default",
        "uniqueness" : "none"
      },
azmeuk commented 2 months ago

Thinking outloud here:

Indeed, Email inherits from MultiValuedComplexAttribute which implements the $ref attribute: https://github.com/yaal-coop/scim2-models/blob/3e9f7fc182a8328975f8eb9bd7ff9d537f34de7a/scim2_models/rfc7643/user.py#L49

https://github.com/yaal-coop/scim2-models/blob/3e9f7fc182a8328975f8eb9bd7ff9d537f34de7a/scim2_models/base.py#L735-L737

It is unclear to me on which level the schemas displayed on RFC7643 §8.7 are authoritative.

 The following sections provide representations of schemas for both
 SCIM resources and service provider schemas.  Note that the JSON
 representation has been modified for readability and to fit the
 specification format.

:point_up: This quote from RFC7643 §8.7 is unclear on what exactly has been modified for readability on the example schemas. Is the ref attribute has been removed for readability?

The following is intended as an example of the SCIM schema
representation in JSON format for SCIM resources.  Where permitted,
individual values and schema MAY change.  This example includes
schema representations for "User", "Group", and "EnterpriseUser";
other schema representations are possible.

:point_up: This quote from RFC7643 §8.7.1 seems to indicate that the schemas defined on that section are just examples and that implementations may change here and there where it is permitted. But what does permitted mean in this context? Is adding a $ref attribute permitted regarding §2.4 that you are quoting?

$ref
   The reference URI of a target resource, if the attribute is a
   reference. [...]

:point_up: This quote from RFC7643 §2.4 indicates that the $ref attribute should be a part of the default sub-attributes if the attribute is a reference. Indeed email is not a reference, and RFC7643 §4.1.2 details the display and type sub-attribute but does not tell anything about $ref:

emails
     Email addresses for the User.  The value SHOULD be specified
     according to [[RFC5321](https://datatracker.ietf.org/doc/html/rfc5321)].  Service providers SHOULD canonicalize the
     value according to [[RFC5321](https://datatracker.ietf.org/doc/html/rfc5321)], e.g., "bjensen@example.com" instead
     of "bjensen@EXAMPLE.COM".  The "display" sub-attribute MAY be used
     to return the canonicalized representation of the email value.
     The "type" sub-attribute is used to provide a classification
     meaningful to the (human) user.  The user interface should
     encourage the use of basic values of "work", "home", and "other"
     and MAY allow additional type values to be used at the discretion
     of SCIM clients.

There are other examples of multi-valued complex attributes in the documentations that do define the ref attribute, for example the Group members attribute from RFC7643 §4.2:

  The "value"
  sub-attribute contains the value of an "id" attribute of a SCIM
  resource, and the "$ref" sub-attribute must be the URI of a SCIM
  resource such as a "User", or a "Group".

In the end, I think you are right and the email $ref attribute should probably not be filled by scim2-tester.

fflorent commented 2 months ago

Thanks for your investigations around the spec! :monocle_face:

I am thinking of making a contribution, unless you know how to do that exactly. I would go for splitting MultiValuedComplexAttributes into several mixins:

class MultiValuedComplexAttribute(ComplexAttribute):
    pass

class TypeAttributeMixin:
    type: Optional[str] = None
    """A label indicating the attribute's function."""

class PrimaryAttributeMixin:
    primary: Optional[bool] = None
    """A Boolean value indicating the 'primary' or preferred attribute value
    for this attribute."""

class DisplayAttributeMixin:
    display: Annotated[Optional[str], Mutability.immutable] = None
    """A human-readable name, primarily used for display purposes."""

class ValueAttributeMixin:
    value: Optional[Any] = None
    """The value of an entitlement."""

class RefAttributeMixin:
    ref: Optional[Reference] = Field(None, serialization_alias="$ref")
    """The reference URI of a target resource, if the attribute is a
    reference."""

This way, for instance, the Email would inherit from MultiValuedComplexAttribute but use the other classes has mixins (note the absence of RefAttributeMixin):

class Email(MultiValuedComplexAttribute, ValueAttributeMixin, DisplayAttributeMixin, PrimaryAttributeMixin, TypeAttributeMixin):
   ...

Does it make sense to you?

pbouda commented 1 month ago

I like the idea with the mixins, it would make the models more flexible.

Just to add something here, I saw the scim2-models adds the primary attribute to GroupMember via the MultiValuedComplexAttribute. Is that expected? I tried to understand the discussion above but I am still not sure which fields are now mandatory via the RFC. Logically I think it does not make sense that there is a primary member.