Implement basic OIDC core server handling #12567
Conversation
|
@rullzer Would you be willing to review this, since you worked as the last person on these files? |
|
@tisoft yes I will :) I must admit I have not read up a lot on OIDC. I would need to do that as well. I'll try to look into this and get back to you. |
tisoft
force-pushed
the
oidc
branch
2 times, most recently
from
3f95ae5
to
d3585f7
Compare
Allow Nextcloud to be used as a OpenID Connect server. CLients can authenticate against it. Signed-off-by: Markus Heberling <markus.heberling@hengsbeck.de>
|
I have rebased to latest master. The test failures seem unrelated to me. Anything I can do to help the review process? |
|
@tisoft sorry for having this around for so long. Reading up on openid is still on my list but time 😉 Could you point me to the related RFC/component of openID I have to read up on to check this? Also I'll do a pass over the code tomorrow to give some more feedback. Thnx again. |
|
I tried to implement the minimal required elements of the specification here: https://openid.net/specs/openid-connect-core-1_0.html I have especially focused on the parts noted in the section 15.1. Mandatory to Implement Features for All OpenID Providers. In my opinion that section basically says, I must return an id_token, with some required fields and I must "support" some url parameters. Where "support" means, I can ignore them, as long as its usage does not lead to an error). Section 15.2 defines more requirements, that I would love to implement but that isn't basic anymore. :) I wanted to start with the most minimalistic implementation, that is actually usable. |
| 'auth_time' => $this->time->getTime(), | ||
|
|
||
| // optional, can be requested by claims, we don't support requesting claims as of now, so we just send them always | ||
| 'email' => $user->getEMailAddress(), |
There was a problem hiding this comment.
Is it a problem if those are empty?
There was a problem hiding this comment.
As I understand it, they can be left empty.
| $base64UrlPayload = str_replace(['+', '/', '='], ['-', '_', ''], base64_encode($payload)); | ||
|
|
||
| // Create Signature Hash | ||
| $signature = hash_hmac('sha256', $base64UrlHeader . "." . $base64UrlPayload, $client->getSecret(), true); |
There was a problem hiding this comment.
So the signature is required it seems. But how can it every be validated?
There was a problem hiding this comment.
The OpenID Connect client will do the same hash calculation on his side. Since the client knows the oauth client secret he can do that. That way the signature can be verified.
|
I think it would be useful to add https://github.com/RobDWaller/ReallySimpleJWT as dependency so that we get automatic updates. |
I was unsure on the process to get external dependencies in, so I tried to do this without :) But if using a library is preferred, I can change this. |
Signed-off-by: Markus Heberling <markus.heberling@hengsbeck.de>
|
Hi @tisoft - sorry that we didn't had time to look into this for the Nextcloud 16 milestone. We were quite busy with other tasks. We still appreciate the work you put into this, but the freeze for Nextcloud 16 is active since last Friday and I will put this into the Nextcloud 17 bucket. I hope that is okay for you. |
|
@MorrisJobke No problem. Just ping me, when I need to change something. 😄 |
|
Is this still worked on? And is there anything someone not from the nextcloud team can do? |
|
I‘m still willing to bring this in. Would need a feedback from the Nextcloud team. |
|
cc @rullzer |
|
It's time someone from Nextcloud steps up to the plate and do SOMETHING with this. So @rullzer and @MorrisJobke is anyone interested in getting this merged/reviewed before... lets say... 2025? or shall we start working op Startrek-Connect instead for Nextcloud 654? |
|
Fervently hoping this gets triaged for NC20 @rullzer @MorrisJobke |
|
This is one of those things where as I said I'm not against it. But we'll really need proper intergration test of OAuth (and then of course of OIDC as well). Since else this becomes this untested complex beast. If anybody is up to add those please do. That would help a lot. And then we can move this forward as well. |
MorrisJobke
added
2. developing
if I'm not mistaken this is still not done. shall we close this for now? |
|
Closing for now as there has been no traction on this. |
Allow Nextcloud to be used as a OpenID Connect provider. OpenID Connect Clients can authenticate against it. Fixes a part of #8846.
Manually tested with the OpenID Connect Playground and the OpenID Connect Generic Wordpress plugin
This is a minimal implementation. It could be extended by the following:
This is my first contribution to nextcloud, so I would be very helpful if someone would point out the shortcomings of my code. I will add a test case, but wanted to have feedback first, if this has any chance on being accepted.