Repository navigation
[client-v2] Require 'clientId' in the payload - #47533
Conversation
532aebb to
d2b3579
Compare
|
While I didn't know about #47574, I already handled it in my PR opened before this one. Let's go with your version as you have linked actual opened issue. |
|
@michalvavrik Ahh, didn't know about that. Whatever you prefer. |
There was a problem hiding this comment.
I think the CLI portion is fine (I handled it by simply detecting the violations, but making it explicit is better for backwards compatibility), thanks for fixing it.
As for #47524 (requiring clientId) I think this is architectural decision. It is bit suprising for the PUT as then you have it twice (in path and in the object). I remember that we used to use different DTO for PUT when I used to write CRUD microservices, which would avoid this problem, but that is just a preference.
However, it is aligned with what Steven keeps repeating (that PUT should require the full resource). I'll leave decision on that part to other reviewers.
|
@mabartos As discussed today during the WG meeting, can you please also make |
|
@sschu Makes sense. Added. |
Closes keycloak#47524 Signed-off-by: Martin Bartoš <mabartos@redhat.com>
Closes keycloak#47579 Signed-off-by: Martin Bartoš <mabartos@redhat.com>
Closes keycloak#47574 Signed-off-by: Martin Bartoš <mabartos@redhat.com>
| var request = switch (getHttpMethod()) { | ||
| case HttpPost.METHOD_NAME -> { | ||
| var r = new HttpPost(getClientsApiUrl()); | ||
| r.setHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON); | ||
| yield r; | ||
| } | ||
| case HttpPut.METHOD_NAME -> { | ||
| var r = new HttpPut(getClientApiUrl("%20")); | ||
| r.setHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON); | ||
| yield r; | ||
| } | ||
| default -> throw new IllegalStateException("Unexpected value: " + getHttpMethod()); | ||
| }; | ||
| setAuthHeader(request); |
There was a problem hiding this comment.
Why not to use getRequest?
There was a problem hiding this comment.
@vmuzikar For the POST it is doable, but not for PUT, as we check this special case as having the '%20' as clientId in the path.
Signed-off-by: Martin Bartoš <mabartos@redhat.com>8BD0
* Require clientId in the payload Closes keycloak#47524 Signed-off-by: Martin Bartoš <mabartos@redhat.com> * Require 'protocol' field to be specified Closes keycloak#47579 Signed-off-by: Martin Bartoš <mabartos@redhat.com> * Client API v2 CLI Client: Jakarta validation errors are not handled Closes keycloak#47574 Signed-off-by: Martin Bartoš <mabartos@redhat.com> * Use getRequest() for POST in test Signed-off-by: Martin Bartoš <mabartos@redhat.com> --------- Signed-off-by: Martin Bartoš <mabartos@redhat.com>
clientIdare the same - if it's missing, it fails - but it's still good to mark it as required for OpenAPI spec + be more defensive for some possible cases like this