Skip to content

add user_pin_length and user_pin_description to Credential Offer #95

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 18 commits into from
Nov 30, 2023
Merged

add user_pin_length and user_pin_description to Credential Offer #95

merged 18 commits into from
Nov 30, 2023

Conversation

@paulbastian
Copy link
Contributor

@paulbastian paulbastian commented Nov 1, 2023

Closes #10
The proposal follows discussions in the issue and in the DCP workshop before iiw#37.

@paulbastian paulbastian linked an issue Nov 1, 2023 that may be closed by this pull request
[OpenID4VCI] UserPIN description and length #10
Closed
danielfett
danielfett reviewed Nov 2, 2023
View reviewed changes
danielfett
danielfett reviewed Nov 2, 2023
View reviewed changes
@paulbastian @danielfett
Update examples/credential_offer_multiple_credentials.json
4a51f90 
Co-authored-by: Daniel Fett <fett@danielfett.de>
@paulbastian paulbastian mentioned this pull request Nov 2, 2023
Closed
peppelinux
peppelinux requested changes Nov 2, 2023
View reviewed changes
@paulbastian @peppelinux
Update openid-4-verifiable-credential-issuance-1_0.md
04dc42c 
Co-authored-by: Giuseppe De Marco <demarcog83@gmail.com>
pmhsfelix
pmhsfelix reviewed Nov 2, 2023
View reviewed changes
danielfett
danielfett reviewed Nov 5, 2023
View reviewed changes
peppelinux
peppelinux approved these changes Nov 5, 2023
View reviewed changes
danielfett
danielfett reviewed Nov 5, 2023
View reviewed changes
@paulbastian @danielfett
Update openid-4-verifiable-credential-issuance-1_0.md
0282656 
Co-authored-by: Daniel Fett <fett@danielfett.de>
jogu
jogu reviewed Nov 7, 2023
View reviewed changes
@paulbastian @jogu
Update openid-4-verifiable-credential-issuance-1_0.md
9baf677 
Co-authored-by: Joseph Heenan <joseph@heenan.me.uk>
Sakurann
Sakurann requested changes Nov 9, 2023
View reviewed changes
Copy link
Collaborator

@Sakurann Sakurann left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can we add pin type? It is needed to decide whether to show the alphanumeric or numeric keyboard to the user.

@paulbastian
Copy link
Contributor Author

paulbastian commented Nov 9, 2023

Can we add pin type? It is needed to decide whether to show the alphanumeric or numeric keyboard to the user.

Right now it is only allowed to have numeric PIN with 4-8 digits. I'm open to change these constraints.

@paulbastian
Copy link
Contributor Author

paulbastian commented Nov 9, 2023

Also, we should think about incorporating the bike-shedding from #96 before merging this

@paulbastian paulbastian mentioned this pull request Nov 24, 2023
Merged
@paulbastian
Copy link
Contributor Author

paulbastian commented Nov 24, 2023

I applied changes from the bikeshedding discussion #96 .

Do we have consensus on adding the alphabet?
I propose something like:

 "urn:ietf:params:oauth:grant-type:pre-authorized_code": {
         "pre-authorized_code": "oaKazRN8I0IbtZ0C7JuMn5",
         "tx_code" : {
            "alphabet" : "numeric"
            "length": 4,
            "description": "Please provide the PIN which was sent via e-mail"
         }
      }

Adding alphabet under tx_code with possible values numeric and alphanumeric

@jogu
Copy link
Contributor

jogu commented Nov 24, 2023

There seem to be about 20 instances of PIN still in the spec that I think at least some of still need to be changed?

@jogu
Copy link
Contributor

jogu commented Nov 24, 2023

Adding alphabet under tx_code with possible values numeric and alphanumeric

alphabet seems a bit awkward as a key name. Perhaps "inputmode" (as used in HTML) or "type"?

