CREDENTIAL: Generic matching algorithm #292

adrianhopebailie · 2015-04-22T23:25:47Z

This change updates the model for Credential to include an isMatch() function hook which is called during processing of the Credential request algorithm. The hook can be overridden by developers designing their own Credential classes in script therefor making the API flexible and extensible through ECMAScript.

Resolves #274

Implemented a more generic matching logic using an isMatch() method hook on the Credential class.

mikewest · 2015-04-23T08:10:21Z

Before I skim in more detail, would you mind rebasing on top of master? This patch doesn't cleanly apply locally.

mikewest · 2015-04-23T08:10:41Z

(Also, thanks for taking a stab at this!)

This reverts commit d98c689.

adrianhopebailie · 2015-04-23T09:16:25Z

Before I skim in more detail, would you mind rebasing on top of master? This patch doesn't cleanly apply locally.

@mikewest Not sure why, I mistakenly committed my .gitignore but have reverted that. The changes are only in index.src.html and were on top of the latest from master.

My git-fu is pretty basic, any suggestions on how to fix this?

mikewest · 2015-04-23T10:06:36Z

git add remote w3c https://github.com/w3c/webappsec.git
git fetch w3c
git rebase w3c/master
[resolve conflicts]

Something like the above should do the trick! :)

Implemented a more generic matching logic using an isMatch() method hook on the Credential class.

This reverts commit d98c689.

Conflicts: specs/credentialmanagement/index.src.html

adrianhopebailie · 2015-04-23T10:43:57Z

Done.

Mixing and matching betwen the GUI tool and command line is a mistake!
Already had w3c as a remote just hadn't done a rebase (thought Git for Mac did this automatically when you did a sync - clearly not).

This was my first adventure into writing WebIDL definitions so there may be some tweaks required however I think it get's the genera; gist across.

I'll add an example of using a custom credential (defined in script) later today as I think that will truly illustrate the reasons for the change.

mikewest · 2015-04-24T07:49:49Z

specs/credentialmanagement/index.src.html

