Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

JWT VC Issuer Metadata /.well-known/jwt-vc-issuer to comply with SD-JWT VC Specification #29635

Merged
merged 8 commits into from
May 28, 2024
Merged

JWT VC Issuer Metadata /.well-known/jwt-vc-issuer to comply with SD-JWT VC Specification #29635

merged 8 commits into from
May 28, 2024

Conversation

francis-pouatcha
Copy link
Contributor

closes #29634

@francis-pouatcha
Exposing JWT VC Issuer Metadata /.well-known/jwt-vc-issuer to comply …
111038e 
…with SD-JWT VC Specification

Signed-off-by: Francis Pouatcha <francis.pouatcha@adorsys.com>
@francis-pouatcha francis-pouatcha requested a review from a team as a code owner May 17, 2024 07:40
wistefan
wistefan approved these changes May 17, 2024
View reviewed changes
Copy link
Contributor

@wistefan wistefan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@keycloak-github-bot keycloak-github-bot bot mentioned this pull request May 17, 2024
Open
@ahus1
Copy link
Contributor

ahus1 commented May 17, 2024

@mposolda - I assume this is in the area of core-clients to approve and review.

@keycloak-github-bot
Copy link

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.federation.ldap.LDAPUserLoginTest#loginLDAPUserCredentialVaultAuthenticationNoneEncryptionNone

Keycloak CI - Java Distribution IT (windows-latest - temurin - 19)

org.openqa.selenium.TimeoutException: 
java.net.SocketTimeoutException: Read timed out
Build info: version: '3.14.0', revision: 'aacccce0', time: '2018-08-02T20:19:58.91Z'
System info: host: 'fv-az1258-893', ip: '10.1.0.29', os.name: 'Windows Server 2022', os.arch: 'amd64', os.version: '10.0', java.version: '19.0.2'
Driver info: driver.version: HtmlUnitDriver
...

Report flaky test

keycloak-github-bot[bot]
keycloak-github-bot bot reviewed May 20, 2024
View reviewed changes
Copy link

@keycloak-github-bot keycloak-github-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreported flaky test detected, please review

mposolda
mposolda reviewed May 22, 2024
View reviewed changes
Copy link
Contributor

@mposolda mposolda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@francis-pouatcha @wistefan Thanks for adding this and for the review!

Added comment inline regarding to avoid code duplication.

Is it also possible to add automated tests for this?

services/src/main/java/org/keycloak/protocol/oid4vc/issuance/JWTVCIssuerWellKnownProvider.java Outdated Show resolved Hide resolved
@mposolda mposolda added the missing/tests Tests are missing label May 22, 2024
@francis-pouatcha
Merge branch 'main' into issue-29634
7179277
@francis-pouatcha
Refactor: Extract common key transformation logic to reduce duplication.
4b1bfd7 
Addresses review comment in PR #29635: #29635

Signed-off-by: Francis Pouatcha <francis.pouatcha@adorsys.com>
@francis-pouatcha
@francis-pouatcha @wistefan Added automated test covering the new fun…
9cc28b3 
…ctionality and verifying that the correct issuer and JWK set are returned.

This addresses the request for tests in the review discussion: #29635

Signed-off-by: Francis Pouatcha <francis.pouatcha@adorsys.com>
@francis-pouatcha francis-pouatcha requested review from a team as code owners May 24, 2024 15:48
This was referenced May 24, 2024
Open
Open
keycloak-github-bot[bot]
keycloak-github-bot bot reviewed May 24, 2024
View reviewed changes
Copy link

@keycloak-github-bot keycloak-github-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unreported flaky test detected, please review

@keycloak-github-bot
Copy link

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginFailedWithIntermediateRevocationListFromHttp

Keycloak CI - FIPS IT (strict)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:307)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginFailedWithInvalidSignatureCRL

Keycloak CI - FIPS IT (strict)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:307)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

org.keycloak.testsuite.x509.X509BrowserCRLTest#loginWithMultipleRevocationLists

Keycloak CI - FIPS IT (strict)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:307)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
...

Report flaky test

@francis-pouatcha
Copy link
Contributor Author

@mposolda not sure how to proceed here. Will wait for your review comments.

mposolda
mposolda reviewed May 27, 2024
View reviewed changes
Copy link
Contributor

@mposolda mposolda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@francis-pouatcha Thanks for the updates!

The test is actually failing. It probably works with Keycloak on embedded undertow (so I guess it works for you when you run the test from your IDE), but it fails with quarkus distribution, which can be simulated probably by command like this:

mvn clean install
cd testsuite/integration-arquillian
mvn clean install -Pauth-server-quarkus -Dtest=JWTVCIssuerWellKnownProviderTest

I believe it will help to slightly update the test and test by actually sending real HTTP request instead of using "testing client" to directly call Java API. That's how OIDC well-known endpoint is also tested. Added inline comment with some details.

...st/java/org/keycloak/testsuite/oid4vc/issuance/signing/JWTVCIssuerWellKnownProviderTest.java Outdated Show resolved Hide resolved
@ahus1 ahus1 requested a review from mposolda May 28, 2024 07:04
@ahus1
Copy link
Contributor

ahus1 commented May 28, 2024

@mposolda - tests are now green.

mposolda
mposolda approved these changes May 28, 2024
View reviewed changes
Copy link
Contributor

@mposolda mposolda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@francis-pouatcha @wistefan @ahus1 Thanks everyone for the work on this PR and reviews

@mposolda mposolda merged commit 4317a47 into keycloak:main May 28, 2024
63 checks passed
@mposolda mposolda removed the missing/tests Tests are missing label May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keycloak-github-bot keycloak-github-bot[bot] keycloak-github-bot left review comments

@mposolda mposolda mposolda approved these changes

@wistefan wistefan wistefan approved these changes

Assignees

@mposolda mposolda

Labels
flaky-test team/cloud-native team/core-clients team/core-iam
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Expose JWT VC Issuer Metadata /.well-known/jwt-vc-issuer to comply with SD-JWT VC Specification
5 participants
@francis-pouatcha @ahus1 @mposolda @wistefan @bengo237