Update trust levels for existing actions to spec #9954

dreamofabear · 2017-06-16T00:50:14Z

Fixes #9699.

Note that this will remove functionality by requiring high trust for some actions. Specifically, the actions:

AMP.goBack
<video>.play and <video>.unmute
<form>.submit

...can no longer be triggered by the following medium-trust events:

change (except from <select>)
slideChange
submit-success, submit-error
input-debounced

We don't have any low trust events yet, so actions that require low trust have no effective change.

See this spreadsheet for the canonical trust spec.

dreamofabear · 2017-06-16T01:56:59Z

/to @aghassemi @cvializ /cc @jridgewell @ericlindley-g

cvializ

LGTM

cvializ · 2017-06-16T16:52:08Z

Do we have a way to know how many pages in the wild this might break?

aghassemi · 2017-06-16T17:05:09Z

extensions/amp-form/0.1/amp-form.js

@@ -351,7 +350,7 @@ export class AmpForm {
      event.preventDefault();
    }
    // Submits caused by user input have high trust.
-    this.submit_(ActionTrust.MEDIUM); // TODO(choumx, #9699): HIGH.
+    this.submit_(ActionTrust.HIGH);


this should unblock #9879 right? @cvializ

Yep. /cc @jridgewell

Yes it should

aghassemi · 2017-06-16T17:10:02Z

spec/amp-actions-and-events.md

  </tr>
  <tr>
    <td>play</td>
    <td>Plays the video.</td>
+    <td>High</td>


This should be High only if video is unmuted. Muted videos should be playable with Low, could you create an issue and assign to me?

dreamofabear

Do we have a way to know how many pages in the wild this might break?

We could use go/cookbook for this but I'm not sure it's necessary.

dreamofabear · 2017-06-16T17:35:41Z

extensions/amp-form/0.1/amp-form.js

@@ -351,7 +350,7 @@ export class AmpForm {
      event.preventDefault();
    }
    // Submits caused by user input have high trust.
-    this.submit_(ActionTrust.MEDIUM); // TODO(choumx, #9699): HIGH.
+    this.submit_(ActionTrust.HIGH);


Yep. /cc @jridgewell

dreamofabear · 2017-06-16T17:36:19Z

spec/amp-actions-and-events.md

  </tr>
  <tr>
    <td>play</td>
    <td>Plays the video.</td>
+    <td>High</td>


camelburrito · 2017-06-16T17:51:44Z

extensions/amp-selector/0.1/amp-selector.js

        this.action_.trigger(this.element, name, selectEvent,
-            ActionTrust.MEDIUM);


@choumx What does this mean for selector?

camelburrito · 2017-06-16T17:54:03Z

extensions/amp-selector/0.1/amp-selector.js

        this.action_.trigger(this.element, name, selectEvent,
-            ActionTrust.MEDIUM);
+            ActionTrust.HIGH);


Would this not prevent slides from being in sync with selector via bind?

It wouldn't since high trust events can do everything that medium trust events can.

ericlindley-g · 2017-06-18T21:16:36Z

Just to double-check—was this taken through design review with these specific levels?

dreamofabear · 2017-06-18T21:51:05Z

@ericlindley-g No, only in this PR with relevant parties.

Any concerns? It's a small PR so we can easily undo and review next week if you'd like.

ericlindley-g · 2017-06-18T22:20:54Z

Thanks for the quick response, @choumx — non-urgent, so feel free to ignore until the week officially starts :)

I agree that the risk appears very low that we would break anyone in production with this (using medium-trust events to trigger these more disruptive actions in general would be strange). Would it make sense, process-wise, to run this through review next week, and only undo the merge if there are significant issues that come up?

As a side note: one reasonable implementation AMP may want to allow that would be restricted by this PR is playing or un-muting videos using checkboxes or radio buttons—though it's still a bit of an edge case. That's something we could either update the PR to allow now, or just think through it later on request, and see if it makes sense to open up. Would it be easy to make a targeted cookbook query just to make sure we're not breaking folks in production who might have already done these?

And is there validation in place that will also make pages that misuse these trust levels invalid, or does misuse just disable the interaction and a user-error is surfaced in the developer console?

aghassemi · 2017-06-20T00:15:52Z

@choumx @ericlindley-g Ability to unmute based on checkbox seems important. I suggest we do a blanket rule that if event.isTrusted == true then trust is high. So change could be either medium or high depending on its trigger.

ericlindley-g · 2017-06-20T17:27:09Z

@aghassemi What events fall in the category of "isTrusted == true"? I may not understand the significance of "isTrusted", but it seems like there would be events that the runtime trusts come from the user, but wouldn't want to trigger highly disruptive actions.

MrSeanpull · 2017-06-28T07:07:47Z

I used it normally so far
Suddenly since yesterday
button click error

