search

w3c-fedid / FedCM

A privacy preserving identity exchange Web API
https://w3c-fedid.github.io/FedCM/
Other
375 stars 72 forks source link

Add disclosureTextShown to the processing logic #474

Closed npm1 closed 1 year ago

npm1 commented 1 year ago

The disclosure_text_shown is supposed to be part of the ID assertion fetch. It is already somewhat part of the spec but was unfortunately not properly integrated into the processing model. This PR addresses that. The disclosure_text_shown solves the problem that the IDP wants assurance that the user has been presented the privacy policy and terms of service to use federated login, when they are new users. Since it is now the user agent that controls the UI, it can provide that assurance to the IDP via the ID assertion request.

Preview | Diff

yi-gu commented 1 year ago

Thanks! LGTM

npm1 commented 1 year ago

This has a small normative addition. @bvandersloot-mozilla @cboozar or @martinthomson could one of you review this before I merge it? Thanks!

samuelgoto commented 1 year ago

LGTM++

npm1 commented 1 year ago

Friendly ping

bvandersloot-mozilla commented 1 year ago

Does this mean that this variable is only for this one interaction? So if the user linked in the past and saw the disclosure text then- and then later reauthenticated there would be no "disclosureTextShown" on the later token request? If so, that is counterintuitive to me given the description.

npm1 commented 1 year ago

Does this mean that this variable is only for this one interaction? So if the user linked in the past and saw the disclosure text then- and then later reauthenticated there would be no "disclosureTextShown" on the later token request? If so, that is counterintuitive to me given the description.

That is correct. Why do you think it is counterintuitive?

bvandersloot-mozilla commented 1 year ago

Before the algorithm was in place I assumed it was "has ever been shown the disclosure text". I suppose that may be my own confusion. Basically, a "has opted-in on this browser" bit. That seems more useful to me, but I am not an IDP. That was a while ago that the field was added in the IDP section.

npm1 commented 1 year ago

Before the algorithm was in place I assumed it was "has ever been shown the disclosure text". I suppose that may be my own confusion. Basically, a "has opted-in on this browser" bit. That seems more useful to me, but I am not an IDP. That was a while ago that the field was added in the IDP section.

Hmm ok. Well the only way to never show the disclosure text is if the IDP tells the user agent that this is a returning user via approved_clients, because otherwise the disclosure text will be shown if this user is considered to not have been seen before by the browser. So I think this would not be very useful to the IDP. So I do think the interesting one is "did you show the disclosure text in the UI in this particular instance" since this may or may not have happened depending on whether the browser sees this user as returning or not (assuming of course it does not show up in the approved_clients).

npm1 commented 1 year ago

This PR fixes the bug that discloure_text_shown is not specified properly. It's not really introducing this into the spec: it was already there. In terms of why it was added, I believe this allows the IDP to receive confirmation about whether the privacy policy and terms of service are shown. This is particularly important for the cases where the IDP believes that they must be shown: the user agent is in those cases confirming that it did show them. There may be legal requirements for the IDP to have such assurance, although I cannot speak to those.

achimschloss commented 1 year ago

I believe this allows the IDP to receive confirmation about whether the privacy policy and terms of service are shown.

The IDP must be aware both of the presented scope of information access (it would be good to provide the scope in any case btw.) and that that was shown in context of the tos/privacy policy (of the relying party website), otherwise it can't proceed with minting a token from pure legal perspective but also unrelated to that honestly in its user facing role in general. Given FedCM as of now does not allow the IDP to shown its own UI to request a users approval.

martinthomson commented 1 year ago

As a point for future reference, it is good practice to document why a change is being made. Or rather, what problem it solves. For a pull request, referencing an issue is acceptable, but the pull request comment here was empty.

FWIW, I think that the disclosure text feature is problematic, but if it already exists and this is just fixing what is effectively a spec bug, then we can take the question about whether disclosure notice stuff is something a browser should be taking responsibility for.

samuelgoto commented 1 year ago

then we can take the question about whether disclosure notice stuff is something a browser should be taking responsibility for.

Yes, I believe it should.

npm1 commented 1 year ago

As a point for future reference, it is good practice to document why a change is being made. Or rather, what problem it solves. For a pull request, referencing an issue is acceptable, but the pull request comment here was empty.

Yea, totally agree. I have updated the PR comment.

FWIW, I think that the disclosure text feature is problematic, but if it already exists and this is just fixing what is effectively a spec bug, then we can take the question about whether disclosure notice stuff is something a browser should be taking responsibility for.

Ok, should we hold off on merging this PR until we've discussed your concerns? We could add this to the agenda of the FedID CG and discuss in the next call if you'd like.

martinthomson commented 1 year ago

I'm ok with landing technical fixes like this while the larger issue remains open.