Use Permissions Policy instead of sameOriginWithAncestors #236

johannhof · 2022-03-24T11:11:24Z

johannhof · 2022-03-24T11:11:57Z

(I'm going to join fedidcg to make the IPR pass)

dj2 · 2022-03-24T11:53:52Z

We inherit the [[DiscoverFromExternalSource]] from Credential Management Level 1 (https://www.w3.org/TR/credential-management-1/#algorithm-discover-creds). So, this would need to be changed there and then propagated to FedCM.

johannhof · 2022-03-24T20:07:23Z

Ah, right, I missed that this parameter came from the original function. I'll file an issue with credential management about PP vs. sameOriginWithAncestors, but I think for now it's probably acceptable to mention the parameter but ignore it in the algorithm.

Thanks!

johannhof · 2022-03-24T21:34:32Z

@dj2 updated, PTAL :)

nsatragno · 2022-03-31T16:17:34Z

Note that permission policy check itself should be added to the relevant algorithm in the credential management side since [[Discover From External Source]] is called from "in parallel" steps and cannot access the environment settings object.

johannhof · 2022-04-04T09:48:58Z

Thanks @nsatragno, I think that makes sense. I'll update the PR.

johannhof · 2022-04-07T12:23:02Z

@nsatragno @dj2

I noticed another potential issue that I wanted to get your thoughts on:

Besides [[DiscoverFromExternalSource]], Request a Credential will also invoke "Collect Credentials from the credential store" which calls into [[CollectFromCredentialStore]]. This is already implemented by FederatedCredential and includes another sameOriginWithAncestors check.

I'm not yet super familiar with Credentials so maybe I'm wrong here, but it looks to me like this algorithm would also be called for FedCM FederatedCredentials and effectively prevent the Permissions Policy check in [[DiscoverFromExternalSource]] from happening.

Potential solutions:

Override [[CollectFromCredentialStore]] in FedCM and get rid of the same-origin check
Use the presence of some indicator (e.g. the providers array) in the FederatedCredential variant of [[CollectFromCredentialStore]] to skip the check conditionally.
Move the same-origin check out of the parallel steps and into the main "Request a credential" algorithm, where the PP check already lives.

Any thoughts/preferences?

dj2 · 2022-04-07T14:37:22Z

There is also #237 which could move us away from monkey patching FederatedCredential if people agree with that direction. This would mean we'd have to put the definition for the [[CollectFromCredentialStore]] in this spec and we could handle it the same as Discover.

johannhof · 2022-04-08T10:02:04Z

Right, that might be good. Either way, it does feel like we could move [[CollectFromCredentialStore]] into this spec (overriding FederatedCredential for now) and potentially do #237 as a follow-up.

nsatragno · 2022-04-14T20:02:35Z

(Sorry I somehow missed this)

My recommendation is to move away from monkey patching and instead create a brand-new credential type to avoid this class of issues now and in the future, e.g. if we fix a bug on credman's FederatedCredential we might accidentally break FedCM. Making eventual deprecation of credman's FederatedCredential easier is a nice bonus.

If it's at all helpful to have the current implementation that overrides FederatedCredential in the meantime, re-specifying [[CollectFromCredentialStore]] in the FedCM spec sounds good -- it'll be less work to migrate to FederatedCredential2.

johannhof · 2022-04-21T10:18:07Z

@dj2 @nsatragno I made some updates to reflect our discussion, PTAL. I wanted to avoid overriding CollectFromCredentialStore since that would be global (i.e. making the version in credman irrelevant), instead opting to modify the algorithm to carve out a case for federated credentials. This should definitely get cleaned up long-term and as mentioned in #237 I'm starting to think that moving this proposal's FederatedCredential implementation into the Credential Management spec could be a way forward.

If you're okay with this new version I'll also submit a PR to credman adding the allowed to use check.

npm1

Looks good to me. @samuelgoto PTAL?

npm1 · 2022-04-25T21:05:48Z

spec/index.bs

@@ -1026,6 +1026,19 @@ requests.
    ::  Optional |accountId| hint. Maybe used by the UA when displaying the
 </dl>

+This specification modifies the {{FederatedCredential}}'s {{FederatedCredential/[[CollectFromCredentialStore]](origin, options, sameOriginWithAncestors)}} method by prepending the following step:


Technically this would be the second step, i.e. after the assert, right?

Yeah, I guess technically you're right, I figured we can skip the assert for simplicity as existence of the map is somewhat implied by exists, but I can try to reword.

npm1 · 2022-04-25T21:06:07Z

spec/index.bs

+
+  1.  If |options|["{{CredentialRequestOptions/federated}}"]["{{FederatedCredentialRequestOptions/providers}}"] [=map/exists=], return the result of <a abstract-op lt="Retrieve a list of credentials">retrieving</a>
+      credentials from the [=credential store=] that match the following filter:
+        1.  The credential is a {{FederatedCredential}}


nit: add period for consistency with other bullet points.

npm1 · 2022-04-25T21:08:54Z

spec/index.bs

+        Note: This step skips any checks for |sameOriginWithAncestors|, which is instead done by
+        evaluating whether the document is [=allowed to use=] the `"fedcm"` feature at at the
+        [[!CREDENTIAL-MANAGEMENT-1]] level. See [[#permissions-policy-integration]].
+
 This specification overrides the {{FederatedCredential}}'s


Should we later not override this method? Maybe add an Issue

I wouldn't require us to change this as long as CredMan has a no-op in place of the overriden function, this draft is for incubation anyway and a final decision could be made in #237 as a result of which this will likely change anyway.

npm1 · 2022-04-25T21:10:58Z

spec/index.bs

+
+This restriction can be controlled using the mechanisms described in [[PERMISSIONS-POLICY]].
+
+Note: Algorithms specified in [[!CREDENTIAL-MANAGEMENT-1]] perform the actual permissions policy evaluation. This is because such policy evaluation needs to occur when there is access to the [=current settings object=]. The [=internal method=]s modified by this specification do not have such access since they are invoked [=in parallel=] by {{CredentialsContainer}}'s <a abstract-op>Request a `Credential`</a> abstract operation [[!CREDENTIAL-MANAGEMENT-1]].


The last mention to [[!CREDENTIAL-MANAGEMENT-1]] here seems off, should we remove it?

Yeah, that seems removable.

johannhof · 2022-08-03T13:26:35Z

I think I can take another stab at this now that we have IdentityCredential in place.

See rationale here: w3c-fedid#233 (comment)

Companion change to w3c-fedid/FedCM#236

npm1

Looks good modulo naming!

npm1 · 2022-08-04T22:48:56Z

spec/index.bs

+# Permissions Policy Integration # {#permissions-policy-integration}
+<!-- ============================================================ -->
+
+FedCM defines a [=policy-controlled feature=] identified by the string `"fedcm"`.


Let's use a different name, not fedcm. How about identity-credential (or the plural, not sure what is better in this case)?

Hmm so for publickey (which is the closest to this, I guess) we have publickey-credentials-get, which might make sense as it describes the capability that we're exposing, so identity-credential-get? It sounds a bit funky though.

@clelland do you have any thoughts/advice on naming for PP?

identity-credential-get for consistency with the publickey SGTM

Thanks, let's agree on that, then!

Should we do the plural identity-credentials-get to be consistent?

Yeah, I agree, it's also more consistent with the API call (credentials.get) thanks for catching that! I can update this later, as I was going to make it linkable as well.

@samuelgoto @npm1 assuming no concerns with this but lmk otherwise

No concern from my end

samuelgoto · 2022-08-16T18:35:50Z

LGTM with the small naming changes to keep it consistent. Once you resolve them, I'll merge.

johannhof · 2022-08-16T20:03:43Z

Done! :)

samuelgoto · 2022-08-16T20:06:57Z

Merged!

@samuelgoto

SHA: 9c05a08 Reason: push, by @samuelgoto Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Add permissions policy check for FedCM Companion change to w3c-fedid/FedCM#236

) * Use Permissions Policy instead of sameOriginWithAncestors See rationale here: w3c-fedid#233 (comment) * Update feature name based on feedback

johannhof mentioned this pull request Mar 25, 2022

Permissions Policy vs. sameOriginWithAncestors checks w3c/webappsec-credential-management#195

Closed

dj2 approved these changes Apr 6, 2022

View reviewed changes

npm1 approved these changes Apr 25, 2022

View reviewed changes

johannhof force-pushed the permissions-policy branch 2 times, most recently from 7094b1a to 516b53e Compare August 4, 2022 13:03

Use Permissions Policy instead of sameOriginWithAncestors

b4cfabf

See rationale here: w3c-fedid#233 (comment)

johannhof force-pushed the permissions-policy branch from 516b53e to b4cfabf Compare August 4, 2022 13:09

johannhof requested a review from npm1 August 4, 2022 13:20

johannhof added a commit to johannhof/webappsec-credential-management that referenced this pull request Aug 4, 2022

Add permissions policy check for FedCM

4b13ebc

Companion change to w3c-fedid/FedCM#236

johannhof mentioned this pull request Aug 4, 2022

Add permissions policy check for FedCM w3c/webappsec-credential-management#204

Merged

npm1 reviewed Aug 4, 2022

View reviewed changes

Update feature name based on feedback

e2e229f

samuelgoto merged commit 9c05a08 into w3c-fedid:main Aug 16, 2022

github-actions bot added a commit that referenced this pull request Aug 16, 2022

Use Permissions Policy instead of sameOriginWithAncestors (#236)

d59a650

SHA: 9c05a08 Reason: push, by @samuelgoto Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

johannhof deleted the permissions-policy branch August 16, 2022 20:12

johannhof mentioned this pull request Aug 18, 2022

Update and export PP feature name #327

Merged

johannhof added a commit to w3c/webappsec-credential-management that referenced this pull request Aug 30, 2022

Add permissions policy check for FedCM (#204)

05f4402

* Add permissions policy check for FedCM Companion change to w3c-fedid/FedCM#236

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Permissions Policy instead of sameOriginWithAncestors #236

Use Permissions Policy instead of sameOriginWithAncestors #236

johannhof commented Mar 24, 2022 •

edited by pr-preview bot

Loading

johannhof commented Mar 24, 2022

dj2 commented Mar 24, 2022

johannhof commented Mar 24, 2022

johannhof commented Mar 24, 2022

nsatragno commented Mar 31, 2022

johannhof commented Apr 4, 2022

johannhof commented Apr 7, 2022 •

edited

Loading

dj2 commented Apr 7, 2022

johannhof commented Apr 8, 2022

nsatragno commented Apr 14, 2022

johannhof commented Apr 21, 2022 •

edited

Loading

npm1 left a comment

npm1 Apr 25, 2022

johannhof Apr 26, 2022

npm1 Apr 25, 2022

npm1 Apr 25, 2022

johannhof Apr 26, 2022

npm1 Apr 25, 2022

johannhof Apr 26, 2022

johannhof commented Aug 3, 2022

npm1 left a comment

npm1 Aug 4, 2022

johannhof Aug 5, 2022

npm1 Aug 16, 2022

samuelgoto Aug 16, 2022

johannhof Aug 16, 2022

yi-gu Aug 17, 2022

johannhof Aug 17, 2022

johannhof Aug 17, 2022

npm1 Aug 17, 2022

samuelgoto commented Aug 16, 2022

johannhof commented Aug 16, 2022

samuelgoto commented Aug 16, 2022


		This restriction can be controlled using the mechanisms described in [[PERMISSIONS-POLICY]].

		Note: Algorithms specified in [[!CREDENTIAL-MANAGEMENT-1]] perform the actual permissions policy evaluation. This is because such policy evaluation needs to occur when there is access to the [=current settings object=]. The [=internal method=]s modified by this specification do not have such access since they are invoked [=in parallel=] by {{CredentialsContainer}}'s <a abstract-op>Request a `Credential`</a> abstract operation [[!CREDENTIAL-MANAGEMENT-1]].

Use Permissions Policy instead of sameOriginWithAncestors #236

Use Permissions Policy instead of sameOriginWithAncestors #236

Conversation

johannhof commented Mar 24, 2022 • edited by pr-preview bot Loading

johannhof commented Mar 24, 2022

dj2 commented Mar 24, 2022

johannhof commented Mar 24, 2022

johannhof commented Mar 24, 2022

nsatragno commented Mar 31, 2022

johannhof commented Apr 4, 2022

johannhof commented Apr 7, 2022 • edited Loading

dj2 commented Apr 7, 2022

johannhof commented Apr 8, 2022

nsatragno commented Apr 14, 2022

johannhof commented Apr 21, 2022 • edited Loading

npm1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johannhof commented Aug 3, 2022

npm1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelgoto commented Aug 16, 2022

johannhof commented Aug 16, 2022

samuelgoto commented Aug 16, 2022

johannhof commented Mar 24, 2022 •

edited by pr-preview bot

Loading

johannhof commented Apr 7, 2022 •

edited

Loading

johannhof commented Apr 21, 2022 •

edited

Loading