Repository navigation
Fix POST /admin/realms/{realm}/clients-initial-access returning HTTP 200 instead of 201 - #49232
Conversation
afb6839 to
8a51a2e
Compare
There was a problem hiding this comment.
Thank you. Let's try to limit level of changes to only necessary ones and let's test your changes locally before you push them. I think description of this PR why is it not possible is not precise.
| @@ -39,7 +40,7 @@ public interface ClientInitialAccessResource { | |||
| @POST | |||
| @Consumes(MediaType.APPLICATION_JSON) | |||
| @Produces(MediaType.APPLICATION_JSON) | |||
| ClientInitialAccessPresentation create(ClientInitialAccessCreatePresentation rep); | |||
| Response create(ClientInitialAccessCreatePresentation rep); | |||
There was a problem hiding this comment.
this is not necessary, Quarkus REST uses instanceof under the hood
There was a problem hiding this comment.
It's also breaking as this interface is part of the java client. This change, and all of the test logic updates should be removed.
Either introduce a seperate method, e.g.
Response doCreate(ClientInitialAccessCreatePresentation rep);
Or test the server side change with a rest call not via the java client.
There was a problem hiding this comment.
Thank you for the feedback, @shawkins.
I have addressed both points:
- Reverted the breaking change — the create() method on ClientInitialAccessResource is back to returning ClientInitialAccessPresentation unchanged. All test files that were updated to call .readEntity() have also been reverted. Existing callers of the Java client are unaffected.
- Added doCreate() as suggested — a separate method returning the raw Response has been added to the interface: Response doCreate(ClientInitialAccessCreatePresentation rep);
- This gives callers who need HTTP-level details (status code, Location header) an escape hatch without touching the existing create() method.
- Test updated to use doCreate() — the new test testCreateReturns201WithLocationHeader now uses doCreate() through the typed Java client to assert the 201 Created status and the presence of the Location header, which is exactly what the server-side fix introduced.
There was a problem hiding this comment.
I don't understand why should we enhance client API just for testing purposes. Showing users create and doCreate, do we have some user scenario where users care about response status? if not, you can test response status using plain HTTP request instead of the REST client.
There was a problem hiding this comment.
@michalvavrik You're right. doCreate was added following @shawkins's suggestion as an alternative to avoid the breaking change, but it has no real user scenario. It exists purely for the test assertion. I've removed it and rewritten testCreateReturns201WithLocationHeader to use a plain HTTP POST via CloseableHttpClient, asserting the 201 status and Location header directly on the raw response without going through the typed REST client.
There was a problem hiding this comment.
@michalvavrik , @shawkins can I get approval now?
There was a problem hiding this comment.
The other part of this change is to at least add more javadocs to the get / set status methods so that this doesn't happen again.
There was a problem hiding this comment.
Updated javadocs for the get/set status methods.
| @@ -83,7 +81,7 @@ public ClientInitialAccessResource(KeycloakSession session, AdminPermissionEvalu | |||
| @Tag(name = KeycloakOpenAPI.Admin.Tags.CLIENT_INITIAL_ACCESS) | |||
| @Operation( summary = "Create a new initial access token.") | |||
| @APIResponse(responseCode = "201", description = "Created", content = @Content(schema = @Schema(implementation = ClientInitialAccessCreatePresentation.class))) | |||
| public Object create(ClientInitialAccessCreatePresentation config) { | |||
| public Response create(ClientInitialAccessCreatePresentation config) { | |||
There was a problem hiding this comment.
@michalvavrik Just curious, if the create method only returns Response type, why should the return type of the method still be Object ?
There was a problem hiding this comment.
@michalvavrik Just curious, if the
createmethod only returnsResponsetype, why should the return type of the method still beObject?
It limits changes to the necessary ones, which makes backports easier. Refactoring to Admin API v1 is not important as we are working on v2. You don't need to follow my suggestion, if you disagree, keep it.
e17ce15 to
a4eb542
Compare
The POST /clients-initial-access endpoint was returning 200 OK instead of 201 Created. The server-side create() method has been updated to return a proper JAX-RS Response with status 201 and a Location header pointing to the created resource. A doCreate() method is added to the ClientInitialAccessResource Java client interface returning the raw JAX-RS Response, allowing callers to access HTTP-level details such as the status code and Location header that the existing create() method hides. A test is added using doCreate() to verify the 201 status and Location header without modifying the existing typed create() interface. Closes keycloak#49185 Signed-off-by: Vinit Kumar <30852363+ThreeMangoTrees@users.noreply.github.com>
Signed-off-by: Vinit Kumar <30852363+ThreeMangoTrees@users.noreply.github.com>
945492e to
6ba3b3c
Compare
There was a problem hiding this comment.
Almost there, thanks for your patience.
| @Produces(MediaType.APPLICATION_JSON) | ||
| ClientInitialAccessPresentation create(ClientInitialAccessCreatePresentation rep); | ||
|
|
There was a problem hiding this comment.
Let's drop this change, it is just a noise.
| assertNotNull(entity.getToken()); | ||
| assertThat(location, endsWith("/clients-initial-access/" + id)); | ||
| } | ||
| resource.delete(id); |
There was a problem hiding this comment.
I am not sure about this pattern. I thought this should be done roughly like id=null and here if (id != null) { resource.delete(id); } because for example if assertThat(location, endsWith("/clients-initial-access/" + id)); failed while the entity was created, this test would still have side-effects.
There was a problem hiding this comment.
Makes sense. I edited the change.
…onse javadocs - Revert whitespace-only change in admin-client ClientInitialAccessResource - Fix testCreateReturns201WithLocationHeader to null-initialize id and delete in a finally block so cleanup always runs even if an assertion fails - Add javadocs to HttpResponse.getStatus/setStatus warning that the value is overwritten by JAX-RS when a plain object (not Response) is returned Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Vinit Kumar <30852363+ThreeMangoTrees@users.noreply.github.com>
There was a problem hiding this comment.
services/src/main/java/org/keycloak/services/resources/admin/ClientInitialAccessResource.java and the test changes as solution for the one issue we know of LGTM.
I am not sure about javadoc and the javadoc being solution for a general bug that is under this HttpResponse status functionality. I'd not document something referring to 200 unless I have a test that:
- the javadoc regarding 200 is still true in the future as Quarkus side can change
- I had tests for every other relevant status code so that I know it really is 200/201 only bug
- I had upstream bug tracker
but I am with this PR based on the 2 files which are fixing #49185 and @shawkins will review the rest (like javadoc he requested 😄 )
Co-authored-by: Steven Hawkins <shawkins@redhat.com> Signed-off-by: Vinit Kumar <30852363+ThreeMangoTrees@users.noreply.github.com>
Co-authored-by: Steven Hawkins <shawkins@redhat.com> Signed-off-by: Vinit Kumar <30852363+ThreeMangoTrees@users.noreply.github.com>
@shawkins waiting for your approval. |
There was a problem hiding this comment.
@shawkins waiting for your approval.
Separated off the deprecation task #49374 so that this PR is more suitable for backport if needed.
Thanks @ThreeMangoTrees
…200 instead of 201 (keycloak#49232) * Fix clients-initial-access returning 200 instead of 201 The POST /clients-initial-access endpoint was returning 200 OK instead of 201 Created. The server-side create() method has been updated to return a proper JAX-RS Response with status 201 and a Location header pointing to the created resource. A doCreate() method is added to the ClientInitialAccessResource Java client interface returning the raw JAX-RS Response, allowing callers to access HTTP-level details such as the status code and Location header that the existing create() method hides. A test is added using doCreate() to verify the 201 status and Location header without modifying the existing typed create() interface. Closes keycloak#49185 Signed-off-by: Vinit Kumar <30852363+ThreeMangoTrees@users.noreply.github.com> * Removed doCreate method and corresponding test references Signed-off-by: Vinit Kumar <30852363+ThreeMangoTrees@users.noreply.github.com> * Address reviewer feedback: drop noise, fix test cleanup, add HttpResponse javadocs - Revert whitespace-only change in admin-client ClientInitialAccessResource - Fix testCreateReturns201WithLocationHeader to null-initialize id and delete in a finally block so cleanup always runs even if an assertion fails - Add javadocs to HttpResponse.getStatus/setStatus warning that the value is overwritten by JAX-RS when a plain object (not Response) is returned Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Vinit Kumar <30852363+ThreeMangoTrees@users.noreply.github.com> * Update server-spi/src/main/java/org/keycloak/http/HttpResponse.java Co-authored-by: Steven Hawkins <shawkins@redhat.com> Signed-off-by: Vinit Kumar <30852363+ThreeMangoTrees@users.noreply.github.com> * Update server-spi/src/main/java/org/keycloak/http/HttpResponse.java Co-authored-by: Steven Hawkins <shawkins@redhat.com> Signed-off-by: Vinit Kumar <30852363+ThreeMangoTrees@users.noreply.github.com> --------- Signed-off-by: Vinit Kumar <30852363+ThreeMangoTrees@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Steven Hawkins <shawkins@redhat.com>
Summary
POST /admin/realms/{realm}/clients-initial-accesswas returning HTTP 200 instead of 201 as required by the specHttpResponse.setStatus()is silently overwritten by the JAX-RS runtime whenever a resource method returns a plain object — only aResponsereturn type carries the status code throughClientInitialAccessResource.create()to returnResponse.status(201).entity(rep).location(uri).build()using the proper JAX-RS patternClientInitialAccessResource.create()return type fromClientInitialAccessPresentationtoResponseso callers can inspect the HTTP status andLocationheaderreadEntity(ClientInitialAccessPresentation.class)for correct client-side deserializationChanges
services/src/main/java/org/keycloak/services/resources/admin/ClientInitialAccessResource.java— core fixintegration/admin-client/src/main/java/org/keycloak/admin/client/resource/ClientInitialAccessResource.java— return type changetests/base/src/test/java/org/keycloak/tests/admin/InitialAccessTokenResourceTest.java— assert 201 and Location headertestsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/InitialAccessTokenTest.java— usereadEntity()readEntity()after the interface changeWhy Local Reproduction Was Not Possible
The fix for issue #49185 (POST /admin/realms/{realm}/clients-initial-access returning HTTP 200
instead of 201) could not be verified locally by starting a patched Keycloak server. There were
three compounding reasons.
Pre-existing compilation errors in the main branch
Three modules in the current main branch fail to compile. These failures are unrelated to the
fix for #49185 and were introduced by other contributors in the days preceding this fix.
model/infinispan: JGroupsConfigurator.java fails because JpaConnectionProvider does not
satisfy a generic type bound during compilation. Introduced by commit 23bac7b on May 20.
model/storage-services: UserStorageProviderResource.java has a similar type resolution
failure where LDAPStorageMapper cannot be verified to satisfy , and
RealmModel cannot be found by the compiler.
rest/admin-v2: Multiple files reference symbols (AdminAuth, AdminEventBuilder,
AdminPermissionEvaluator) that no longer exist in the packages they expect.
Because these modules are in the transitive dependency chain of quarkus/runtime and
quarkus/dist (the modules that produce the runnable server), a full build of Keycloak
cannot complete.
The only available distribution is from April 22 and is stale
Rather than building from scratch, patching the pre-built distribution at
quarkus/dist/target/keycloak-999.0.0-SNAPSHOT/was attempted. However, that distribution isapproximately four weeks old. Many classes were added to the source since April 22 that are
absent from its JARs. The first missing class encountered at server startup was:
org.keycloak.representations.idm.oid4vc.UserVerifiableCredentialRepresentationThis caused a
ClassNotFoundExceptionthat prevented the server from starting.Patching individual class files into the Quarkus uber-JAR causes classloader conflicts
Quarkus bundles all application classes into a single uber-JAR at lib/app/keycloak.jar.
The Quarkus augmentation/deployment code (run by kc.sh build) was compiled against the
April 22 versions of all Keycloak classes and baked into the distribution.
When newer class files were injected into
keycloak.jarto resolve theClassNotFoundException,the old deployment code and the new application code held conflicting versions of the same
class (e.g.
org.keycloak.representations.userprofile.config.UPConfig). This caused aLinkageError during kc.sh build:
loader constraint violation: loader QuarkusClassLoader wants to load class UPConfig.
A different class with the same name was previously loaded by URLClassLoader.
There is no clean way to partially patch an existing Quarkus distribution — the entire
distribution must be rebuilt as a coherent unit from the same source snapshot.
Test plan
InitialAccessTokenResourceTest.testInitialAccessTokens()asserts HTTP 201 and a non-nullLocationheaderInitialAccessTokenResourceTest.testPeriodicExpiration()usesreadEntity()correctlyInitialAccessTokenTest(Arquillian) updated to usereadEntity()Fixes #49185