Skip to content

Support request byValue and byReference and presentationDefinition #57

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

wistefan
Copy link
Collaborator

@wistefan wistefan commented May 2, 2025

Requires CredentialConfigService > 3.x

@wistefan wistefan added the major Should be applied for all breaking changes. label May 2, 2025
@wistefan wistefan requested a review from pulledtim May 2, 2025 14:40
@@ -165,6 +208,11 @@ In order to ease the integration into frontends, VCVerifier offers a login-page

In order to start a ```same-device```-flow(e.g. the credential is hold by the requestor, instead of an additional device like a mobile wallet) call:
```shell
# scope to be requested from the wallet
Copy link
Contributor

Choose a reason for hiding this comment

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

Misplaced C&P?

@@ -258,7 +311,12 @@ configRepo:

### Gaia-X Registry

When using the [Gaia-X Digital Clearing House's](https://gaia-x.eu/services-deliverables/digital-clearing-house/) Registry Services, the issuer to be checked needs to fullfill the requirements of a Gaia-X participant. Thus, only did:web is supported for such and they need to provide a valid ```x5u``` location as part of their ```publicKeyJwk```. Usage of such registries can than be configured as following:
When using the [Gaia-X Digital Clearing House's](https://gaia-x.eu/services-deliverables/digital-clearing-house/) Registry Services, the issuer to be checked needs to fullfill the requirements of
# scope to be requested from the wallet
Copy link
Contributor

Choose a reason for hiding this comment

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

Again C&P? 😅

```
openid4vp://?client_id=did:key:verifier&request_uri=verifier.org/api/v1/request/randomState&request_uri_method=get"
```
The object than can be retrived via:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The object than can be retrived via:
The object than can be retrieved via:

The object than can be retrived via:
```shell
curl https://verifier.org/api/v1/request/randomState
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it help to show an example value of the retrieved data here?

@@ -18,6 +18,7 @@ paths:
- $ref: '#/components/parameters/QueryState'
- $ref: '#/components/parameters/ClientCallback'
- $ref: '#/components/parameters/ClientId'
- $ref: '#/components/parameters/RequestMode'
operationId: VerifierPageDisplayQRSIOP
summary: Presents a qr as starting point for the auth process
description: Returns a rendered html with a QR encoding the login-starting point for the siop flow - e.g. 'openid://?scope=somethign&response_type=rt&response_mode=rm&client_id=ci&redirect_uri=uri&state=state&nonce=nonce'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description: Returns a rendered html with a QR encoding the login-starting point for the siop flow - e.g. 'openid://?scope=somethign&response_type=rt&response_mode=rm&client_id=ci&redirect_uri=uri&state=state&nonce=nonce'
description: Returns a rendered html with a QR encoding the login-starting point for the siop flow - e.g. 'openid://?scope=somethign&response_type=rt&response_mode=rm&client_id=ci&redirect_uri=uri&state=state&nonce=nonce&request_mode=urlEncoded'

required: false
schema:
type: string
enum:
Copy link
Contributor

Choose a reason for hiding this comment

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

byReference missing?

@@ -58,6 +58,10 @@ type Logging struct {
type Verifier struct {
// did to be used by the verifier
Did string `mapstructure:"did"`
// Identification to be used for the verifier
ClientIdentification ClientIdentification `mapstructure:"clientIdentification"`
// supported request modes - currently 'urlEncoded', 'byValue' an 'byReference' are available. In case of byValue, the keyPath has to be set.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// supported request modes - currently 'urlEncoded', 'byValue' an 'byReference' are available. In case of byValue, the keyPath has to be set.
// supported request modes - currently 'urlEncoded', 'byValue' and 'byReference' are available. In case of byValue, the keyPath has to be set.

}

func (cs ConfiguredService) GetPresentationDefinition(scope string) PresentationDefinition {
logging.Log().Warnf("The ScopeEntry %s", logging.PrettyPrintObject(cs.GetScope(scope)))
Copy link
Contributor

Choose a reason for hiding this comment

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

I know its highly unlikely but what if the config from the CCS get updated between these two calls and we return a different config than we log here

cacheEntry, hit := common.GlobalCache.ServiceCache.Get(serviceIdentifier)
if hit {

logging.Log().Warnf("The definition %s", logging.PrettyPrintObject(presentationDefinition))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logging.Log().Warnf("The definition %s", logging.PrettyPrintObject(presentationDefinition))
logging.Log().Debugf("The definition %s", logging.PrettyPrintObject(presentationDefinition))


didSigningKey, err := getRequestSigningKey(verifierConfig.ClientIdentification.KeyPath, verifierConfig.ClientIdentification.Id)
if (slices.Contains(verifierConfig.SupportedModes, REQUEST_MODE_BY_VALUE) || slices.Contains(verifierConfig.SupportedModes, REQUEST_MODE_BY_REFERENCE)) && err != nil {
logging.Log().Errorf("Was not able to get a signing key, despite mode %s supported. Err: %v", REQUEST_MODE_BY_VALUE, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to wrap the error with the additional description and return it?

} else {
err = nil
}

logging.Log().Warnf("Initiated key %s.", logging.PrettyPrintObject(key))
Copy link
Contributor

Choose a reason for hiding this comment

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

can you move this line over the new code so its with the init of the field "key"?

requestObject, err := v.createAuthenticationRequestObject(redirectUri, state, clientId)
loginSession.requestObject = string(requestObject[:])

logging.Log().Infof("Store session with id %s", state)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logging.Log().Infof("Store session with id %s", state)
logging.Log().Debugf("Store session with id %s", state)

logging.Log().Errorf("Was not able to get the presentationDefintion for %s. Err: %v", clientId, err)
return requestObject, err
}
logging.Log().Warnf("The definition %s", logging.PrettyPrintObject(presentationDefinition))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logging.Log().Warnf("The definition %s", logging.PrettyPrintObject(presentationDefinition))
logging.Log().Debugf("The definition %s", logging.PrettyPrintObject(presentationDefinition))

}

headers := jws.NewHeaders()
headers.Set("typ", REQUEST_OBJECT_TYP)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the header key correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. (even thought, I'm also always confused about the missing "e")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major Should be applied for all breaking changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants