search

a-sit-plus / vck

Kotlin Multiplatform library implementing W3C VC Data Model and ISO 18013-5 mDL
https://a-sit-plus.github.io/vck/
Apache License 2.0
29 stars 2 forks source link

Extend RequestParsing to SignatureRequests #143

Closed n0900 closed 1 month ago

n0900 commented 1 month ago

SignatureRequest can be passed by reference just like AuthenticationRequest. Here we extend AuthenticationRequestParser to also handle requests by reference for signatures sent by the driving application/terminal service. For consistency we rename the class to RequestParser and introduce the SignatureRequestFrom data classes.

nodh commented 1 month ago

It's hard to review this, since that other one #127 is still a draft... But to me the SignatureRequestParameters look a lot like AuthenticationRequestParameters, so why split it up? Since the latter one still has numSignatures and other stuff in it?!

n0900 commented 1 month ago

It's hard to review this, since that other one #127 is still a draft...

Admittedly the draft became quite unwieldy but it is not yet at a point where merging makes sense so I planned on splitting new changes into seperate merge requests and the draft in the end is the full feature implementation. What do you think of this?

But to me the SignatureRequestParameters look a lot like AuthenticationRequestParameters, so why split it up? Since the latter one still has numSignatures and other stuff in it?!

Since they do not have a lot of fields in common I think it makes sense to keep the two seperate. We also talked about seperating some other classes at a previous time and I thought it would be good to not add more to these. So I introduced the interface RequestParameter

We could realistically combine AuthenticationRequestParameterFrom and SignatureRequestParameterFrom to simply be RequestparameterFrom however this has a long rat tail in OidcSiopWallet and beyond which was rather unpleasant to work with so I decided against it. It you want I could also implement a version like this.

n0900 commented 1 month ago

But to me the SignatureRequestParameters look a lot like AuthenticationRequestParameters, so why split it up? Since the latter one still has numSignatures and other stuff in it?!

On second look I agree that they look related and I have moved the vals they have in common to the interface. However CSC actually extends AuthenticationRequestParameters as well and as such uses both in different contexts. This in turn means that combining them is very messy from a readability perspective.

n0900 commented 1 month ago

On a different note: In AuthenticationRequestParameters it says that clientID is required, but it is nullable. When making it not nullable tests fail.

nodh commented 1 month ago

But to me the SignatureRequestParameters look a lot like AuthenticationRequestParameters, so why split it up? Since the latter one still has numSignatures and other stuff in it?!

On second look I agree that they look related and I have moved the vals they have in common to the interface. However CSC actually extends AuthenticationRequestParameters as well and as such uses both in different contexts. This in turn means that combining them is very messy from a readability perspective.

Yeah, now I get it ... it is really not a good idea to combine them

nodh commented 1 month ago

It looks to me that the parsed SignatureRequestParametersFrom are never used anywhere?

n0900 commented 1 month ago

It looks to me that the parsed SignatureRequestParametersFrom are never used anywhere?

This is true. It is only used as a container for SignatureRequestParameters in order to use RequestParser as originally written - the 'From' part is currently ignored. The class will be used when parsing requests which are sent by a driving application. Tests coming up