Closed
Bug 983481
Opened 10 years ago
Closed 10 years ago
post-Activation landing page
Categories
(Firefox Graveyard :: SocialAPI, defect)
Tracking
(firefox30 verified, firefox31 verified, firefox32 verified, b2g-v1.4 fixed)
VERIFIED
FIXED
Firefox 32
People
(Reporter: mixedpuppy, Assigned: mixedpuppy)
Details
Attachments
(2 files, 3 obsolete files)
626.41 KB,
video/quicktime
|
Details | |
9.59 KB,
text/plain
|
mixedpuppy
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details |
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 | ||
Comment 1•10 years ago
|
||
Assignee: nobody → mixedpuppy
Attachment #8406496 -
Flags: review?(mhammond)
Comment 2•10 years ago
|
||
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.
Comment 3•10 years ago
|
||
Seems like there should be some kind of restriction on what postActivationURL values are allowed. http[s] + restricted to provider's origin perhaps?
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
I'm splitting out the ui indicator from comment #2 into bug 999865 since these are very different mechanisms implementation-wise.
Assignee | ||
Comment 6•10 years ago
|
||
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)
Comment 7•10 years ago
|
||
(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)
Assignee | ||
Comment 8•10 years ago
|
||
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.
Comment 9•10 years ago
|
||
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)
Comment 10•10 years ago
|
||
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.
Assignee | ||
Comment 11•10 years ago
|
||
Got clarity from Gavin via irc, the scheme check is to ensure urls are http[s] and not chrome/resource/etc
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8410716 -
Attachment is obsolete: true
Attachment #8410716 -
Flags: review?(mhammond)
Attachment #8411233 -
Flags: review?(mhammond)
Comment 13•10 years ago
|
||
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+
Assignee | ||
Comment 14•10 years ago
|
||
comments addressed, pushed: https://hg.mozilla.org/integration/fx-team/rev/4fde9c8a24b3
Attachment #8411233 -
Attachment is obsolete: true
Attachment #8415491 -
Flags: review+
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4fde9c8a24b3
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Assignee | ||
Comment 16•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8415491 -
Flags: approval-mozilla-beta?
Attachment #8415491 -
Flags: approval-mozilla-beta+
Attachment #8415491 -
Flags: approval-mozilla-aurora?
Attachment #8415491 -
Flags: approval-mozilla-aurora+
Comment 17•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/71a7b58dfeef https://hg.mozilla.org/releases/mozilla-beta/rev/315f0fda0470
Comment 18•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/315f0fda0470
status-b2g-v1.4:
--- → fixed
Updated•10 years ago
|
Whiteboard: [qa+]
Comment 19•10 years ago
|
||
(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)
Assignee | ||
Comment 20•10 years ago
|
||
(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)
Comment 21•10 years ago
|
||
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)
Assignee | ||
Comment 22•10 years ago
|
||
(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)
Comment 23•10 years ago
|
||
Verified fixed 31.0a2 (2014-05-14), 32.0a1 (2014-05-14) Win 7
Updated•5 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•