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

Resolve issues of accessing settings object in parallel steps #93

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

battre
Copy link
Collaborator

@battre battre commented Jul 12, 2017

Solves #92


Preview | Diff

@equalsJeffH
Copy link
Collaborator

equalsJeffH commented Aug 23, 2017

Overall the changes in this PR look fine, and it looks like it addresses the "accessing settings object in parallel steps" issues that exist in credman itself, though it does not address everything that webauthn needs.

A key item is that the create a credential alg needs to pass the current setting's object's origin to [[create]]() (see also further comments below: #93 (review))

More complexly, WRT webauthn, and credman upon return from webauthn's [[create]]() method, @domenic wrote in w3c/webauthn#472 (comment):

I think I would advise:

  • [...]
  • It never returns actual JS objects or messes with actual JS globals. Instead it returns the necessary data to create one. (Perhaps as an Infra struct.)
    • This also needs to be done for the exceptions, actually.
  • The "request a credential" operation, after synchronously retrieving the error code or data necessary to create a credential, posts a task (going "back to the main thread") to actually do the object/exception creation and resolve/reject the promise.

Presently webauthn, in #createCredential's current step 26, does "mess with actual JS globals" in constructing a new PublicKeyCredential object to return to it's caller (credman's create a credential).

Given @domenic's feedback, and in looking at WebAssembly/design#1093 (now merged into here: https://github.com/WebAssembly/design/blob/master/Web.md), it seems to me the way to fix this up is to:

I'll work on a PR for credman to do this, building on this PR. for webauthn, I'll update the existing PR w3c/webauthn#498

pls let me know if I'm missing something....

@equalsJeffH equalsJeffH self-requested a review August 23, 2017 14:57
Copy link
Collaborator

@equalsJeffH equalsJeffH left a comment

Choose a reason for hiding this comment

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

A key item is that the create a credential alg needs to pass the current setting's object's origin to [[create]](), since webauthn's [[create]]() method (aka #createCredential) plugs in there and needs the relevant origin.

It looks to me like credman's [[CollectFromCredentialStore]](options) internal method was modified to be passed origin rather than modifying credman's [[create]]() internal method.

An adjunct thought/question that occurs to me is:

see also further comments/thoughts in #93 (comment)

@equalsJeffH
Copy link
Collaborator

PR #100 works on addressing the items in the above review

@equalsJeffH
Copy link
Collaborator

shall we close this PR since it is effectively superseded by the now-merged PR #100 ?

Base automatically changed from master to main February 16, 2021 23:21
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.

2 participants