error.js:151 Trust for 'submit' (2) insufficient (min: 3).

Ud	@	error.js:151
f.error	@	log.js:214
Hg.satisfiesTrust	@	action-impl.js:164
Ug	@	action-impl.js:502
f.trigger	@	action-impl.js:435
(anonymous)	@	action-impl.js:259

<form id="myform" class="p3 hide-inputs" method="post" action-xhr="/vote" target="_blank">
<label class="ampstart-btn vote">
<input type="radio" name="vote" value="up" on="change:myform.submit">GOOD 1</label>
<label class="ampstart-btn vote">
<input type="radio" name="vote" value="down" on="change:myform.submit">BAD 0</label>
</form>

http://imgur.com/tQNsOp9.jpg

dreamofabear · 2017-06-28T15:54:20Z

@MrSeanpull Thanks for reporting this. Per our discussion in design review, we'll rollback this change and review a way forward in #10166.

…9954)" This reverts commit 07977bd.

…0179) This reverts commit 07977bd.

update trusts to spec for existing actions/events

3c778fc

googlebot added the cla: yes label Jun 16, 2017

update docs

29e9eb8

dreamofabear force-pushed the trust-levels-final branch from 437c0f5 to 29e9eb8 Compare June 16, 2017 01:19

dreamofabear requested review from aghassemi and cvializ June 16, 2017 01:57

cvializ approved these changes Jun 16, 2017

View reviewed changes

aghassemi approved these changes Jun 16, 2017

View reviewed changes

dreamofabear commented Jun 16, 2017

View reviewed changes

camelburrito reviewed Jun 16, 2017

View reviewed changes

dreamofabear merged commit 07977bd into ampproject:master Jun 16, 2017

dreamofabear deleted the trust-levels-final branch June 16, 2017 21:00

dreamofabear mentioned this pull request Jun 20, 2017

Design Review 2017-06-21 (action trust levels) #9849

Closed

dreamofabear pushed a commit to dreamofabear/amphtml that referenced this pull request Jun 28, 2017

Revert "Update trust levels for existing actions to spec (ampproject#…

2f15fce

…9954)" This reverts commit 07977bd.

dreamofabear mentioned this pull request Jun 28, 2017

Revert "Update trust levels for existing actions to spec (#9954)" #10179

Merged

dreamofabear pushed a commit that referenced this pull request Jun 28, 2017

Revert "Update trust levels for existing actions to spec (#9954)" (#1…

d38cff6

…0179) This reverts commit 07977bd.

dreamofabear pushed a commit that referenced this pull request Jun 28, 2017

Revert "Update trust levels for existing actions to spec (#9954)" (#1…

431bffd

…0179) This reverts commit 07977bd.

dreamofabear pushed a commit that referenced this pull request Jun 28, 2017

Revert "Update trust levels for existing actions to spec (#9954)" (#1…

7a7dc2e

…0179) This reverts commit 07977bd.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update trust levels for existing actions to spec #9954

Update trust levels for existing actions to spec #9954

dreamofabear commented Jun 16, 2017 •

edited

dreamofabear commented Jun 16, 2017

cvializ left a comment

cvializ commented Jun 16, 2017

aghassemi Jun 16, 2017

dreamofabear Jun 16, 2017

cvializ Jun 16, 2017

aghassemi Jun 16, 2017

dreamofabear Jun 16, 2017

dreamofabear left a comment

dreamofabear Jun 16, 2017

dreamofabear Jun 16, 2017

camelburrito Jun 16, 2017

camelburrito Jun 16, 2017

dreamofabear Jun 16, 2017

ericlindley-g commented Jun 18, 2017

dreamofabear commented Jun 18, 2017

ericlindley-g commented Jun 18, 2017

aghassemi commented Jun 20, 2017

ericlindley-g commented Jun 20, 2017

MrSeanpull commented Jun 28, 2017 •

edited

dreamofabear commented Jun 28, 2017

		this.action_.trigger(this.element, name, selectEvent,
		ActionTrust.MEDIUM);

Update trust levels for existing actions to spec #9954

Update trust levels for existing actions to spec #9954

Conversation

dreamofabear commented Jun 16, 2017 • edited

dreamofabear commented Jun 16, 2017

cvializ left a comment

Choose a reason for hiding this comment

cvializ commented Jun 16, 2017

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

dreamofabear 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

ericlindley-g commented Jun 18, 2017

dreamofabear commented Jun 18, 2017

ericlindley-g commented Jun 18, 2017

aghassemi commented Jun 20, 2017

ericlindley-g commented Jun 20, 2017

MrSeanpull commented Jun 28, 2017 • edited

dreamofabear commented Jun 28, 2017

dreamofabear commented Jun 16, 2017 •

edited

MrSeanpull commented Jun 28, 2017 •

edited