alphanumeric sounds like it excludes punctation and perhaps non-Latin characters. We might want to be explicit about exactly what is meant.

@paulbastian
Copy link
Contributor Author

paulbastian commented Nov 24, 2023

There seem to be about 20 instances of PIN still in the spec that I think at least some of still need to be changed?

solved

Sakurann
Sakurann reviewed Nov 28, 2023
View reviewed changes
Sakurann
Sakurann reviewed Nov 28, 2023
View reviewed changes
@Sakurann
Copy link
Collaborator

Sakurann commented Nov 28, 2023

alphabet seems a bit awkward as a key name. Perhaps "inputmode" (as used in HTML)?

I like the suggestion. we can also use text, decimal, and numeric as defined here

@Sakurann Sakurann mentioned this pull request Nov 28, 2023
Merged
@paulbastian
Copy link
Contributor Author

paulbastian commented Nov 30, 2023

added text on input_mode, please recheck @jogu @Sakurann

@paulbastian paulbastian requested a review from Sakurann November 30, 2023 13:45
Sakurann
Sakurann reviewed Nov 30, 2023
View reviewed changes
openid-4-verifiable-credential-issuance-1_0.md
* `pre-authorized_code`: REQUIRED. The code representing the Credential Issuer's authorization for the Wallet to obtain Credentials of a certain type. This code MUST be short lived and single use. If the Wallet decides to use the Pre-Authorized Code Flow, this parameter value MUST be included in the subsequent Token Request with the Pre-Authorized Code Flow.
* `user_pin_required`: OPTIONAL. Boolean value specifying whether the AS expects presentation of the End-User PIN along with the Token Request in a Pre-Authorized Code Flow. Default is `false`. This PIN is intended to bind the Pre-Authorized Code to a certain transaction to prevent replay of this code by an attacker that, for example, scanned the QR code while standing behind the legitimate End-User. It is RECOMMENDED to send a PIN via a separate channel. If the Wallet decides to use the Pre-Authorized Code Flow, a PIN value MUST be sent in the `user_pin` parameter with the respective Token Request.
* `tx_code`: OPTIONAL. An object specifying whether the AS expects presentation of a Transaction Code by the End-User along with the Token Request in a Pre-Authorized Code Flow. If the AS does not expect a Transaction Code, this object is absent, this is the default. The Transaction Code is intended to bind the Pre-Authorized Code to a certain transaction to prevent replay of this code by an attacker that, for example, scanned the QR code while standing behind the legitimate End-User. It is RECOMMENDED to send the Transaction Code via a separate channel. If the Wallet decides to use the Pre-Authorized Code Flow, the Transaction Code value MUST be sent in the `tx_code` parameter with the respective Token Request as defined in (#token_request). If no `length` or `description` is given, this object may be empty, indicating that a Transaction Code is required.
* `input_mode` : OPTIONAL. String specifying the input characters set. Possible values are `numeric` (only digits) and `text` (any character). The default is `numeric`.
Copy link
Collaborator

@Sakurann Sakurann Nov 30, 2023

Choose a reason for hiding this comment

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

should this say that this input_mode can also control the keyboard being displayed to the user?

jogu
jogu reviewed Nov 30, 2023
View reviewed changes
@Sakurann Sakurann merged commit 4ae490a into main Nov 30, 2023
@cryptphil cryptphil mentioned this pull request Dec 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danielfett danielfett danielfett approved these changes

@Sakurann Sakurann Sakurann approved these changes

@jogu jogu jogu approved these changes

@pmhsfelix pmhsfelix pmhsfelix approved these changes

@peppelinux peppelinux peppelinux approved these changes

@c2bo c2bo c2bo approved these changes

@tlodderstedt tlodderstedt Awaiting requested review from tlodderstedt

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[OpenID4VCI] UserPIN description and length

8 participants

@paulbastian @jogu @Sakurann @danielfett @pmhsfelix @peppelinux @c2bo