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

Private claims implementation #1122

Open
wants to merge 40 commits into
base: 9.0.0-WIP
Choose a base branch
from

Conversation

skroczek
Copy link

@skroczek skroczek commented Jun 4, 2020

This PullRequest should implement private claims as described in RFC7519 in section 4.2. According to #1120 , this is still missing, but is wanted.
This implementation is based on that of scopes, which is basically also only a (registered) claim.
I don't think the implementation is finished yet. There are a few questions open and the testing is still missing completely. But the code works and can already be used for evaluation.
One open question is whether the claims should be checked for registered claims before the token is generated. If so, we would have to think about how to react in case of a violation. Will the claim simply be ignored, or should an exception be thrown?
About the testing: I have already looked at the test suite, but I'm not quite sure where and how to put it there. A suggestion would be very welcome.

In general I would be very happy about feedback.

@Pchelolo
Copy link

Pchelolo commented Jul 9, 2020

I have left a few comments inline for the things that popped out. We have not yet fully tested this implementation, will report the results as we go, but this looks great overall.

I have been looking into standards around this, and haven't found much: OAuth spec doesn't specify what would be used as a token, thus it doesn't contain any requirements or restrictions regarding the claims we can add into JWT. It seems logical that the library itself wouldn't impose any additional restrictions, so in my opinion the ClaimEntityInterface is rightfully very generic.

I've surveyed a few other popular oauth2 server frameworks that support to adding custom claims within OAuth:

  • Spring Security: allows to register a TokenEnhancer interface with the authentication manager. The enhancer receives the token object and a context-like Authentication object and can set additional information to the token object. Example. That is much more generic then what's implemented in the pull request, however this library encapsulates more logic than spring does, so I think following more generic approach taken by spring would be an overkill here.
  • nodejs oauth2-server - leaves generating the access token entirely to the library client, does not care about the access token semantics, treating it as an opaque string. Not viable approach here either
  • python OAuthLib - allows to provide a custom token generator, treating the token as an opaque string. alternatively has JWT-based implementation, that allows setting custom claims and is fairly similar to what's implemented here.

To sum up the approach taken in this PR seems reasonable to me, is on par with the capabilities of other libraries and does not violate any existing standards or RFCs as far as I could see.

@skroczek
Copy link
Author

Thank you very much for the close look and for the feedback. I have already answered to a few points. The suggestions all make sense to me, so I will implement them over the next days.

@Pchelolo
Copy link

@skroczek do you need any assistance implementing/testing your changes?

@skroczek
Copy link
Author

@Pchelolo It took me a while to find time to take care of it. But I think I've implemented all your suggestions except for the two tests. Thanks again for your suggestions and your patience. If you notice anything else, feel free to bring it up.

@Pchelolo
Copy link

Thanks a lot and sorry that some of the suggestions didn't make sense. The CI seem to be failing. We will test the new version out and I'll share the results

@garethellis36
Copy link

Hello folks. Is this PR likely to be merged soon? I think it would help me with a problem I am facing.

@Sephster
Copy link
Member

No timescale on this I'm afraid. It will be done as soon as I get time but there is a lot to check.

@Starfox64
Copy link

Sorry for bumping this but is this PR still alive?

I was hoping this could be merged for or before 9.00. I'm happy to help if required.

I'm asking to know if I should wait on an official implementation or just roll out my own like many have done by overriding the library.

@skroczek
Copy link
Author

skroczek commented Sep 2, 2021

We are now using zitadel as our IDP for various reasons. We have also switched from PHP to Go as our main language. Because of this, I unfortunately have neither much time nor interest to take care of it anymore. Nevertheless, this remains an excellent library. I will of course leave the PR open so that it can be merged with the repository if desired.

@PaolaRuby
Copy link
Contributor

@Sephster any news?

@fredsal
Copy link

fredsal commented Apr 20, 2022

Hi, is this PR likely to be merged soon?

@mariusjp
Copy link

Would also like to know why this is "ignored"? Custom claims are perfectly normal

@parallels999
Copy link

@Sephster is possible merging or rebasing this with master?

@systemsolutionweb
Copy link

@Sephster hi, we would love to have this feature
any news?

This is the PR with the most comments

Thanks

@@ -161,8 +161,18 @@ public function respondToAccessTokenRequest(
}
}

$privateClaims = [];

if ($this->claimRepository !== null) {
Copy link

Choose a reason for hiding this comment

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

First of all, sorry for the (very) late comment. But just got an email that people are interested again.
Maybe just personal preference, but checking if something is not null is not actually checking anything (because it can be ANYTHING at this point).
I'd rather check if it is what I want it to be, e.g. an instanceof check.

thephpleague:9.0.0-WIP -> skroczek:wip/private-claims Upstream merge and conflict resolve
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.