Supporting OID4VCI AuthZCode flow: by bucchi · Pull Request #29685 · keycloak/keycloak

bucchi · 2024-05-20T06:51:34Z

#17616 (comment)
As discussion above, commit test code for "Step1:without scope parameter mode" .

closes #29724

Step1 for without scope parameter mode Fixes keycloak#23628 Signed-off-by: Yutaka Obuchi <yutaka.obuchi.sd@hitachi.com>

tnorimat · 2024-05-21T01:22:27Z

@bucchi I created a sub issue for the main issue.

Could you modify your commit log from

Fixes #23628

to

Closes #29724

?

tnorimat

@bucchi Thank you. I reviewed the PR and I added some review comments. Could you check them ?

Also, when you finally squash commits after the PR is approved, could you fix the PR's squashed commit message as I mentioned in the previous comment of the PR?

tnorimat · 2024-05-21T07:21:18Z

+        Response discoveryResponse = oid4vciDiscoveryTarget.request().get();
+        CredentialIssuer oid4vciIssuerConfig = JsonSerialization.readValue(discoveryResponse.readEntity(String.class), CredentialIssuer.class);
+
+        assertEquals(200, discoveryResponse.getStatus());


IMO, it might be good if this assertion be just after L458 (line 458).

tnorimat · 2024-05-21T07:23:46Z

+        CredentialIssuer oid4vciIssuerConfig = JsonSerialization.readValue(discoveryResponse.readEntity(String.class), CredentialIssuer.class);
+
+        assertEquals(200, discoveryResponse.getStatus());
+        assertEquals("https://localhost:8543/auth/realms/test", oid4vciIssuerConfig.getCredentialIssuer());


If possible, could you avoid hard-coded https://localhost:8543/auth/realms/test ?

tnorimat · 2024-05-21T07:25:15Z

+
+        assertEquals(200, discoveryResponse.getStatus());
+        assertEquals("https://localhost:8543/auth/realms/test", oid4vciIssuerConfig.getCredentialIssuer());
+        assertEquals("https://localhost:8543/auth/realms/test/protocol/oid4vc/credential", oid4vciIssuerConfig.getCredentialEndpoint());


If possible, could you avoid hard-coded https://localhost:8543/auth/realms/test/protocol/oid4vc/credential ?

tnorimat · 2024-05-21T07:27:55Z

+        request.setFormat(oid4vciIssuerConfig.getCredentialsSupported().get("test-credential").getFormat());
+        request.setCredentialIdentifier(oid4vciIssuerConfig.getCredentialsSupported().get("test-credential").getId());
+
+        assertEquals("jwt_vc", oid4vciIssuerConfig.getCredentialsSupported().get("test-credential").getFormat().toString());


"jwt_vc" can be Format.JWT_VC.toString() .

tnorimat · 2024-05-21T07:30:18Z

+        Response response = credentialTarget.request().header(HttpHeaders.AUTHORIZATION, "bearer " + token).post(Entity.json(request));        
+        CredentialResponse credentialResponse = JsonSerialization.readValue(response.readEntity(String.class),CredentialResponse.class);
+
+        assertEquals(200, response.getStatus());


IMO, it might be good if this assertion be just after L478 (line 478).

tnorimat · 2024-05-21T07:41:34Z

+        assertEquals("https://localhost:8543/auth/realms/test/protocol/oid4vc/credential", oid4vciIssuerConfig.getCredentialEndpoint());
+
+        // 4. With the access token, get the credential
+        Client clientForCredentialRequest = AdminClientUtil.createResteasyClient();


The same as L454 (line 454).

tnorimat · 2024-05-21T07:52:49Z

+        String token = getBearerToken(oauth.openid(false).scope(null));
+
+        // 3. Get the credential configuration id from issuer metadata at .wellKnown
+        Client client = AdminClientUtil.createResteasyClient();


It might be better to use try-finally clause for this Client clientForCredentialRequest.
For example, see

keycloak/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/OIDCWellKnownProviderTest.java

Lines 230 to 245 in 8d1956e

@Test

public void testHttpDiscovery() {

Client client = AdminClientUtil.createResteasyClient();

try {

OIDCConfigurationRepresentation oidcConfig = getOIDCDiscoveryRepresentation(client, "http://localhost:8180/auth");

Assert.assertNotNull(oidcConfig.getJwksUri());

// Token Revocation

Assert.assertNotNull(oidcConfig.getRevocationEndpoint());

Assert.assertNotNull(oidcConfig.getRevocationEndpointAuthMethodsSupported());

Assert.assertNotNull(oidcConfig.getRevocationEndpointAuthSigningAlgValuesSupported());

} finally {

client.close();

}

}

