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

Add permissions-policy check for publicKey credentials #182

Merged
merged 4 commits into from
Jan 6, 2022

Conversation

equalsJeffH
Copy link
Collaborator

@equalsJeffH equalsJeffH commented Dec 6, 2021

If the CredentialRequestOptions supplied to the Request a Credential algorithm contains a object named publicKey then check that the responsible document is allowed to use the publickey-credentials-get policy-controlled feature.

This PR supersedes PR #138.

Fixes #136
Improves #135


Preview | Diff

If the `CredentialRequestOptions` supplied to the Request a Credential algorithm contains a object named `publicKey` then check that the responsible document is allowed to use the publickey-credentials-get policy-controlled feature.
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated
7. Run the following steps [=in parallel=]:
7. Let |key| be the result of [=map/getting the key=] of |options|.

8. If |key| is {{CredentialRequestOptions/publicKey}} then
Copy link
Member

Choose a reason for hiding this comment

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

We should add web OTP

https://wicg.github.io/web-otp/#sctn-permissions-policy

This mechanism is a bit of a pain to extend to other credential types and somewhat defeats the purpose of the credential types registry. Can we add the permission policy names to the registry, and then iterate those?

Copy link
Collaborator Author

@equalsJeffH equalsJeffH Dec 21, 2021

Choose a reason for hiding this comment

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

Good idea, agreed. Although I suggest we do not attempt to do this in this PR, and we instead:

  1. submit an issue about this. ( done: see issue Add "policy controlled feature tokens" column to "Credential Types Registry"  #184 )
  2. address issue #184 in a separate PR after both this PR and PR 181 (which creates the "Credential Types" registry) land.

index.bs Outdated Show resolved Hide resolved
@equalsJeffH
Copy link
Collaborator Author

equalsJeffH commented Dec 21, 2021

I am thinking that we ought to land this PR before we land PR #181 so the latter PR can incorporate these changes (by rebasing onto main branch) because the changes in this PR are less invasive than those in PR 181.

@equalsJeffH
Copy link
Collaborator Author

equalsJeffH commented Dec 25, 2021

I resolved the conflicts with main branch using the github UI (which did a merge-from-main it seems (sorry Nina if that was the wrong thing to do)).

I think this PR is ready to merge modulo the three outstanding questions (above) I have for @nina @jyasskin:
#182 fix issue #184 in separate PR ?
#182 which error to return is not allowed by policy?
#182 Merge this PR before PR #181 ?

Copy link
Member

@jyasskin jyasskin left a comment

Choose a reason for hiding this comment

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

Merging into your feature branch won't cause any problems since the repo is configured to only allow squash-merges back to main.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
@equalsJeffH
Copy link
Collaborator Author

equalsJeffH commented Jan 6, 2022

Thanks @jyasskin for the corrections. I think this PR is now ready to land. Then, we need to complete PR #181, and then address issue #184 (as outlined in this #182 (comment) above).

@equalsJeffH equalsJeffH merged commit 307bd77 into main Jan 6, 2022
github-actions bot added a commit that referenced this pull request Jan 6, 2022
SHA: 307bd77
Reason: push, by @equalsJeffH

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

add permissions policy support for webauthn
4 participants