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.

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

feature/SPRIND-84 #255

Merged
merged 42 commits into from
Nov 4, 2024
Merged

feature/SPRIND-84 #255

merged 42 commits into from
Nov 4, 2024

Conversation

zoemaas
Copy link
Contributor

@zoemaas zoemaas commented Oct 18, 2024

No description provided.

@zoemaas zoemaas self-assigned this Oct 18, 2024
}

private async registerCryptoServiceCallback(platformCallback: RegisterCryptoServiceCallbackArgs): Promise<RegisterCryptoServiceCallbackResult> {
CryptoServiceJS.register(platformCallback);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not expose this function in the SDK. We have a jwt-service that should be used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the function from the SDK. As far as I can understand, the default service should be used. Is that correct? Does it make sense to keep the verifyJwt() function exposed?

Copy link
Contributor

Choose a reason for hiding this comment

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

We have a plugin that does JWTs/JWS. So that should be leveraged to sign JWTs, as it has integrated support for all the identifier types we support (X509, DIDs, JWKs, kids etc).

The verify function is also exposed in the OIDF client. Depending on whether that function is doing more than a regular JWT check it makes sense to delegate it to the OIDF client. IF not then it could also use the JWT exposed CompactJwt/Jwe verify function from the SDK.

What would be nice if you have the option to have a callback. But then it should not be exposed as a method, but as a constructor argument to the plugin. Then you could do something like:

If callback is set on the plugin, delegate to the respective function of the callback. If not then delegate to the JWT service of the SDK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the verifyJwt() and signJwt functions using @sphereon/ssi-sdk-ext.jwt-service. Still cannot make it work properly because the CryptoService.register(...) function from the OIDF client is ignoring the input. I'm checking it with @jcmelati

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the verifyJwt() and signJwt() functions using @sphereon/ssi-sdk-ext.jwt-service. Still cannot make it work properly because the second parameter of the verifyJwt function (context). I get a compilation error which I'm not sure how to fix.

@zoemaas
Copy link
Contributor Author

zoemaas commented Oct 23, 2024

At the moment the underlying library does not provide an implementation of a default callback, so I've created one for testing and I'm passing it to the constructor of the plugin

@zoemaas zoemaas marked this pull request as ready for review October 23, 2024 14:46
packages/oidf-client/package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@sanderPostma sanderPostma left a comment

Choose a reason for hiding this comment

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

LGTM

@sanderPostma sanderPostma merged commit e02b30b into develop Nov 4, 2024
2 checks passed
@BtencateSphereon BtencateSphereon deleted the feature/SPRIND-84 branch November 5, 2024 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants