panva / openid-client

OAuth 2 / OpenID Connect Client API for JavaScript Runtimes
MIT License
1.83k stars 392 forks source link

Is throwing `expected claim "${claim}" in "${sourceName}"` the correct behaviour on missing source? #197

Closed seybsen closed 5 years ago

seybsen commented 5 years ago

An error expected claim "${claim}" in "${sourceName}" is thrown if distributed sources do not return all claims referenced in _claim_names: https://github.com/panva/node-openid-client/blob/master/lib/client.js#L55

Example:

{
    "sub": "dxu....",
    "_claim_names": {
        "address": "55eb6148-9ddf-4f2d-98a6-30cbae6ebbab",
        "gender": "55eb6148-9ddf-4f2d-98a6-30cbae6ebbab",
        "phone_number": "55eb6148-9ddf-4f2d-98a6-30cbae6ebbab",
        "given_name": "55eb6148-9ddf-4f2d-98a6-30cbae6ebbab",
        "family_name": "55eb6148-9ddf-4f2d-98a6-30cbae6ebbab",
        "email": "55eb6148-9ddf-4f2d-98a6-30cbae6ebbab"
    },
    "_claim_sources": {
        "55eb6148-9ddf-4f2d-98a6-30cbae6ebbab": {
            "access_token": "eyJraW...",
            "endpoint": "https://api-identity.example.com/userinfo"
        }
    }
}

when fetching the distributed claims gender is missing:

{
    "aud": "d7466ksfdzdxq",
    "sub": "dxu....",
    "id4me.identifier": "test.example.com",
    "id4me.identity": "test.example.com",
    "nbf": 1571127548,
    "address": {
        "street_address": "",
        "country": "",
        "formatted": "",
        "locality": "",
        "region": "",
        "postal_code": ""
    },
    "iss": "https://api-identity.example.com",
    "exp": 1571127578,
    "given_name": "Tester",
    "iat": 1571127548,
    "family_name": "Test",
    "email": "tester@example.com"
}

which then throws RPError: expected claim "gender" in "55eb6148-9ddf-4f2d-98a6-30cbae6ebbab"

Is this the correct behaviour according to RFC?

seybsen commented 5 years ago

https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.5.6.2

endpoint REQUIRED. OAuth 2.0 resource endpoint from which the associated Claim can be retrieved. The endpoint URL MUST return the Claim as a JWT.

... does this mean the endpoint MUST return the Claim or that the Claim MUST be JWT?

panva commented 5 years ago

Hi Sebastian,

https://openid.net/specs/openid-connect-core-1_0.html#AggregatedDistributedClaims

The JWT returned by the resource (endpoint) is the same JWT as for the aggregated claims, for which the following is defined

JWT that MUST contain all the Claims in the _claim_names object that references the corresponding _claim_sources member.

Its processing is the same, ergo throw if the _claim_names point to something that's not there.

Feel free to double check on the OIDC WG mailing list or issue tracker, i might be in the wrong to apply the same processing for distributed as for aggregated.

pawel-kow commented 5 years ago

OMHO throwing an exception is a bit hard, as more graceful handling would be possible.

In case of distributed claims IdP may not know what claims are stored by the claims source, therefore there is no such strict language as with Aggregated Claims. Anyway I've opened an issue at OIDF: https://bitbucket.org/openid/connect/issues/1117/core-562-behavior-for-distributed-claims

panva commented 5 years ago

Reopening based on WG feedback to the issue. Expect a fix in the next release.