oauth2-server Private claims implementation

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.

Jun 04 '20 14:06 skroczek

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.

Jul 09 '20 21:07 Pchelolo

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.

Jul 13 '20 09:07 skroczek

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

Jul 16 '20 14:07 Pchelolo

@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.

Jul 16 '20 22:07 skroczek

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

Jul 16 '20 22:07 Pchelolo

We've tested this implementation more and it seems to work well. I've left a comment about backwards compatibility inline, otherwise this LGTM. I'm not a maintainer of the library, so I would ask @Sephster to kindly look at it.

Jul 17 '20 18:07 Pchelolo

I already expected, after the last review, that the pull request will not make it into the current master branch. I noticed the breaking changes quite late. I have implemented your suggested changes so far. Anyway, I agree of course, if it is merged into the 9.0.0-WIP branch.

Currently there seems to be a problem with styleci, but unfortunately I couldn't find out why the check fails. As far as I have seen, no changes have been made that could cause the check to fail. However, this is also the first time I am working with this service.

I will be poorly reachable in the next two weeks, so it may take some time before I can respond.

Jul 27 '20 19:07 skroczek

Thanks @skroczek for the changes. If you are happy to, I can pick up any remaining issues to push this through to the 9.x branch? Will aim to do that tomorrow

Jul 27 '20 20:07 Sephster

Hi @Sephster, if you could take care of the rest of these issues, I would be very grateful. Thank you very much for your support.

Jul 28 '20 08:07 skroczek

Hello, anything we can do to help getting this in quicker?

Aug 10 '20 13:08 Pchelolo

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

Mar 12 '21 19:03 garethellis36

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

Mar 13 '21 18:03 Sephster

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.

Sep 01 '21 15:09 Starfox64

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.

Sep 02 '21 07:09 skroczek

@Sephster any news?