786133 - Need a way to discover and install additional social providers

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Comment 1

[Axel Nennker]

As suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=809694#c16
I still think that adding the manifest to the event.detail is the easiest way to add external manifests. This is better then to search link elements and download the manifest.

e.detail should become a parameter to activateFromOrigin:
let provider = Social.activateFromOrigin(providerOrigin, e.detail);

I have not completed this patch because my last comment https://bugzilla.mozilla.org/show_bug.cgi?id=809694#c16 was ignored.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Hi Axel,

I think we will take an approach like the one you describe when it comes time to implement this. But we've been focused on multiprovider support and fixes to the existing functionality, so we've not had time to look into this much. Supporting installation of arbitrary providers will require some additional UI work, so it's not quite as simple as making it work with the existing event (though that's certainly an important part of it). You can find me (and some other devs) on #socialdev on irc.mozilla.org if you want to chat about it more!

Comment 3

[Shane Caraveo (:mixedpuppy)]

We'll need to outline our requirements around the manifest data.  While using the event is convenient, it is not discoverable and updating the manifest would be more difficult (ie. user has to browse to a page and initiate an action).

Comment 4

[Axel Nennker]

To make the manifest discoverable I think it is best put into some webfinger record 
 https://datatracker.ietf.org/doc/draft-ietf-appsawg-webfinger/
of the site hosting the social service. Or bake your own /.well-known/social-manifest.json

[In previous years Mozilla discourage addons that watch for webpage changes as being too expensive and slowing the UI too much. I would go this way: if social is enabled than attach to the DOMContentLoaded event of each page and parse the page once for <link rel="urn:mdn:social:manifest:1.0" href="someurl"/> or something like that.]

I don't understand your priorities regarding multiple providers and manifest discovery/addition. If the manifest has to be added magically to the preferences beforehand than I - as a social provider - have to write an addon to do that. If I have to write an addon anyway and then the social-integration does not help me much.
So I think that adding manifests through the event or through webfinger or whatever should be near the top of this team's priorities.
Maybe I failed to discover other ways to add manifests...?

To the point of updating manifests: I suggest to have a field in the manifest that provides an update url like in addon's install.rdf.
https://developer.mozilla.org/en-US/docs/Install_Manifests#updateURL

