[OID4VCI]: Enforce ownership validation when deleting issued verifiable credentials via the account API - #51418
Conversation
There was a problem hiding this comment.
Pull request overview
Scopes issued credential deletion to the authenticated owner across local, federated, and cached storage paths.
Changes:
- Adds user-scoped removal APIs and provider implementations.
- Updates the account DELETE endpoint to return 404 for non-owned credentials.
- Adds ownership and role authorization tests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/base/src/test/java/org/keycloak/tests/admin/user/FederatedIssuedVerifiableCredentialTest.java |
Tests federated ownership enforcement. |
tests/base/src/test/java/org/keycloak/tests/account/AccountRestServiceRolesTest.java |
Tests account API deletion authorization. |
services/src/main/java/org/keycloak/services/resources/account/AccountIssuedVerifiableCredentialResource.java |
Uses owner-scoped deletion. |
server-spi/src/main/java/org/keycloak/models/UserProvider.java |
Adds the scoped removal SPI. |
model/storage/src/main/java/org/keycloak/storage/federated/UserVerifiableCredentialFederatedStorage.java |
Adds scoped federated removal. |
model/storage-private/src/main/java/org/keycloak/storage/UserStorageManager.java |
Routes removal by storage type. |
model/jpa/src/main/java/org/keycloak/storage/jpa/JpaUserFederatedStorageProvider.java |
Implements federated ownership checks. |
model/jpa/src/main/java/org/keycloak/models/jpa/JpaUserProvider.java |
Implements locked local removal. |
model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/UserCacheSession.java |
Delegates scoped removal through caching. |
- Add user-scoped removeIssuedVerifiableCredential(userId, credentialId) to UserProvider SPI and federated storage interface - Implement ownership-checked removal in JpaUserProvider and JpaUserFederatedStorageProvider - Route scoped removal through UserStorageManager and UserCacheSession - Restrict AccountIssuedVerifiableCredentialResource deletion to the authenticated user - Add integration tests for cross-user, owner, and missing-role deletion scenarios Closes keycloak#50368 Signed-off-by: Awambeng Rodrick <awambengrodrick@gmail.com>
Signed-off-by: Awambeng Rodrick <awambengrodrick@gmail.com>
jimmychakkalakal
left a comment
There was a problem hiding this comment.
Reviewed the ownership enforcement fix across all storage layers. The approach is correct — atomic check-then-delete with PESSIMISTIC_WRITE, consistent routing via StorageId.isLocalStorage, and no information leakage (404 for both "not found" and "not owned"). Test coverage is solid.
Two observations for follow-up (not blocking):
- The admin API endpoint
UserVerifiableCredentialResource.revokeIssuedCredentialhas the same unscoped deletion pattern — an admin authorized for User A could delete User B's credential by ID. Lower severity but same class of vulnerability. - The old single-arg
removeIssuedVerifiableCredential(String)in the SPI could benefit from a@Deprecatedannotation or Javadoc warning to prevent future user-facing code from accidentally using the unscoped version.
jimmychakkalakal
left a comment
There was a problem hiding this comment.
Thanks for the fix. Good fix with test coverage. I approve the PR.
|
@VinodAnandan - please don't share people's holidays in public forums, this is avoidable OSINT. When there is a need to share any urgency, share it in a private channel. Once Marek is back, agree on a stand-in. Looking at the test cases, there are IMHO things that would be worth reviewing in follow-up steps:
|
This PR fixes an object-level authorization gap in the account REST API where the delete endpoint for issued verifiable credentials performed a global primary-key lookup and removal without verifying ownership of the credential.
Key changes:
Closes #50368