feat: #1974 changes for mirroring custom claims

[fl0lli]

Related issue

#1974

Proposed changes

As described in #1974 custom claims are currently put under "ext".
For a transition period @aeneasr suggested that custom claims get put top-level and mirrored under "ext" (i.e. for backwards compatibility).
The original author of #1974 suggested using a configuration flag for custom claims which are allowed to go top-level.

I'm new to Go, but tried to implement the said features.

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.

Further comments

I'm happy for advice on how to retrieve config/env flags to use in the code and other coding guidelines I might have missed :)

I should have mentioned, that I wrote with @zepatrik on Slack, who told me to create a draft PR, so my code of the concept could be seen.

[CLAassistant]

All committers have signed the CLA.

[aeneasr]

Thank you for these changes and ideas! Unfortunately, the CI is currently failing and we also need some tests. The approach though is correct (adding the config flag, parsing the config flag, and echoing the session correctly without ext)! :)

[fl0lli]

Hey! I added some local tests which pass and am still trying to figure out how to get objects from the config.
I'm honestly unsure on where to put the tests, so I just created a new test file.
I think it could also be possible to integrate tests into the different grant_type test cases.
Feel free to tell me what could be improved! :)
I'd like to ask for some advice on how to handle reserved claims: If the config-flag contains a reserved claim, i.e. "sub" or "aud", should it be allowed, or should reserved claims never be overridden?

[zepatrik]

Nice, found some hints that should help you finish this 😉

[fl0lli]

It seems like session_custom_claims_test.go still passes all cases, I'm waiting for the CI results.
I encountered a problem, that the method GetJwtClaims() works as expected, but the access token doesn't change its shape (and the tests I wrote so far just check the method).
I am working with a custom WebApp which adds some custom claims to the access token and logs show this:

// the token handler gets called, the constructor has a hardcoded empty sub, and the ATLC get correctly read from the config
Token-Handler: sub = "" AllowedTopLevelClaims = [foo, bar, baz]

// the constructor of session.go (NewSessionWithCustomClaims); all parameters get passed as excpected
Constructor: sub = "" allowedTopLevelClaims = [foo, bar, baz]

// now the log statement for the token handler call
started handling request [...] host:localhost:4444 method:POST path:/oauth2/token query:<nil>

//somehow the method gets called with a sub now but no ATLC
GetJwtClaims(): s.Subject = "user" s.AllowedTopLevelClaims = []  s.Extra: map[foo:foo1 bar:bar1 baz:baz1]

I'm trying to find out what's happening there and write tests to check for the correct behaviour.

Seems like CI is also failing, I'm looking into it!
//might be a problem on the CI side, says there's no space left on the device

[aeneasr]

I've merged with master - for test instructions see:

https://github.com/ory/hydra/blob/master/README.md#develop

[fl0lli]

Sorry, I forgot that!
I added the ATLC to the session creation of the auth-handler, and now the access token looks like it should.
I just tested it with my local custom app, but will try to write a go test for it.

Do you want to also be able to set these allowed claims by environment variable? (maybe this even happens automatically, I did not look into it yet)

I'm also gonna look into the documentation and try to extend it.

[fl0lli]

Oops, forgot to format!

[aeneasr]

Yo, good job! Sorry for the long review time!

[vaggvisa]

This is a very welcome feature for my usecase. Unfortunately all attempts to make it work failed. I added the custom claim "my_claim to my config but hydra won't allow to start

The configuration contains values or keys which are invalid:
additionalProperties "allowed_top_level_claims" not allowed
level=fatal msg=Unable to instantiate configuration. audience=application error=map[message:I[#/oauth2] S[#/properties/oauth2/additionalProperties] additionalProperties "allowed_top_level_claims" not allowed] service_name=ORY Hydra service_version=v1.10.2

oauth2:
  allowed_top_level_claims:
    - my_claim

Any hints what I am doing wrong?

[fl0lli]

Hi @vaggvisa,
just to make sure, you build Hydra yourself, correct?
Because this change has not yet been released.
Let me know :)

[vaggvisa]

Hi @vaggvisa,
just to make sure, you build Hydra yourself, correct?
Because this change has not yet been released.
Let me know :)

I followed the instructions for a Docker based installation, basically

• pull newest master from git
• rebuild via docker-compose

[fl0lli]

In my understanding, what you did is docker-compose -f quickstart.yml up --build?
What I mean by "build Hydra yourself" is listed in Build Docker where you need Make to build a Docker Image from the source code.

Doing what you did results in having the newest source code locally, but using the released 1.10.2 image which is being pulled from the DockerHub (because that's what's used in the quickstart.yml).

Hope that helped, let me know if I might have misunderstood something! ;)

I followed the instructions for a Docker based installation, basically

• pull newest master from git
• rebuild via docker-compose

[vaggvisa]

What I mean by "build Hydra yourself" is listed in Build Docker where you need Make to build a Docker Image from the source code.

Oh boy! You saved my day - it works! Thanks for pointing out to rebuild the hydra image myself.

[fl0lli]

Alright, glad I could help :)

Oh boy! You saved my day - it works! Thanks for pointing out to rebuild the hydra image myself.

[alee792]

How long will the claims be mirrored to ext? Is that going to be configurable or just deprecated in a future release?