Or, maybe, it can be re-written as follows:

Response discoveryResponse; CredentialIssuer oid4vciIssuerConfig; try (Client client = AdminClientUtil.createResteasyClient()) { UriBuilder builder = UriBuilder.fromUri(OAuthClient.AUTH_SERVER_ROOT); URI oid4vciDiscoveryUri = RealmsResource.wellKnownProviderUrl(builder).build("test", OID4VCIssuerWellKnownProviderFactory.PROVIDER_ID); WebTarget oid4vciDiscoveryTarget = client.target(oid4vciDiscoveryUri); discoveryResponse = oid4vciDiscoveryTarget.request().get(); oid4vciIssuerConfig = JsonSerialization.readValue(discoveryResponse.readEntity(String.class), CredentialIssuer.class); }

tnorimat · 2024-05-21T07:53:48Z

+        assertEquals("jwt_vc", oid4vciIssuerConfig.getCredentialsSupported().get("test-credential").getFormat().toString());
+        assertEquals("test-credential", oid4vciIssuerConfig.getCredentialsSupported().get("test-credential").getId());
+
+        Response response = credentialTarget.request().header(HttpHeaders.AUTHORIZATION, "bearer " + token).post(Entity.json(request));        


The same as L454 (line 454).

francis-pouatcha

@bucchi all my review comment match those of @tnorimat . I sent a pull request Hitachi#707 to address those issues.

…view-2067746577 Signed-off-by: Francis Pouatcha <francis.pouatcha@adorsys.com>

tnorimat

LTGM.

tnorimat · 2024-05-28T00:09:19Z

@mposolda I approved the PR. Could you check it?

tnorimat · 2024-05-28T08:17:42Z

The PR closes #29724

mposolda

@bucchi @tnorimat @francis-pouatcha Thanks for the fix and reviews!

mposolda

@bucchi @tnorimat @francis-pouatcha Thanks for the PR and reviews!

tnorimat · 2024-05-28T11:07:10Z

@mposolda Thank you!

Test code commit for supporting OID4VCI AuthZCode flow:

21fcd4f

Step1 for without scope parameter mode Fixes keycloak#23628 Signed-off-by: Yutaka Obuchi <yutaka.obuchi.sd@hitachi.com>

bucchi requested review from a team as code owners May 20, 2024 06:51

keycloak-github-bot Bot added team/cloud-native team/core-clients team/core-iam labels May 20, 2024

tnorimat suggested changes May 21, 2024

View reviewed changes

mposolda assigned mposolda and tnorimat May 22, 2024

francis-pouatcha suggested changes May 25, 2024

View reviewed changes

Fix: Improvements from review discussion keycloak#29685#pullrequestre…

f540371

…view-2067746577 Signed-off-by: Francis Pouatcha <francis.pouatcha@adorsys.com>

tnorimat approved these changes May 28, 2024

View reviewed changes

mposolda approved these changes May 28, 2024

View reviewed changes

mposolda merged commit 68d9dce into keycloak:main May 28, 2024

tnorimat deleted the 26328-OID4VCIAuthZCodeFlow-without-scope branch May 21, 2025 05:10

	@Test
	public void testHttpDiscovery() {
	Client client = AdminClientUtil.createResteasyClient();
	try {
	OIDCConfigurationRepresentation oidcConfig = getOIDCDiscoveryRepresentation(client, "http://localhost:8180/auth");

	Assert.assertNotNull(oidcConfig.getJwksUri());

	// Token Revocation
	Assert.assertNotNull(oidcConfig.getRevocationEndpoint());
	Assert.assertNotNull(oidcConfig.getRevocationEndpointAuthMethodsSupported());
	Assert.assertNotNull(oidcConfig.getRevocationEndpointAuthSigningAlgValuesSupported());
	} finally {
	client.close();
	}
	}

Uh oh!

Conversation

bucchi commented May 20, 2024 • edited by mposolda Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnorimat commented May 21, 2024

Uh oh!

tnorimat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

francis-pouatcha left a comment

Choose a reason for hiding this comment

Uh oh!

tnorimat left a comment

Choose a reason for hiding this comment

Uh oh!

tnorimat commented May 28, 2024

Uh oh!

tnorimat commented May 28, 2024

Uh oh!

mposolda left a comment

Choose a reason for hiding this comment

Uh oh!

mposolda left a comment

Choose a reason for hiding this comment

Uh oh!

tnorimat commented May 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bucchi commented May 20, 2024 •

edited by mposolda

Loading