RFC-7591 - Dynamic Client Registration#735
RFC-7591 - Dynamic Client Registration#735sanjuthomas wants to merge 4 commits intoeclipse-vertx:masterfrom
Conversation
|
@pmlopes - thank you! I'm looking forward to hearing from others in the community. |
|
@sanjuthomas I'll have a look at the PR soon-ish, the goal is to make it for 5.1 |
| }); | ||
| } | ||
|
|
||
| private Future<DCRResponse> constructResponse(SimpleHttpResponse response, int expectedStatusCode) { |
There was a problem hiding this comment.
Error handling should be included at minimum with support for error and error_description as stated by the https://datatracker.ietf.org/doc/html/rfc7591#section-3.2.2
|
|
||
| @DataObject | ||
| @JsonGen(publicConverter = false) | ||
| public class DCRRequest { |
There was a problem hiding this comment.
I would suggest to include all possible values that are listed in https://datatracker.ietf.org/doc/html/rfc7591#section-2
This is a finite list so possibility to have these configured is a must, as with current options no real world usage of this this implementation is possible.
There was a problem hiding this comment.
@pendula95 - Keycloak exposes two endpoints to create clients.
- /realms/{realm}/clients-registrations/default
- /realms/{realm}/clients-registrations/openid-connect
The second one accepts the attributes mentioned here -> https://datatracker.ietf.org/doc/html/rfc7591#section-2 with the same names and naming conventions.
We initially decided to use /realms/{realm}/clients-registrations/default - I think it supports most, if not all, of the attributes mentioned in the spec, but with a different naming convention. I was wondering which one we should shoot for?
There was a problem hiding this comment.
I'd rather go for the second one since it aligns with the spec and makes this component generic (not highly coupled to keycloak)
There was a problem hiding this comment.
There is a discrepancy in how Keycloak handles the registration and get endpoints. The registration endpoint returns results in snake case, but the get endpoint returns them in camel case. This behavior will force us to maintain two POJOS or write a custom JSON parser.
/realms/{realm}/clients-registrations/openid-connect
/realms/master/clients-registrations/default/{client_id}
@pendula95 - any suggestions?
There was a problem hiding this comment.
Yes, looks like Keycloak DefaultClientRegistrationProvider camel snake case POJOs and OIDCClientRegistrationProvider uses snake case.
As I am not aware of the agreements that were made before this PR on which one to implement, I would suggest to implement OIDC one as it is more generic and should be supported by other servers as well. If you want to implement both then I would go for maintaining 2 sets of POJOs.
| */ | ||
| @VertxGen | ||
| public interface ClientRegistrationProvider { | ||
| Future<DCRResponse> create(String clientId); |
There was a problem hiding this comment.
Better provide a way to create a client with client metada data (DCRRequest) then just passing just a client id
|
|
||
| @DataObject | ||
| @JsonGen(publicConverter = false) | ||
| public class DCRResponse { |
There was a problem hiding this comment.
Possibly provide a structure that will capture all meta data returned by the auth server as in advance all possible fields can not be know but should be parsed by the client.
| * | ||
| */ | ||
| @VertxGen | ||
| public interface ClientRegistrationProvider { |
There was a problem hiding this comment.
Missing implementation of https://datatracker.ietf.org/doc/html/rfc7592#section-2.2
|
what is the status of this PR ? |
4 participants
Please refer to the original proposal to learn more about the Dynamic Client Registration and its needs in Vert.x.
This PR contains changes to implement RFC-7591 for Vert.x OAuth. Just so you know, this implementation has been tested with Keycloak for the Client Authenticated Registration Flow mentioned here.
This implementation supports two optional "manage registration flows" for reading and deleting a client.
I would start with the integration test case DCRKeycloak25_0_0_IT to get a jumpstart with this PR.