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

feat: oidc provider claims config option #753

Merged
merged 5 commits into from Oct 13, 2020

Conversation

NickUfer
Copy link
Contributor

Related issue

#735
@aeneasr

Proposed changes

Adds an option to oidc providers to request specific openid claims on an authorization request

Checklist

  • I have read the contributing guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

@NickUfer NickUfer force-pushed the NickUfer-provider-oidc-claims-config branch from 950ffe3 to 0846eda Compare October 10, 2020 10:32
Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution 😉
I think it would be better to have the requested_claims as a JSON object. Is there some way to validate them further or are they proprietary?

.schema/config.schema.json Outdated Show resolved Hide resolved
selfservice/strategy/oidc/provider_config.go Outdated Show resolved Hide resolved
selfservice/strategy/oidc/provider_generic_test.go Outdated Show resolved Hide resolved
@NickUfer
Copy link
Contributor Author

Thanks for your contribution
I think it would be better to have the requested_claims as a JSON object. Is there some way to validate them further or are they proprietary?

The spec defines that on the top level can only be a userinfo and / or id_token JSON object. Once inside userinfo or id_token the keys are pretty much proprietary but the values are constrained. The values could be:

  • null
  • another JSON object with the keys:
    • essential which indicates if this claim is, well, essential to the requester. Value can only be a boolean.
    • value which indicates that the value in the id_token or the userinfo endpoint for this field must have that exact value
    • or values which is the same as value but only with multiple values

Example claims request:

  {
   "userinfo":
    {
     "given_name": {"essential": true},
     "nickname": null,
     "email": {"essential": true},
     "email_verified": {"essential": true},
     "picture": null,
     "http://example.info/claims/groups": null
    },
   "id_token":
    {
     "auth_time": {"essential": true},
     "acr": {"values": ["urn:mace:incommon:iap:silver"] }
    }
  }

If it is possible to define this in the schema without concrete key names then this should be possible.

@zepatrik
Copy link
Member

Right, JSON schema can get a bit complicated. Here is my take:

{
      "additionalProperties": false,
      "patternProperties": {
        "^userinfo$|^id_token$": {
          "type": "object",
          "additionalProperties": false,
          "patternProperties": {
            ".*": {
              "oneOf": [
                {
                  "const": null,
                  "description": "Indicates that this Claim is being requested in the default manner"
                },
                {
                  "allOf": [
                    {
                      "propertyNames": {
                        "$comment": "This is to implement 'additionalProperties: false'",
                        "enum": [
                          "essential",
                          "value",
                          "values"
                        ]
                      }
                    },
                    {
                      "properties": {
                        "essential": {
                          "description": "Indicates whether the Claim being requested is an Essential Claim",
                          "type": "boolean"
                        }
                      }
                    },
                    {
                      "$comment": "Although not required by the spec, it makes no sense to allow both at the same time",
                      "oneOf": [
                        {
                          "properties": {
                            "value": {
                              "description": "Requests that the Claim be returned with a particular value",
                              "$comment": "There seem to be no constrains on value"
                            }
                          }
                        },
                        {
                          "properties": {
                            "values": {
                              "description": "Requests that the Claim be returned with one of a set of values, with the values appearing in order of preference",
                              "type": "array",
                              "items": {
                                "$comment": "There seem to be no constrains on individual items"
                              }
                            }
                          }
                        }
                      ]
                    }
                  ]
                }
              ]
            }
          }
        }
      }
    }

Maybe you can check if it works as expected by adding schema tests for it 😉
Have a look in test/schema and define above schema under the definition key. If you need help with that, let me know.
And maybe you can also improve the descriptions/add titles? I just copied the spec which is not super understandable.
Thanks a lot 😉

@zepatrik
Copy link
Member

When #757 is merged and you rebase, there should also be no timeout any more in the CI

@zepatrik
Copy link
Member

When everything passes, this looks good to me 👍

@NickUfer NickUfer force-pushed the NickUfer-provider-oidc-claims-config branch from 5b54885 to a38a55b Compare October 13, 2020 09:06
@NickUfer
Copy link
Contributor Author

NickUfer commented Oct 13, 2020

I will quickly merge this PR from @zepatrik then I would be ready to merge too

@aeneasr aeneasr merged commit bf94a40 into ory:master Oct 13, 2020
@aeneasr
Copy link
Member

aeneasr commented Oct 13, 2020

Awesome 🎉

Thank you for your contribution!

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

Successfully merging this pull request may close these issues.

None yet

3 participants