Closed Bug 983481 Opened 6 years ago Closed 6 years ago

post-Activation landing page

Categories

(Firefox Graveyard :: SocialAPI, defect)

x86
macOS
defect
Not set

Tracking

(firefox30 verified, firefox31 verified, firefox32 verified, b2g-v1.4 fixed)

VERIFIED FIXED
Firefox 32
Tracking Status
firefox30 --- verified
firefox31 --- verified
firefox32 --- verified
b2g-v1.4 --- fixed

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

Details

Attachments

(2 files, 3 obsolete files)

We need a way for providers to know (easily) that they've had an activation occur.  This becomes slightly more problematic with the directory site, where activations will not happen on a providers site.  The idea I'm proposing is that if a manifest contains a learnmoreURL that we open that url in a browser tab after activation.
Assignee: nobody → mixedpuppy
Attachment #8406496 - Flags: review?(mhammond)
A landing page is a great start, but it lacks a key element: directing the user's attention to their recently-changed UI.  A good first step towards post activation would be a landing page opening and some visual indication on the actual UI elements that have been added - perhaps a glow.

A better (likely further down the road) solution would source the post-activation information - in a panel, for instance, directly from the added UI.

Even better still would be to use an animation like the Bookmark animation to indicate where the UI has landed.  This is particularly useful because users will need to move from one focus area (a permission dialog on the left) to elsewhere (a sidepanel or icon), and they may get lost on the way.  I'm attaching a basic animation showing how elements from the dialog could indicate the UI they create.
Seems like there should be some kind of restriction on what postActivationURL values are allowed. http[s] + restricted to provider's origin perhaps?
Comment on attachment 8406496 [details] [diff] [review]
post activation landing page patch

Clearing review request given Boriss's comments that imply further changes are necessary
Attachment #8406496 - Flags: review?(mhammond)
I'm splitting out the ui indicator from comment #2 into bug 999865 since these are very different mechanisms implementation-wise.
more extensive patch with tests.  also a small bit of refactoring to avoid future confusion in the manifest parsing code.

https://tbpl.mozilla.org/?tree=Try&rev=6e81e217fc14
Attachment #8406496 - Attachment is obsolete: true
Attachment #8410716 - Flags: review?(mhammond)
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #3)
> Seems like there should be some kind of restriction on what
> postActivationURL values are allowed. http[s] + restricted to provider's
> origin perhaps?

It transpires that the existing code is misleading - see http://mxr.mozilla.org/mozilla-central/source/toolkit/components/social/SocialService.jsm#503.  Although there is the variable:

let sameOriginRequired = ['workerURL', 'sidebarURL', 'shareURL', 'statusURL', 'markURL'];

None of those manifest attributes are actually checked for being same origin.  I vaguely recall some time ago that we decided to relax the same origin requirements, but can't recall the details - but the upshot is that *none* of those attributes are checked for same origin.

Shane's most recent patch here does enforce same origin for this new postActivationURL, as well as the workerURL - but it seems strange to insist on postActivationURL being in the same origin when none of the UI itself gets that check.  It's not even clear to me that there is real value in checking the workerURL when we don't check the UI elements - Shane suggests the user at least *can* check the origin of the panels etc, but user's aren't going to do that in practice.

Gavin, what are your thoughts?  I'm inclined to suggest *all* these should be same-origin (but leave things like images without that check).  OTOH, providers using a CDN is a valid use-case - but also one that applies to postActivationURL.
Flags: needinfo?(gavin.sharp)
There is some use of differing sub-domains in both live and demo providers.  I'm uncertain if the actual working same origin check allows sub-domains.
The bug you're thinking of is bug 897485, I think (existing checks being broken).

I don't think enforcing same origin for workerURL/shareURL/statusURL/markURL is all that useful, because loading random things in those contexts doesn't seem useful in any attack scenario (though we should perhaps check the scheme - followup bug?).

sidebarURL/postActivationURL felt different, though, because loading some arbitrary page in those contexts could be confusing to the user, but I suppose that confusion risk is pretty minimal. A page opening after install is probably clearly enough linked that this concern is unwarranted.

