-
Notifications
You must be signed in to change notification settings - Fork 167
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
Normalize RFC2119 langugage #470
Conversation
index.bs
Outdated
@@ -102,7 +102,7 @@ the existence, of credentials scoped to other [RPS]. | |||
|
|||
[RPS] employ the [=Web Authentication API=] during two distinct, but related, [=ceremonies=] involving a user. The first | |||
is [=Registration=], where a [=public key credential=] is created on an [=authenticator=], and associated by a [=[RP]=] | |||
with the present user's account (the account may already exist or may be created at this time). The second is | |||
with the present user's account (the account MAY already exist or MAY be created at this time). The second is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please read through the changes you're making here and check that you actually intend the RFC 2119 meanings. This change is in a non-normative section, so it's definitely not right.
please merge-from-master (into this branch) and fix build errors, to facilitate further reviews, thx. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of these changes are good but some are in descriptive, rather than normative contexts. In descriptive contexts, RFC 2119 language is not appropriate.
Also, I believe that master needs to be merged into this branch.
@@ -373,8 +373,8 @@ operations with an existing credential. All such operations are performed in the | |||
and/or platform on the user's behalf. At no point does the script get access to the credentials themselves; it only gets | |||
information about the credentials in the form of objects. | |||
|
|||
In addition to the above script interface, the authenticator may implement (or come with client software that implements) a user | |||
interface for management. Such an interface may be used, for example, to reset the authenticator to a clean state or to inspect | |||
In addition to the above script interface, the authenticator MAY implement (or come with client software that implements) a user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is descriptive - not normative language. Please back this individual change out.
In addition to the above script interface, the authenticator may implement (or come with client software that implements) a user | ||
interface for management. Such an interface may be used, for example, to reset the authenticator to a clean state or to inspect | ||
In addition to the above script interface, the authenticator MAY implement (or come with client software that implements) a user | ||
interface for management. Such an interface MAY be used, for example, to reset the authenticator to a clean state or to inspect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is descriptive - not normative language. Please back this individual change out.
index.bs
Outdated
@@ -678,7 +678,7 @@ authorizing an authenticator. | |||
The <dfn for="PublicKeyCredential" method>\[[DiscoverFromExternalSource]](options)</dfn> method is used to discover and use an | |||
existing [=public key credential=], with the user's consent. The script optionally specifies some criteria to indicate what | |||
credentials are acceptable to it. The user agent and/or platform locates credentials matching the specified criteria, and guides | |||
the user to pick one that the script will be allowed to use. The user may choose not to provide a credential even if one is | |||
the user to pick one that the script will be allowed to use. The user MAY choose not to provide a credential even if one is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is descriptive - not normative language. Please back this individual change out.
index.bs
Outdated
@@ -678,7 +678,7 @@ authorizing an authenticator. | |||
The <dfn for="PublicKeyCredential" method>\[[DiscoverFromExternalSource]](options)</dfn> method is used to discover and use an | |||
existing [=public key credential=], with the user's consent. The script optionally specifies some criteria to indicate what | |||
credentials are acceptable to it. The user agent and/or platform locates credentials matching the specified criteria, and guides | |||
the user to pick one that the script will be allowed to use. The user may choose not to provide a credential even if one is | |||
the user to pick one that the script will be allowed to use. The user MAY choose not to provide a credential even if one is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is descriptive - not normative language. Please back this individual change out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
index.bs
Outdated
@@ -872,8 +872,8 @@ during registration. | |||
<div dfn-type="attribute" dfn-for="AuthenticatorAttestationResponse"> | |||
: {{AuthenticatorResponse/clientDataJSON}} | |||
:: This attribute, inherited from {{AuthenticatorResponse}}, contains the [=JSON-serialized client data=] (see | |||
[[#sctn-attestation]]) passed to the authenticator by the client in order to generate this credential. The | |||
exact JSON serialization must be preserved, as the [=hash of the serialized client data=] has been computed | |||
[[#cred-attestation]]) passed to the authenticator by the client in order to generate this credential. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like merging master into this branch is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just merged. thank you for noticing :)
index.bs
Outdated
`myCompany_extension`. | ||
|
||
All extension identifiers MUST be a maximum of 32 octets in length and MUST consist only of printable USASCII characters, | ||
excluding backslash and doublequote, i.e., VCHAR as defined in [[!RFC5234]] but without %x22 and %x5c. Implementations MUST | ||
match WebAuthn extension identifiers in a case-sensitive fashion. | ||
|
||
Extensions that may exist in multiple versions should take care to include a version in their identifier. In effect, different | ||
Extensions that MAY exist in multiple versions SHOULD take care to include a version in their identifier. In effect, different |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Back out both of these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
index.bs
Outdated
@@ -2542,13 +2543,13 @@ input=]. For extensions that do not require input parameters and are defined as | |||
value set to `true`, this method SHOULD consist of passing an [=authenticator extension input=] value of `true` (CBOR major type | |||
7, value 21). | |||
|
|||
Note: Extensions should aim to define authenticator arguments that are as small as possible. Some authenticators communicate | |||
Note: Extensions SHOULD aim to define authenticator arguments that are as small as possible. Some authenticators communicate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is descriptive - not normative language. Please back this individual change out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
index.bs
Outdated
@@ -2601,7 +2602,7 @@ The extension also requires the client to set the authenticator parameter to the | |||
|
|||
The extension requires the authenticator to specify its geolocation in the [=authenticator extension output=], if known. The | |||
extension e.g. specifies that the location shall be encoded as a two-element array of floating point numbers, encoded with CBOR. | |||
An authenticator does this by including it in the [=authenticator data=]. As an example, [=authenticator data=] may be as | |||
An authenticator does this by including it in the [=authenticator data=]. As an example, [=authenticator data=] MAY be as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this one to "might"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done. good catch :)
index.bs
Outdated
@@ -3095,7 +3096,7 @@ platform to enumerate all the authenticator's credentials so that the client can | |||
|
|||
This is the first-time flow, in which a new credential is created and registered with the server. | |||
|
|||
1. The user visits example.com, which serves up a script. At this point, the user must already be logged in using a legacy | |||
1. The user visits example.com, which serves up a script. At this point, the user MUST already be logged in using a legacy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is descriptive - not normative language. Please back this individual change out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
index.bs
Outdated
@@ -3176,7 +3177,7 @@ credential. | |||
1. The user visits example.com, which serves up a script. | |||
|
|||
2. The script asks the client platform for an Authentication Assertion, providing as much information as possible to narrow | |||
the choice of acceptable credentials for the user. This may be obtained from the data that was stored locally after | |||
the choice of acceptable credentials for the user. This MAY be obtained from the data that was stored locally after |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this one to "can"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I support @selfissued's feedback.
@AngeloKai Can you please apply Mike's changes and merge ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Angelo, I believe this is now ready to merge after the merge conflicts are resolved. Can you please resolve them and merge? Thanks.
Address issue #182
Preview | Diff