--- social provider site js ---->
var manifest = {
 "origin":"http://axel.nennker.de",
 "name":"Demo Messenger",
 "workerURL":"http://axel.nennker.de/socialapi-demo-gh-pages/worker.js",
 "iconURL":"data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAYAAAAf8/9hAAAAGXRFWHRTb2Z0d2FyZQBBZG9iZSBJbWFnZVJlYWR5ccllPAAAAU1JREFUeNqckc1qg0AUhY9TsQEXwUWCbrOQbF3Xh+m+6z5EX6ob3ZQKLitBhOpGF020UEziX50JI0ZjaHrgOtfjfHfuHYWmaRAEQUNXLp4PvaG/Xq8FkcKapo3gvxTyPK8R+cvxeBwBfL23XpA/PJ/5s9kMdV2DTJ3Wh1EVZ36/C3INZmsLsxjANKqqArkEZVmGzWaDNE1bo2qjRhRFcBwHRVGwccuyPBVATxT2fR9JkrB8Pp+j/vlCmXyweXe7HYMsy+oYsd8ShelGmq9WKxwOB9x9vgN1icViwcK27f6ZEHmS53l3u3Tjfr8/fWhhriHcFaBgGIZsZkmSoCgKLsk0zZHH7oC2raoqttst4jjGLSL8VgkhWC6Xk61OFqAwl67rNxchQ6NfhI/z/fg6WaD7C7Isd6ZhGF1ePb1BvtKBQB+u6zb4h9qDhF8BBgDVYxk7AyQpfgAAAABJRU5ErkJggg==",
 "sidebarURL":"http://axel.nennker.de/socialapi-demo-gh-pages/sidebar.htm",
 "updateURL":"http://axel.nennnker.de/socialapi-demo-gh-pages/manifest.json",
 "jwk":{"alg":"RSA","n": "0vx7agoebGcQSuuPiLJXZptN9nndrQmbXEps2aiAFbWhM78LhWx4cbbfAAtVT86zwu1RK7aPFFxuhDR1L6tSoc_BJECPebWKRXjBZCiFV4n3oknjhMs
  tn64tZ_2W-5JsGY4Hc5n9yBXArwl93lqt7_RN5w6Cf0h4QyQ5v-65YGjQR0_FDW2QvzqY368QQMicAtaSqzs8KJZgnYb9c7d0zgdAZHzu6qMQvRL5hajrn1n91CbOpbISD08qNLyrdkt-bFTWhAI4vMQFh6WeZu0fM4lFd2NcRwr3XPksINHaQG_xBniIqbw0Ls1jF44-csFCur-kEgU8awapJzKnqDKgw","e":"AQAB","kid":"2012-12-27"}};

var customEventInit = {"detail":manifest};

var event = new CustomEvent("ActivateSocialFeature", customEventInit);

document.dispatchEvent(event);
<--- social provider site js ----

For the time being I would not require https for updateURL but maybe the manifest can be a signed json and the signing key be learned on "trust on first use"-basis using a json web key?

Axel

ps: IRC did not work good for me because of the different timezones

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: install, whitelist and directory patch

patch exploring how to implement for requirements around whitelisting, builtin providers and directory install.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: install, whitelist and directory patch

updated to latest m-c

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 706081 [details] [diff] [review]
install, whitelist and directory patch

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

untested, but LGTM in principal.

::: browser/base/content/browser-social.js
@@ +212,5 @@
>  
>    // This handles "ActivateSocialFeature" events fired against content documents
>    // in this window.
>    _activationEventHandler: function SocialUI_activationHandler(e) {
> +    let targetDoc = e.target.ownerDocument;

We seem to now be allowing the target to be any element in the doc - why is that?  And assuming it is reasonable, the following comment probably needs to be updated.

@@ +235,5 @@
>      Social.lastEventReceived = now;
>  
> +    // Check that the associated document's origin is in our whitelist
> +    let providerOrigin = targetDoc.nodePrincipal.origin;
> +    let installType = Social.canActivateOrigin(providerOrigin);

sounds like canActivateOrigin needs a rename to getOriginActivationType or similar?

@@ +254,5 @@
> +    }
> +
> +    if (installType === 'foreign') {
> +      // foreign installation from a website
> +      Social.installProvider(node.baseURI, data, function(data) {

it almost looks like we need a new module SocialActivation.jsm with these functions, plus the Social.install*, Social.validate etc stuff?

@@ +258,5 @@
> +      Social.installProvider(node.baseURI, data, function(data) {
> +        this.doActivation(data.origin);
> +      }.bind(this));
> +      return;
> +    } else {

can we tweak these if conditions?  Eg, I see here that 'installType' may be 'foreign' or 'builtin' (and maybe others).  'foreign' is handled in the first if, but the else has a comment that it's "not builtin" - at face value, it looks like it *is* (possibly) builtin.

@@ +293,5 @@
>        let notificationPanel = SocialUI.notificationPanel;
>        // Set the origin being activated and the previously active one, to allow undo
>        notificationPanel.setAttribute("origin", provider.origin);
>        notificationPanel.setAttribute("oldorigin", oldOrigin);
> +      notificationPanel.setAttribute("pid", provider.id);

'providerid' sounds better than 'pid' to me

::: toolkit/components/social/SocialService.jsm
@@ +226,5 @@
> +    if (whitelist.indexOf(origin) >= 0)
> +      return 'whitelist';
> +    // directory whitelisted?  Assume it is reviewed/approved by the
> +    // directory.
> +    let directories = Services.prefs.getCharPref("social.directories").split(',');

this looks wrong to me.  The directory origin is different than the origin of providers it allows you to install.  Don't we need to pass 2 origins here - the origin where the install is being done from, plus the origin of the provider being installed?

@@ +251,5 @@
>        }
>      }
> +  },
> +
> +  validateManifest: function(data, baseURI) {

"validate" implies it returns a bool, but infact it returns the manifest itself.  Something like manifestFromData() seems like a better name

@@ +260,5 @@
> +    let type = this.canActivateOrigin(URI.prePath);
> +    // directory whitelisted?  Assume it is reviewed/approved by the
> +    // directory.
> +    let directories = Services.prefs.getCharPref("social.directories").split(',');
> +    if (type == 'foreign') {

similarly to the comment for browser-social, how about a switch(type), with a default branch rejecting the manifest?  That would prevent future new values for 'type' breaking something if this code isn't updated accordingly.

@@ +265,5 @@
> +      // web installed, force some requirements on the manifest:
> +      // 1. origin must be site we're installing from
> +      // 2. we ensure a unique id by tieing it to the origin
> +      data.id = URI.host;
> +      data.origin = URI.prePath;

I think we were smacked in the past for using URI.prePath - eg, https://bugzilla.mozilla.org/show_bug.cgi?id=799600#c12.  I wonder if we should use
 nsIScriptSecurityManager.getSimpleCodebasePrincipal(in nsIURI aURI); to fetch the principal (which you fetch later anyway) and use the .origin attribute of that?  Or something :)

@@ +290,5 @@
> +      if (data[url]) {
> +        try {
> +          data[url] = Services.io.newURI(principal.URI.resolve(data[url]), null, null);
> +        } catch(e) {
> +          Cu.reportError("invalid manifest from "+baseURI);

why the vague and generic reportError here, but not for other places that return null?  If you are going to bother doing the reportError, you might as well add specific details in the message.

@@ +295,5 @@
> +          return null;
> +        }
> +      }
> +    }
> +    return data;

with this change, I wonder if we should drop the other same-origin checks we later do on these URLs?  There doesn't seem to be any advantage to doing them twice.

@@ +650,5 @@
> +  this.sourceURI = sourceURI;
> +  this.install = function() {
> +    Services.prefs.setCharPref("social.manifest." + aManifest.id,
> +                               JSON.stringify(aManifest));
> +    schedule(function() {

schedule() here probably isn't necessary - we are already in a callback, so there is no need to "pretend" we are async (which is what most of the other schedule() calls are for)

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 706081 [details] [diff] [review]
install, whitelist and directory patch

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

When a new provider has been installed you'll probably want to send an onExternalInstall event through the add-ons API to let the UI know something new has appeared (https://developer.mozilla.org/en-US/docs/Addons/Add-on_Manager/InstallListener#onExternalInstall%28%29)

::: toolkit/components/social/SocialService.jsm
@@ +305,5 @@
> +    args.installs = [new AddonInstaller(sourceURI, aManifest, installCallback)];
> +    args.wrappedJSObject = args;
> +
> +    Services.ww.openWindow(this.window, "chrome://mozapps/content/xpinstall/xpinstallConfirm.xul",
> +                           null, "chrome,modal,centerscreen", args);

Do we actually need this scary dialog, have you chatted with UX/security about it. I was expecting more of a simple popup notification to confirm an install and not needing to go this route.

Comment 9

[Shane Caraveo (:mixedpuppy)]

(In reply to Dave Townsend (:Mossop) from comment #8)
> Comment on attachment 706081 [details] [diff] [review]
> install, whitelist and directory patch

> Do we actually need this scary dialog, have you chatted with UX/security
> about it. I was expecting more of a simple popup notification to confirm an
> install and not needing to go this route.

I'm only using it for initial work on the functionality.  Bug 836452 is created to deal with that ui.

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 706081 [details] [diff] [review]
install, whitelist and directory patch

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

Looking good for now

Comment 11

[Shane Caraveo (:mixedpuppy)]

(In reply to Dave Townsend (:Mossop) from comment #10)
> Comment on attachment 706081 [details] [diff] [review]
> install, whitelist and directory patch
> 
> Review of attachment 706081 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good for now

Thanks.  I got onExternalInstall working, but am floundering a little bit on uninstall.  I have to click remove twice to get it to actually uninstall, so I feel like I'm missing some notification, or that the provider class might be missing something.

Comment 12

[Shane Caraveo (:mixedpuppy)]

Attachment: install, whitelist and directory patch

updated with feedback, fixed removal issues, other cleanup.  Not asking for full review as I haven't looked into more tests yet.

Comment 13

[Shane Caraveo (:mixedpuppy)]

Attachment: install, whitelist and directory patch

updated patch.  comments should all be addressed.  a couple install tests added.

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 716878 [details] [diff] [review]
install, whitelist and directory patch

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

This is quite complex - I wonder if the install stuff shouldn't be split into its own module, or somehow cleaned up to make it a little less complex to read and understand.  The test coverage doesn't look that great either, especially for error conditions.  Testing for a builtin provider having a manifest overridden also seems worthwhile seeing there is special support for that.

f+ on the general direction (but I'd probably r- in its current state if I was asked for that ;)

::: browser/app/profile/firefox.js
@@ +1201,5 @@
>  
>  // Default social providers
>  pref("social.manifest.facebook", "{\"origin\":\"https://www.facebook.com\",\"name\":\"Facebook Messenger\",\"workerURL\":\"https://www.facebook.com/desktop/fbdesktop2/socialfox/fbworker.js.php\",\"iconURL\":\"data:image/x-icon;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAAAQCAYAAAAf8%2F9hAAAAX0lEQVQ4jWP4%2F%2F8%2FAyUYTFhHzjgDxP9JxGeQDSBVMxgTbUBCxer%2Fr999%2BQ8DJBuArJksA9A10s8AXIBoA0B%2BR%2FY%2FjD%2BEwoBoA1yT5v3PbdmCE8MAshhID%2FUMoDgzUYIBj0Cgi7ar4coAAAAASUVORK5CYII%3D\",\"sidebarURL\":\"https://www.facebook.com/desktop/fbdesktop2/?socialfox=true\",\"icon32URL\":\"data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAYAAABzenr0AAAACXBIWXMAAAsTAAALEwEAmpwYAAADbklEQVRYCc1Xv08UQRj99tctexAuCEFjRE0kGBEtLDSGqIWNxkYKbTAxNlY2JhaGWltNtNFeKgsKKxITK43/gCYW+IsoRhA4D47bH7fn9+bcvdm5JR7sefolC3Ozu9978+bNN7PayUv3HN3umdY0Y6IWBtSJ0HSTarXqTOiuTep6Lj+tdxAcA8RAgSmwdd2aCDs0clldYALb/FvgYVhjmfliVA2XpjEgWo0Attn42Z6WH1RFor5ehwo9XQIUZMoVn4qlCoVMSo62EvD8Kh0b3U2Xz43R2PBO6mUCGDlAf65V6MadZzT/rUimoccc2kYA4BfPHqJb105RzjJigKhRq9kEJUBIjgYVuXeL7SAI6eD+Abp5dTwVHOmEHxT50d8WBYJqSOdPj5BjW8gZR8UNqFR2xagx/65XFYaMH+BGWwiYpi4UkBPPLxTp9v1Z+lHc4DWvCQXWmIy6EjITgKowVd5Jjv7N3Hd6y5esigoOwpkJIAmMpZpLJGdiaaC4F0UmAj6bD84GCEwmB/qxMmRilmnwb/mpjAocHh4UEoNAt5NLZB7oy9OJo0PxqkAtePdhiSqunyC1LQUwWMPQaOr6GRre258Ajn4cP7KHcEXhsxpXbj+lT19X2TMNGTLVAcjcalS8gDwsQ2UOMhH4k8FkcrEn5E5ub2sKohxLK2VR77Hl9RUcsrgeRIEiVOT6z+tDbIeLy+vk+kGTCbXxycet6xhl//3f6bJEkdHYhA+mLtDIvoH4ieev5+juoxdk5+pjhALYEdXIpEB5w+NlSKSzqVQ/+H7IO6BLtl3fngGMiqhGJgIwlM6qpyUGFjySdk8m0Zg0ubeD7X9OIDEFajltRQgUJaUKx69tdgaQa0FMADuahZPMFtcEwNPm2hA7ZI5sK4aoE2NvYI+o8hkCIe7CwTv68zS0q9Dk5vpbm/8FXxitSzmMFHpsGj0wyLUheTwD2Y9fVgh1Ae0EPUgD9241ZEnld+v5kgnVZ/8fE0brVh5BK+1oCqKKF72Dk7HwBsssB/pklU1dfChy3S659H5+uelgIb+8WRv1/uGTV9Sdb5wJFlfW6fPCalMhwhSU1j2xKwKbP838GcOwJja4TqO0bjdmXxYTy1EYjFdCWoCEYZhseH/GDL3yJPHnuW6YmT7P1SlIA4768Hke4vOcsX8BE346lLHhDUQAAAAASUVORK5CYII=\", \"icon64URL\":\"data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAEAAAABACAYAAACqaXHeAAAAAXNSR0IArs4c6QAAAARnQU1BAACxjwv8YQUAAAAJcEhZcwAADsMAAA7DAcdvqGQAAAAadEVYdFNvZnR3YXJlAFBhaW50Lk5FVCB2My41LjEwMPRyoQAACNNJREFUeNrtm3tw1NUVxz/399hHHkgCaCBGEFEEREVFYFQcSoOKdkZay4z+4dDpYIsjHWx1WoTMhFi1gzBSpVgVGbU4U1sHfPESKODwEEnRYDFAICEIeZIQshs2u/v73ds/drMsyW7YLEkl2Z6Z32yy+9v7u+fc7znne8+5KzgvAjDunzlv0M13PjDZ6c4cARj0WhEoaZ1tOn3yq9XLf/tNU0O1D5Ad7wq/OpxpaXOL1j5uZAwuaGlVgwNBhULRm0XXBG6HZrlNa9uRrzfM+3DlgjIgGMsA7rl/XDdHOnNf9vosTfVuvTsaQhdkZ4iykh2rHtqydvkxwI58BhjTfv7MmP55E9/1nLNdfU15ACkVvoAaMCRvRPa+re9+DgTaPjMAx+DrJv3M67Mz+6LybWLb4NfTHhxzx31DDhZvOtqGAgNwWbjGICV9XQJB0e/KobcOP1i8qTzaAEYgaDtNU/V5A9hSaUFLuQEt2gVQSgml+j4CUAIppYgK/m0GkCjZ9xGAUNAu0LUhgJRAAAIVzwBSqVRQH4hlAClRKZAFhOgEASoFECBR8QwgUyQGdJT/B8HzCEiBNKhUJzEgBYIgQsTJAkohe9oFZHgHKvQoHtZ9K3tewfiixXABLdoFeuSSEmkF+PH4QTz7+M3o+ENptzvGtS36uSwmjMpAYF10XCllHCYoe84FlLS555Zs5jx6J6ahY+iCl98pJiDNS1hwSZop+cm91zJmxEBefGsPlu1AxKC67V3gf5oGlZSMuz6Dp2fdhWnoAEwaN5T5hsYLb+4hKB1dcgelFDpB8ifk8thDt3DO5+fZxRvxBV0IjQR0EB3KfD1GhJS0GZnnYuGcKTgdF9ZWx4/No/BJjUUrdtJqm4iL+K5SCmSAiWMHMevhcQzNzaa6ron5SzfQ7HeiaSKx+au4m6HupcJKSYZdZVI4dypuV2yo3zoql0VP3cOiFV/Q4jdiGkGhQFqMGpbJL346kbE3DEYIQWNTC39Ysp4Gr4HQtZDyiRhA0NlmSHZbRM7pr1H0m6lckeHqdGXG3jCYoqfupeC17bT49fNRXIFSNrkDTGbNGM9dtw1D10M1DI/Xx3NLP6OqETRdDy1eglPT4rqA7K56gCIrXfHCvHwGZqUnBMtR113FS/N+xHPLtuJpDa1mVobg0emjmX7vqEjsUErhaw1Q8Mo6yk4F0A1HeOW7kIlFx/u7jworRabLpmhuPjmD+iG7YNDrrhnIS09P5cW/buOeO67lkftvJt3tDE06PE7Qsnh++QYOHPOim86wcVUS0+whJug0ghTMmcK1V2eH8m2UHP++nrwhAyIwjiXDcrN5vXAGhqGhFBeMYUvJ0re2sPfgGTTDGUZrEogVopMgmGQWUCgMEWTBE5MZPSLngnGqas/w9j92s31fJfmThvPM7HyMMKRj+qgmOiBHSsnr73/B5r1V6A53KD4k3bFS8dNgckFQoWPxu1kTGDc6N7JqzR4ff/+smE+2H8FSLkx3FluLawhaG3n2iXwcppGoV/Hemt18tK0c3UwLIfUS2nVafBeQJNUXkAF+/dht3H37cKSUBC2bTTv+w98++YazPg1dT0NoIUhruoMd+2sJrtjI7381rQM3iCVrN33N++tL0c30xFNdp0GQeFRYQhcRIO0gv5xxE/fdfSO2bfNVSTmrPtxLZW0A3XSh6VporaLG1XQHu0pOU/TaOhY8+QAuZ3w6vHnnQd74536EkZ50wOsSE0zcugolbWZOG8GM/LGUVdSw6sM97D/UgGa60QxXzMJDZAq6yb7SJgr//CkLn5pOmsvR4Z5dxUdZ9t6XoKfFjNyXkqZjuoBUCpGgCyhp8eDdQ5k++UaWrdrMlr2VSFxoZlpE8YtNWGgGXx9ppnDZpxTMfTCS8gAOlJ5g8ds7kCItTIi6j6FqMVxAC2sV2RB1ekmLCTcNpH+myeyFH7BxT1Voopoe4RKJXQqhmxw45mXh0o/xeH0opThcXs2iv2wmoFyhAnbC4yX+3PgISMDShrA5XHGa3d9UITRniIeTfHASmsGhEz7mL/mI2TPv4sU3t+KzHAnu7JKpCosOkcSIICAB5hZE0OiRCM0Iwb0b6LPQdI5W+Zn/yucoYYayRk+16eK1xqRMDAHtA0r3lep0lNAjO8kfpCpMqpTF4xZEUqA7rIlOCiKpgADVWXc4FQwgEfHPB5AiByTixIDUCYJx+wJoqdIcJV5VOAWygEZcF7BToT2upFDKtuz2BrAtf8v3mju972cBJX2exso6ok6N64BhOtM11xXXPBz6v6340PcuO+DZfaJkzWqgqY3L64Bqaaz0ZV45Mkc308dG2kd97FLSaq4v317gazr5HeCLRoACFTxbfeBw+oDhWYYj4/rw+30H+rb/VMPxXQsbKnbuABqJOi4vogyRiRB5/XNvvz3zytFTDEf61eF9b0dCKTS36c4afymTsgLeQ9Ly13X/aYnzE1Uy6PV7679trNy1xe+tKwPqAH/0Vla0qw65gH7AFeG/Y3Uy9P45o0bm3PTIaplM6lTK9jWf/OBUyQcrpdXaTIyfsXQb9QcLaAn7vJd2vxY5XxBpo8pwDmgFGsLKx1oeh8OVmUUSLXUlrWZPzbdLag9v+BjUqfDzepKAyDDcZbznGHG+1NmqSKHpVlfbadJqLW+o2LHobNX+PUB1WPkfnHwYyTmX6lI7Lehr3F576NM/+T3V3wH17f2w1xkg2ggXuSvga6p8p+bgmpVKWpXAmVh+2AsNEKogdYYAJa0GT03J4obyf60HTgKe6PTTqw0QOpcQ3wXs4LlDZyq2FXrrS4uBmjDxuCw3G5eIgA46yeC5ho11pWsWW35PWTibBC4Xf+9eBLRPg0q2+s5UvHG6bMNqJYPHw7nXutxZYvIIiMoCSgbrPVX/fv7syS+3AKfC5MOmF4iRpP6RjrId8O5vrNhS1NpUWQLUholUr6muXEoatP3emrWNR9e/avk9R8P+HuxNypPkrk93pGdnK0VtXemaN6UdOHo55vdE5b/0NKx+K4AxtAAAAABJRU5ErkJggg==\", \"description\":\"Keep up with friends wherever you go on the web.\",\"author\":\"Facebook\",\"homepageURL\":\"https://www.facebook.com/about/messenger-for-firefox\"}");
>  
> +// comma seperated whitelist of providers that can install from their own

nit - "separate" - also, could this comment be a little clearer by saying something like "comma separated list of provider origins for providers that can ..."

@@ +1204,5 @@
>  
> +// comma seperated whitelist of providers that can install from their own
> +// website without user warnings
> +pref("social.whitelist", "");
> +// comma seperated list of directory websites that can install providers for

separated - and possibly also indicate this is a list of origins rather than pages (although this one is clearer given there is a default value for the pref)

@@ +1207,5 @@
> +pref("social.whitelist", "");
> +// comma seperated list of directory websites that can install providers for
> +// other sites e.g. AMO
> +pref("social.directories", "https://addons.mozilla.org");
> +pref("social.remote-install.enabled", false);

a comment here would be nice too - it almost implies that it overrides the above prefs.

::: browser/base/content/browser-social.js
@@ +214,1 @@
>      // Event must be fired against the document

this comment now looks wrong, as we are handling the event being fired against arbitrary elements.  Also, seeing we've already checked the instanceof above, both the comment and the check seem redundant.

@@ +217,5 @@
>  
>      // Ignore events fired in background tabs
>      if (targetDoc.defaultView.top != content)
>        return;
>  

The call to Social.canActivateOrigin has been removed, and IIRC, this was the only caller.  Should the function itself also be removed?

@@ +231,5 @@
>      Social.lastEventReceived = now;
>  
> +    let data = node.getAttribute("data-service");
> +    if (data)
> +      data = JSON.parse(data);

I wonder if we should guard against invalid json here, and Cu.reportError() and early exit if it fails?

@@ +232,5 @@
>  
> +    let data = node.getAttribute("data-service");
> +    if (data)
> +      data = JSON.parse(data);
> +    Social.installProvider(targetDoc.nodePrincipal.origin, node.baseURI, data, function(manifest) {

I'm a little confused by the need to pass both targetDoc.nodePrincipal.origin and node.baseURI - shouldn't they always refer to the same origin?  Also, is there a risk that evilsite.com will embed addons.mozilla.org in an iframe and fire an event on the iframe itself, meaning node.baseURI might be the addons site while targetDoc..origin will be evilsite.com?

Given SocialService names these params as "installOrigin" and "sourceURI", it's likely I am confused, so maybe some clarifying comments would be useful?

::: browser/base/content/test/social/browser_addons.js
@@ +33,1 @@
>      Services.prefs.clearUserPref("social.manifest.good");

looks like we need to clear social.whitelist too?

::: toolkit/components/social/SocialService.jsm
@@ +55,5 @@
>      }
>      return null;
> +  },
> +  getManifestPrefname: function(origin) {
> +    // Retrieve the builtin manifests from prefs

this seems to be no longer just "builtin" manifests, but also any providers previously installed.

@@ +71,5 @@
> +      }
> +    }
> +    // no existing pref, return a generated prefname
> +    let originUri = Services.io.newURI(origin, null, null);
> +    return originUri.host.replace('.','-');

is the host really suitable here?  Don't we need the entire origin just incase there are 2 providers from the same host but different ports/schemes?

Also, why the .replace() - I'd guess we don't want "my-site.com" and "my_site.com" to use the same pref value.

@@ +290,5 @@
>      });
>    },
>  
> +  getOriginActivationType: function(origin) {
> +    // XXX should use constants for activation type, which can be BUILTIN,

you should probably either just switch to constants, or commit to using fixed strings and either way, delete this comment.

@@ +300,5 @@
> +    let whitelist = Services.prefs.getCharPref("social.whitelist").split(',');
> +    if (whitelist.indexOf(origin) >= 0)
> +      return 'whitelist';
> +    // directory whitelisted? Assume it is reviewed/approved by the directory.
> +    // If someone tries to install from a directory that is not in our list, the

It's not obvious to me why it will fail.  Wont it just be treated as 'foreign'?  _manifestFromData doesn't seem to enforce what the comment implies.  The tests don't seem to cover this case (or indeed, many error cases at all)

@@ +335,5 @@
> +    let URI;
> +    if (type == 'directory') {
> +      // directory provided manifests must have origin in manifest
> +      if (!data['origin']) {
> +        Cu.reportError("SocialService.manifestFromData directory service provided invalid manifest origin.");

it's not so much "invalid" as "missing"...  Also, given the complexity in the type handling, I wonder if we shouldn't also fail if baseURI has a value in this branch?

@@ +348,5 @@
> +    // force/fixup origin
> +    data.origin = principal.origin;
> +
> +    // one of workerURL, sidebarURL, shareURL (when available) is required,
> +    // and must be same-origin

Isn't this comment and code reflecting bug 843862 rather than the current reality?  Do we really work correctly without a worker or without a sidebar?

@@ +356,5 @@
> +    if (!data['workerURL'] && !data['sidebarURL'] && !data['shareURL']) {
> +      Cu.reportError("SocialService.manifestFromData manifest missing workerURL, sidebarURL, shareURL, at least one required.");
> +      return null;
> +    }
> +    if (!data['name'] || !data['iconURL']) {

shouldn't these both be mandatory?  Is there a usecase for a provider with an icon but no name?  Why would a provider not supply an icon, and shouldn't the impact of no icon on our UI mean we should just insist on one?

Comment 15

[Shane Caraveo (:mixedpuppy)]

Attachment: install, whitelist and directory patch

patch implementing feedback

Comment 16

[Shane Caraveo (:mixedpuppy)]

Attachment: install, whitelist and directory patch

This version adds the activation tests from bug 838945.  They are actually important to the install functionality and belong here.  They also caught bugs in the install process which I fixed, as well as a couple minor bugs that have been included (note the chatbar and Social:ToggleNotifications changes).

Comment 19

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 719169 [details] [diff] [review]
install, whitelist and directory patch

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

::: browser/base/content/browser-social.js
@@ +203,5 @@
>    // in this window.
>    _activationEventHandler: function SocialUI_activationHandler(e) {
> +    let targetDoc;
> +    let node;
> +    if (e.target instanceof HTMLDocument) {

I think it would be great if here we could also enforce this event to have originated in the top-level document only (i.e., no iframes). The origin checks and isHandlingUserInput already does a good bit of prevention but unless there are no existing plans with partners about using an iframe for activation, I think it'd be good to start with this requirement for now. And then we can look into easing the requirement later if it becomes necessary.

::: toolkit/components/social/SocialService.jsm
@@ +289,5 @@
>        onDone(SocialServiceInternal.providerArray);
>      });
>    },
>  
> +  getOriginActivationType: function(origin) {

canActivateOrigin is gone but it's not clear to me if any of the installProvider() code automatically ties to the blocklist support added in bug 755126. If that's not the case, perhaps a good way would be to check it in this function and return "blocked" here to prevent installProvider to continue.

r+ if this is already happening automatically somewhere that I didn't notice (maybe with the usage of AddonInstaller/Wrapper), and with the iframe consideration above

Comment 20

[Shane Caraveo (:mixedpuppy)]

Attachment: install, whitelist and directory patch

patch addressing review feedback, along with added tests

Comment 21

[Shane Caraveo (:mixedpuppy)]

try at https://tbpl.mozilla.org/?tree=Try&rev=0b5ffc19ae17

Comment 22

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9a12955bf5d

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a9a12955bf5d

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Did this ever get security-review? Seems like it should, given that it adds a content-triggered install mechanism (also for things like parsing arbitrary content JSON).

Comment 25

[Daniel Veditz [:dveditz]]

Tagging Yvan to see if this was included in the existing SocialAPI code-review (bug 763941)

Comment 26

[:Gavin Sharp [email: [e-mail]]]

Any work related to that review significantly predates any of the work here, I think.

Comment 28

[u279076]

Is there any QA testing needed for this bug before we release?

Comment 29

[Mark Hammond [:markh] [:mhammond]]

It wouldn't hurt to check installation of multiple providers from a clean profile and the general interaction of those providers with each other and with Fx itself.

Comment 30

[u279076]

I've not encountered any regressions in provider activation and usage with Facebook, MSN Now, and Cliqz in the last week dogfooding on Firefox 22.