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

credential_pop typ with status-attestation-request+cwt + huge editorials #37

Merged

Conversation

SaraConsoliACN
Copy link
Contributor

this commit may resolve issue #20.

draft-demarco-oauth-status-attestations.md Outdated Show resolved Hide resolved
draft-demarco-oauth-status-attestations.md Outdated Show resolved Hide resolved
draft-demarco-oauth-status-attestations.md Outdated Show resolved Hide resolved
@peppelinux peppelinux changed the title Fix: issue #20 credential_pop typ with status-attestation-request+cwt May 7, 2024
SaraConsoliACN and others added 3 commits May 8, 2024 15:57
Co-authored-by: Giuseppe De Marco <giuseppe.demarco@teamdigitale.governo.it>
The Content-Type has been changed from application/x-www-form-urlencoded to application/json.
SaraConsoliACN and others added 8 commits May 10, 2024 17:21
Co-authored-by: Giuseppe De Marco <giuseppe.demarco@teamdigitale.governo.it>
Co-authored-by: Giuseppe De Marco <giuseppe.demarco@teamdigitale.governo.it>
Co-authored-by: Giuseppe De Marco <giuseppe.demarco@teamdigitale.governo.it>
Co-authored-by: Giuseppe De Marco <giuseppe.demarco@teamdigitale.governo.it>
@@ -384,7 +385,8 @@ When the JWT or CWT format are used, the JWT/CWT MUST contain the parameters def

| Payload | Description | Reference |
| --- | --- | --- |
| **iss** | Wallet identifier. | {{RFC9126}}, {{RFC7519}} |
| **iss** | Wallet identifier. The Wallet identifier is out of the scope of this specs considering that it may be just a
Copy link
Owner

Choose a reason for hiding this comment

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

the wallet identifier is not a framework

Copy link
Collaborator

Choose a reason for hiding this comment

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

The last sentence ("considering that....") is not clear to me. Is there a reason why we decided to emphasize that the Wallet identifier is out of the scope of this spec? I suggest removing this sentence., if you agree.
@OR13 @marinaado

Copy link
Owner

Choose a reason for hiding this comment

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

@fmarino-ipzs the way a wallet can be identified or the value to be supposed to be used for identify a wallet is definitively out of the scope of this spec

draft-demarco-oauth-status-attestations.md Outdated Show resolved Hide resolved
SaraConsoliACN and others added 3 commits May 16, 2024 09:11
…est"

Co-authored-by: Giuseppe De Marco <giuseppe.demarco@teamdigitale.governo.it>
This commit aims to resolve the following comments:
- peppelinux#37 (comment)
- peppelinux#37 (comment)
@peppelinux peppelinux changed the title credential_pop typ with status-attestation-request+cwt credential_pop typ with status-attestation-request+cwt + huge editorials May 17, 2024
@peppelinux
Copy link
Owner

peppelinux commented May 17, 2024

This PR is ready to be merged, a further PR will be create to address @marinaado 's comment #45 (comment) and the table related to the errors where the placeholder is here defined: https://github.com/peppelinux/draft-demarco-oauth-status-attestations/pull/37/files#diff-6018989306b8996e79d471eb123fc3b59796d1635384b6faac8cdb36cd5f49ccR472

regarding the first comment, since it is required that the status assertion issuer always respond with an application/json containing the status_assertion_errors, I'd try to use a general approach without defining specialized error section for both the requests and the responses

@peppelinux peppelinux merged commit 7c3d3df into peppelinux:main May 20, 2024
1 check passed
@SaraConsoliACN SaraConsoliACN mentioned this pull request May 20, 2024
@peppelinux peppelinux mentioned this pull request Jun 10, 2024
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.

4 participants