Repository navigation
fix for service account role management in admin v2 - #48166
Conversation
|
@edewit Can you please rebase? |
|
rebased |
|
@edewit Can you please fix the build failures? |
|
@edewit Could you please check the failing test? |
|
@edewit Spotless is failing. |
|
done |
|
@edewit Can you rebase once more please? |
|
done |
|
@edewit Thanks but build still seem to fail. |
468945b to
7ae6374
Compare
There was a problem hiding this comment.
@edewit thanks for the PR! Now when DefaultClientService no longer needs RealmAdminResource, shouldn't the realmAdminResource field be cleaned up from DefaultClientsApi and DefaultAdminApi? It's stored but never read and DefaultAdminApi constructs a full RealmsAdminResource + TokenManager on every request just to discard them.
See here what I mean: edewit#16
| fireAdminEvent(alreadyExists ? OperationType.UPDATE : OperationType.CREATE, mapper.fromModel(model)); | ||
| return new CreateOrUpdateResult(mapper.fromModel(model), !alreadyExists); |
There was a problem hiding this comment.
| var representation = mapper.fromModel(model); | |
| fireAdminEvent(alreadyExists ? OperationType.UPDATE : OperationType.CREATE, representation); | |
| return new CreateOrUpdateResult(representation, !alreadyExists); |
I just wanted to point out that mapper.fromModel(model) is called twice here. I already reported this on #47029 (comment) for NewClientService but it was marked as expected. Just leaving it here for a reference as we need to revisit it.
There was a problem hiding this comment.
This is wrongly applied - the fireAdminEvent call on line is left in place still. Regardless, I wanted to just point this out, not fix it - as I said above, I was told that this is expected and it will be solved rather globally in #46553.
fixes: keycloak#47966 Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
…minApi Signed-off-by: Peter Zaoral <pzaoral@redhat.com>
…ent/DefaultClientService.java Co-authored-by: Peter Zaoral <pepo48@gmail.com> Signed-off-by: Erik Jan de Wit <edewit@redhat.com>
|
Tests failed. |
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
There was a problem hiding this comment.
Erik, thanks! I quickly went through it again as more commits have been added since my last check. Specifically the one, where you fixed the context. I think we are missing an NPE check there. This follows how v1 handles it in services/resources/admin/RealmsAdminResource.java (line 212). Without it, a request to /admin/api/nonexistent-realm/clients/v2 returns a 500 instead of 404.
Other than that LGTM.
…pi/DefaultAdminApi.java Co-authored-by: Peter Zaoral <pepo48@gmail.com> Signed-off-by: Erik Jan de Wit <edewit@redhat.com>
…pi/DefaultAdminApi.java Co-authored-by: Peter Zaoral <pepo48@gmail.com> Signed-off-by: Erik Jan de Wit <edewit@redhat.com>
* fix for service account role management in admin v2 fixes: keycloak#47966 Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * fixed merge error Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * fix test Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * fixed build error Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * fixed tests Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * Remove unused RealmAdminResource from DefaultClientsApi and DefaultAdminApi Signed-off-by: Peter Zaoral <pzaoral@redhat.com> * Update rest/admin-v2/services/src/main/java/org/keycloak/services/client/DefaultClientService.java Co-authored-by: Peter Zaoral <pepo48@gmail.com> Signed-off-by: Erik Jan de Wit <edewit@redhat.com> * also update the context Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * fix merge error Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * revert change Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> * Update rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/DefaultAdminApi.java Co-authored-by: Peter Zaoral <pepo48@gmail.com> Signed-off-by: Erik Jan de Wit <edewit@redhat.com> * Update rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/DefaultAdminApi.java Co-authored-by: Peter Zaoral <pepo48@gmail.com> Signed-off-by: Erik Jan de Wit <edewit@redhat.com> --------- Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com> Signed-off-by: Peter Zaoral <pzaoral@redhat.com> Signed-off-by: Erik Jan de Wit <edewit@redhat.com> Co-authored-by: Peter Zaoral <pzaoral@redhat.com> Co-authored-by: Peter Zaoral <pepo48@gmail.com>
fixes: #47966
Signed-off-by: Erik Jan de Wit erikjan.dewit@gmail.com