So what I'm saying is forget the same-origin suggestion from comment 3, but a scheme check is probably needed.
Flags: needinfo?(gavin.sharp)
Shane,
  Are you able to update this patch based on comment 9?  It seems a scheme check should be simple enough to do as part of this bug seeing we are already touching the code around it.
Got clarity from Gavin via irc, the scheme check is to ensure urls are http[s] and not chrome/resource/etc
Attachment #8410716 - Attachment is obsolete: true
Attachment #8410716 - Flags: review?(mhammond)
Attachment #8411233 - Flags: review?(mhammond)
Comment on attachment 8411233 [details] [diff] [review]
post activation landing page patch

Review of attachment 8411233 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

::: browser/base/content/test/social/browser_social_activation.js
@@ +90,5 @@
>      waitForCondition(function() {
>        let sbrowser = document.getElementById("social-sidebar-browser");
>        let provider = SocialSidebar.provider;
> +      let postActivation = provider && gBrowser.contentDocument.location.href == provider.origin + "/browser/browser/base/content/test/social/social_postActivation.html";
> +      

trailing whitespace

::: toolkit/components/social/SocialService.jsm
@@ +532,5 @@
>        if (data[url]) {
>          try {
> +          let resolved = Services.io.newURI(principal.URI.resolve(data[url]), null, null);
> +          if (!(resolved.schemeIs("http") || resolved.schemeIs("https")))
> +            return null;

I think a Cu.reportError() here makes sense before returning null
Attachment #8411233 - Flags: review?(mhammond) → review+
comments addressed, pushed:

https://hg.mozilla.org/integration/fx-team/rev/4fde9c8a24b3
Attachment #8411233 - Attachment is obsolete: true
Attachment #8415491 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/4fde9c8a24b3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Comment on attachment 8415491 [details]
post activation landing page patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: potential difficulty understanding changes to firefox when a user activates a provider.  This allows a provider to offer further details after a successful activation.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #8415491 - Flags: approval-mozilla-beta?
Attachment #8415491 - Flags: approval-mozilla-aurora?
Attachment #8415491 - Flags: approval-mozilla-beta?
Attachment #8415491 - Flags: approval-mozilla-beta+
Attachment #8415491 - Flags: approval-mozilla-aurora?
Attachment #8415491 - Flags: approval-mozilla-aurora+
Whiteboard: [qa+]
(In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> This allows a provider to offer further details after a successful activation.
Can you please provide some info on how can this be verified ?
Flags: needinfo?(mixedpuppy)
(In reply to Paul Silaghi, QA [:pauly] from comment #19)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #16)
> > This allows a provider to offer further details after a successful activation.
> Can you please provide some info on how can this be verified ?

Activate Pocket from https://activations.cdn.mozilla.net/en-US/
After activation, a new tab should be opened with the page: https://getpocket.com/installed?firefoxsave=1
Flags: needinfo?(mixedpuppy)
Verified as fixed using the following environment:
FF 30.0b4
Build Id: 20140512231802
OS: Win 7 x64, Ubuntu 12.10 x86, Mac Os X 10.8.5

When activating Pocket, https://getpocket.com/installed?firefoxsave=1 new tab is opened.

However after Pocket was activated clicking again on "Activate"(Pocket) on https://activations.cdn.mozilla.net/en-US/ page will open a new ttps://getpocket.com/installed?firefoxsave=1 tab. Is this expected behavior?
Flags: needinfo?(mixedpuppy)
(In reply to Catalin Varga [QA][:VarCat] from comment #21)
> Verified as fixed using the following environment:
> FF 30.0b4
> Build Id: 20140512231802
> OS: Win 7 x64, Ubuntu 12.10 x86, Mac Os X 10.8.5
> 
> When activating Pocket, https://getpocket.com/installed?firefoxsave=1 new
> tab is opened.
> 
> However after Pocket was activated clicking again on "Activate"(Pocket) on
> https://activations.cdn.mozilla.net/en-US/ page will open a new
> ttps://getpocket.com/installed?firefoxsave=1 tab. Is this expected behavior?

yes
Flags: needinfo?(mixedpuppy)
Verified fixed 31.0a2 (2014-05-14), 32.0a1 (2014-05-14) Win 7
Status: RESOLVED → VERIFIED
Keywords: verifyme
Whiteboard: [qa+]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.