Repository navigation
Conversation
Part of: keycloak#48201 Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com>
This reverts commit 32b7025.
for verifiable credentials Verifiable credentials now capture and store a snapshot of user attributes at creation time Administrators can refresh the snapshot via an update endpoint which also increments the credential's revision identifier. Closes keycloak#48578 Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com>
…butes-and-updating-them
There was a problem hiding this comment.
@jimmychakkalakal Thanks, nice! I've added some review comments inline.
| throw new BadRequestException("Credential scope not specified"); | ||
| } | ||
|
|
||
| UserModel user = getUserById(session.getContext().getRealm(), userId); |
There was a problem hiding this comment.
Any specific reason for this check here? Just wondering that other similar methods (EG. addConsent) don't have this and maybe it has some performance implications?
There can be a corner-case when user is deleted in the meantime (EG. admin trying to add verifiable credential, but user removed by some "other admin" in the meantime), but I think that if this happens , the em.getReference(UserEntity.class, userId) will return null and it will throw the exception, which would be re-thrown as ModelException (by JpaExceptionConverter). So maybe this check is redundant?
| em.refresh(userEntity); | ||
| UserModel user = new UserAdapter(session, session.getContext().getRealm(), em, userEntity); | ||
|
|
||
| TypedQuery<UserVerifiableCredentialEntity> query = em.createNamedQuery("verifiableCredentialsByUser", UserVerifiableCredentialEntity.class); |
There was a problem hiding this comment.
Nitpick: Can re-use method getVerifiableCredentialsEntitiesByUser instead of manually creating query again?
| ~ * See the License for the specific language governing permissions and | ||
| ~ * limitations under the License. | ||
| --> | ||
| <databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.1.xsd"> |
There was a problem hiding this comment.
Instead of introducing new file jpa-changelog-26.8.0.xml, it will be good to instead update existing file jpa-changelog-26.7.0.xml and just directly add new column inside the creation of USER_VER_CREDENTIAL .
This is OK as 26.7.0 was not released yet (it is planned to be released around end of June) and we don't need to preserve any migration/compatibility support for nightly releases.
| rep.setCredentialScopeName(model.getCredentialScopeName()); | ||
| rep.setRevision(model.getRevision()); | ||
| rep.setCreatedDate(model.getCreatedDate()); | ||
| rep.setUserAttributes(model.getUserAttributes()); |
There was a problem hiding this comment.
I think similar change might be needed in the RepresentationToModel class as well. Is it also possible to slightly test this in OID4VCExportImportTest and make sure that export/import works for this new column as expected? (Probably no need to introduce new method in that test class, but sufficient to just
8000
re-use existing methods and test export/import of "attributes" works as expected).
There was a problem hiding this comment.
Thanks for the comment, I have extended RepresentationToModel for User attributes and modified test in OID4VCExportImportTest to test import and export of user attributes
There was a problem hiding this comment.
@jimmychakkalakal nice work thanks! Added just few inline comments.
| if (userEntity == null || !session.getContext().getRealm().getId().equals(userEntity.getRealmId())) { | ||
| throw new ModelException("User not found: " + userId); | ||
| } | ||
| em.refresh(userEntity); |
There was a problem hiding this comment.
Not sure if the refresh is needed
| } | ||
|
|
||
| String newRevision = SecretGenerator.getInstance().generateSecureID(); | ||
| System.out.println("New Revision" + newRevision); |
There was a problem hiding this comment.
better to remote the console output
| @Path("credentials/{credentialScopeName}") | ||
| @Produces(MediaType.APPLICATION_JSON) | ||
| UserVerifiableCredentialRepresentation updateCredential(@PathParam("credentialScopeName") String credentialScopeName); |
There was a problem hiding this comment.
I’m not sure if it would be better to use a different name for this API and keep the PUT on /credentials for generic updates since we’ll probably need it to update other things like the revision. @mposolda WDYT?
There was a problem hiding this comment.
For me, the updateCredential works. Maybe there is better name, but don't have better ideas.
For example: Is refreshCredential better name or is it rather more confusing? To me, it is rather more confusing (as it can imply it has something to do with "refresh tokens" and "refreshing verifiable credentials" as described in the OID4VCI specs, which is not the case)
Another name like regenerateCredential also might be confusing IMO...
So my vote is to likely keep as it is.
Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com>
…butes-and-updating-them
| TypedQuery<UserVerifiableCredentialEntity> query = getVerifiableCredentialsEntitiesByUser(); | ||
| query.setParameter("userId", userId); | ||
|
|
||
| UserVerifiableCredentialEntity entity = query.getResultStream() |
There was a problem hiding this comment.
It seems to me that this could be simplified further to call just:
UserVerifiableCredentialEntity entity = getVerifiableCredentialsEntitiesByUser(userId)
without a need to manually create query above. However that is a nitpick, which can be possibly addressed in a follow-up. Not a blocker for this PR IMO.
| List<UserVerifiableCredentialRepresentation> importedVerifiableCreds = testRealm.admin().users().get(userAfterImport.getId()).verifiableCredentials().getCredentials(); | ||
|
|
||
| // Verify same number of credentials | ||
| assertEquals(verifiableCreds.size(), importedVerifiableCreds.size()); |
There was a problem hiding this comment.
Could not this be simplified to just something like:
assertEquals(verifiableCreds, importedVerifiableCreds);
and omit the code below (since the equality check would check attributes as well per my understanding as attributes are part of the "equals" check on the UserVerifiableCredentialRepresentation class.
My point is that previously also fields like "revision" were checked, but now they are not.
But this is also nitpick, which can be possibly handled as a follow-up.
Closes keycloak#48578 Signed-off-by: Jimmy Chakkalakal <jimmy.chakkalakal@ibm.com>
Add user attribute snapshot support
for verifiable credentials
Verifiable credentials now capture
and store a snapshot of user
attributes at creation time
Administrators can refresh the
snapshot via an update endpoint
which also increments the
credential's revision identifier.
Closes #48578