@@ -329,9 +331,16 @@ <h4 id="examples-post-signin">Post-sign-in Confirmation</h4>
        credential.send("https://example.com/sign-in/")
          .then(function (response) {
            if (response.status == 200)
+              // Inspect response to determine if this was a successful login then...


Nit: Put this before the if, please. Otherwise you'll need {} around the clause for clarity (because it's multi-line).

mikewest · 2015-04-24T08:11:38Z

specs/credentialmanagement/index.src.html

-  resolving this by rejecting the promise if the types listed in
-  {{CredentialRequest/types}} aren't all instances of the same type, but that
+  resolving this by rejecting the promise if the types listed as keys in
+  {{CredentialRequestFilters}} map aren't all instances of the same type, but that


You dropped this constraint above. If you intend to drop it, we'll need to drop this issue as well. As I noted, it's not clear that we can.

mikewest · 2015-04-24T08:17:17Z

Thanks for the PR! I've left a number of comments.

I'm fine with the central suggestion of exposing the matching algorithm, as that seems to allow some extensibility.

However, I don't like the fact that this makes the use cases the document actually intends to address more complex. Especially in the case of PasswordCredentials, the call is simply unintuitive, and will just add boilerplate to web developers' code. I'd also like to avoid the extra level of nesting for FederatedCredentials.

It's also not clear to me that filter is the right way to frame the thing that you're suggesting, as I don't understand how that verb makes sense for credential types that aren't stored locally in the browser's credential store. If I have to go out to a third-party site to grab data, I'm not filtering anymore. I'm doing something else. I couldn't come up with a term for that which made sense, and settled on the ultra-generic "options".

Does "filtering" fit into @dlongley's modeling of the world? It's not clear that it does.

adrianhopebailie · 2015-04-27T20:23:27Z

@mikewest I've been poring over this and I have come to conclusion that it won't work...

The issue is that trying to create something extensible to the developer means creating a callback that could potentially open a security hole since the code is defined and executed against the DOM.

A Credential type that allows something like this will need to be explicitly designed with this in mind and be able to handle the callback having potentially malicious code. Putting this in the base class is asking for trouble.

mikewest · 2015-04-28T14:29:53Z

What's the threat model you're worried about? If an attacker can define an arbitrary callback, they can already execute arbitrary code in your origin, can't they?

adrianhopebailie · 2015-04-28T14:41:40Z

Again, I may be showing my ignorance wrt the differentiation between the execution context of code behind a browser API vs that defined on the DOM within client scripts.

My proposal was for the site to provide a script that defines a custom Credential type and in so doing provide the execution logic for the isMatch() function. That means there is nothing stopping a rogue script from injecting logic that will be executed within what should be a secure context or am I wrong?

I like the idea of the extensibility you get from encapsulating the filtering logic in that function but don't really understand enough about the line between the two execution contexts within the browser to add anything more to this proposal.

Based on the conversation in #292, and the critique in #349, this patch restructures `get()` to accept two arguments: a sequence of types, and a dictionary of dictionaries that specifies the filter options for each relevant type. This does not completely address the request in #292, as the matching algorithm is not exposed as a method on LocallyStoredCredential. I need to think a bit more about the impact of doing that. Splitting up the patch to make sure the pieces we know we need get in for feedback.

mikewest · 2015-10-08T10:53:40Z

This issue was moved to w3c/webappsec-credential-management#4

adrianhopebailie added 2 commits April 22, 2015 19:27

Added gitignore

d98c689

Altered filtering logic to make more extensible

34aefdf

Implemented a more generic matching logic using an isMatch() method hook on the Credential class.

mozfreddyb added the CREDENTIAL label Apr 23, 2015

mozfreddyb changed the title ~~Generic matching algorithm~~ CREDENTIAL: Generic matching algorithm Apr 23, 2015

Revert "Added gitignore"

18c0026

This reverts commit d98c689.

adrianhopebailie added 4 commits April 23, 2015 12:14

Added gitignore

c41ee4c

Altered filtering logic to make more extensible

44663ca

Implemented a more generic matching logic using an isMatch() method hook on the Credential class.

Revert "Added gitignore"

36c949a

This reverts commit d98c689.

Merge branch 'master' of https://github.com/adrianhopebailie/webappsec

0f1fade

Conflicts: specs/credentialmanagement/index.src.html

adrianhopebailie added 2 commits April 23, 2015 12:47

Bug fix s/filter/isMatch()/

3786588

Fix build error

62cd83d

mikewest reviewed Apr 24, 2015
View reviewed changes

mikewest modified the milestone: CREDENTIALS: Level 1 May 13, 2015

mikewest mentioned this pull request May 13, 2015

CREDENTIAL: Extension points discussion suggests things that are not possible in Web IDL #349

Closed

mikewest mentioned this pull request Oct 8, 2015

CREDENTIAL: Generic matching algorithm w3c/webappsec-credential-management#4

Open

mikewest closed this Oct 8, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CREDENTIAL: Generic matching algorithm #292

CREDENTIAL: Generic matching algorithm #292

adrianhopebailie commented Apr 22, 2015

mikewest commented Apr 23, 2015

mikewest commented Apr 23, 2015

adrianhopebailie commented Apr 23, 2015

mikewest commented Apr 23, 2015

adrianhopebailie commented Apr 23, 2015

mikewest Apr 24, 2015

mikewest Apr 24, 2015

mikewest commented Apr 24, 2015

adrianhopebailie commented Apr 27, 2015

mikewest commented Apr 28, 2015

adrianhopebailie commented Apr 28, 2015

mikewest commented Oct 8, 2015

CREDENTIAL: Generic matching algorithm #292

CREDENTIAL: Generic matching algorithm #292

Conversation

adrianhopebailie commented Apr 22, 2015

mikewest commented Apr 23, 2015

mikewest commented Apr 23, 2015

adrianhopebailie commented Apr 23, 2015

mikewest commented Apr 23, 2015

adrianhopebailie commented Apr 23, 2015

mikewest Apr 24, 2015

Choose a reason for hiding this comment

mikewest Apr 24, 2015

Choose a reason for hiding this comment

mikewest commented Apr 24, 2015

adrianhopebailie commented Apr 27, 2015

mikewest commented Apr 28, 2015

adrianhopebailie commented Apr 28, 2015

mikewest commented Oct 8, 2015