Skip to content
This repository has been archived by the owner on May 10, 2019. It is now read-only.

restructure include.js so that it can be generic enough to cache and let sites self-host #3119

Closed
benadida opened this issue Mar 19, 2013 · 25 comments

Comments

@benadida
Copy link
Contributor

In particular, if we agree on the actual method calls that are supported, since everything is asynchronous, I believe we can build a generic include.js that:

  • generically stashes away any callback functions
  • generically sends all options to the relay frame
  • generically invokes the right callback based on instructions from the other side

e.g.

navigator.id.watch({
   loggedInUser: 'ben@adida.net',
   onlogin: ...,
   onlogout: ...,
   onCrazyCallback: ...
});

would stash the new "crazy callback", and when the internal message comes back as {"callback": "onCrazyCallback", params: [...]} then the generic include.js would invoke it.

@jaredhirsch
Copy link
Member

I'm super skeptical about letting RPs host an unversioned file that is central to Persona's functionality.

What's the goal? Is it improved response time? Can we discuss the motivation and seek out alternative tactics--maybe on the public list?

@benadida
Copy link
Contributor Author

The goal, from a clear potential partner we talked to today, is sites that don't want to include third-party JavaScript, ever. They have CSP rules in place to prevent it. It's a worthy goal, and we have seen these requests from people on the mailing list before.

We should try.

@callahad
Copy link
Contributor

A big win is dramatically reducing the scope of API changes that require a new include.js.

@seanmonstar
Copy link
Contributor

It's always been a sticky point for me, that I recommend front-end best
practices, which is never ever god never include 3rd party scripts,
since you give over the keys to your DOM, but then at the same time tell
people "to use Persona, include this script tag".

tl;dr - +1

@karlht
Copy link
Contributor

karlht commented Mar 20, 2013

Sean speaks for me on this issue. +1.

@jaredhirsch
Copy link
Member

What I'm trying to get at is, this is a fairly young product (still in beta). Letting RPs self-host makes it far slower and more complex for us to deploy API changes in the future.

Or maybe I'm the only one worried about this?

@benadida
Copy link
Contributor Author

No, @6a68 you're not the only one worried about this, I have generally pushed back on freezing that file for a while. But we now have real-world requests for it, so I'm just asking that we consider it, in particular if it can be done while retaining flexibility, i.e. you have to upgrade include.js to get access to new API calls, but old API calls will still work.

As per my description above, because we only make async procedural calls (we don't have a widget), it seems to me that it could be done. But of course, it won't be trivial: we have to capture the redirect-or-popup flow alternatives we're discussing in another issue, etc. So I think at this point this is an exploration, to determine feasibility.

@jaredhirsch
Copy link
Member

@benadida Sounds good. We could absolutely do it, almost every third-party provider does allow self-hosted embed snippets--I just want to make sure we're aware of the tradeoff with respect to supporting older versions of our own API when our files are currently unversioned. I'm happy to help with this.

@lloyd
Copy link
Contributor

lloyd commented Mar 20, 2013

No matter how well we restructure include.js, there will be cases where we must cause rps to update it. How do we deal with this?

Option 1: we build a single stable post message call which will force API update when out of date, meaning we won't asynchronously break websites, we'll just degrade their load time until they upgrade. (@shane-tomlinson and I discussed this last time we had this convo)

Option 2: we notify websites and require a response within a fixed time period, then break them.

Other options?

-- lloyd (thumb-typing)

On Mar 19, 2013, at 4:59 PM, Sean McArthur notifications@github.com wrote:

It's always been a sticky point for me, that I recommend front-end best
practices, which is never ever god never include 3rd party scripts,
since you give over the keys to your DOM, but then at the same time tell
people "to use Persona, include this script tag".

tl;dr - +1

Reply to this email directly or view it on GitHub.

@callahad
Copy link
Contributor

A lighter / more flexible include.js seems like a win regardless, so let's move on that in Q2.

Whether or not we go with Option 1 or Option 2 can probably be deferred until we get the include.js refactored.

@jaredhirsch
Copy link
Member

Option 3: file sends down an identifier, if it is out of date, we inject JS
into the page by xhr-ing over a string that's eval()ed in the RP context.
This is convenience over security, but thought I should mention it :-)

In fact, a more extreme version of this would let RPs self-host a very
minimal snippet, and xhr down the "real" include.js. This would ensure we
never need RPs to update their snippets.

On Wed, Mar 20, 2013 at 12:35 AM, Dan Callahan notifications@github.comwrote:

A lighter / more flexible include.js seems like a win regardless, so let's
move on that in Q2.

Whether or not we go with Option 1 or Option 2 can probably be deferred
until we get the include.js refactored.


Reply to this email directly or view it on GitHubhttps://github.com//issues/3119#issuecomment-15161713
.

@benadida
Copy link
Contributor Author

IMO, the major reason why a site is going to self-host our include file is for isolation reasons, i.e. they have a policy of not including third-party JavaScript. Sites that are knowledgeable enough to do that will also turn on CSP, which means no eval, no dynamic addition of script elements, hard block on sourcing third-party scripts.

In other words, I don't think @lloyd 's option 1, or @6a68 's option 3, are doable. We should try for the lighter-weight approach, and see if we can make that work with enough flexibility that we could have survived the last 6-12 months of include.js changes.

@seanmonstar
Copy link
Contributor

@6a68 sounds like option 3 has the exact same concerns I had originally commented, namely, that you give over the keys to your DOM.

@shane-tomlinson
Copy link

@benadida - CSP allows you to specify specific domains from which JS can be loaded using the script-src directive. Sites could still enable CSP but disable scripts from all sources other the domain and us. I cannot find anywhere in the spec mentioning dynamic addition of script elements, I have been trying to find out if something like this exists because it would hinder a performance enhancement that @fmarier and I are thinking about (dynamically loading bidbundle.js until it is needed).

@6a68 - using either eval or new Function(string) breaks CSP unless unsafe-eval or unsafe-inline directives are specified.

Dynamic updating of include.js could work - if the RP uses CSP but specifies the script-src directive, the RP could load a versioned include.js that can be updated at a later date if necessary. When the communication_iframe loads, it calls session_context which responds with the current include.js version. If the versions are different, upgrade.

We could ignore forced updates and hope that sites that are using CSP are also members of the updates mailing list. If we allow cached versions of include.js, we should announce, several months in advance, that an update is imminent.

@benadida
Copy link
Contributor Author

@shane-tomlinson yes, you could tweak the CSP policy to make inclusion from Persona possible, and to allow for eval, but then you would be losing much of the value of CSP. I don't think this is a win. I think the win is in building a generic include.js that doesn't require big sites to change their policies or open up potential gaps in their security perimeter.

You're absolutely right about the dynamic addition of script include, I overspoke, I only meant that, in a typical CSP setting, you can't source a third-party JS script, dynamic or otherwise.

To summarize my point: I think the biggest reason why people don't want to include our JS file is security perimeter definition / policy. It's not performance. So if we allow them to self-host but we don't solve the security perimeter / policy component of the problem, I think we're not solving the core issue.

@lloyd
Copy link
Contributor

lloyd commented Mar 20, 2013

Whose going to summarize the changes that landed in include.js over the last year so we can dream up a solution that would make us immune to changes for a year?

Also, if we go with option two, we must be extremely clear to websites what kind of checking they must commit to.

Finally, regardless of that - if we break a site, its always our fault from the site owners perspective.

-- lloyd (thumb-typing)

On Mar 20, 2013, at 2:35 PM, Ben Adida notifications@github.com wrote:

@shane-tomlinson yes, you could tweak the CSP policy to make inclusion from Persona possible, and to allow for eval, but then you would be losing much of the value of CSP. I don't think this is a win. I think the win is in building a generic include.js that doesn't require big sites to change their policies or open up potential gaps in their security perimeter.

You're absolutely right about the dynamic addition of script include, I overspoke, I only meant that, in a typical CSP setting, you can't source a third-party JS script, dynamic or otherwise.

To summarize my point: I think the biggest reason why people don't want to include our JS file is security perimeter definition / policy. It's not performance. So if we allow them to self-host but we don't solve the security perimeter / policy component of the problem, I think we're not solving the core issue.


Reply to this email directly or view it on GitHub.

@callahad
Copy link
Contributor

We have persona-notices for pushing out those kind of notices.

@shane-tomlinson
Copy link

@benadida - I agree w.r.t. RP security goals and eval, I most definitely do not want sites to enable 'unsafe-inline' or 'unsafe-eval'. A lack of eloquence hinders my ability to communicate.

My performance info is orthogonal to security minded RPs - I have been experimenting with loading bidbundle asynchronously. Bidbundle is a monster, loading it asynchronously saves mobile users 1 second on 3G and 2-3 seconds on 2G. These gains are twofold, first when visiting a site that uses the .watch API and again when the user opens the dialog. If CSP forbade dynamic insertion of first party script tags, then we would not be able to do this.

After a re-read, I think I understand the suggestion more clearly. The suggestion you are making is, basically make a include.js that will most likely not need to be changed, even if our API changes. A generic include.js could relay messages to a resource (like the communication_iframe) that can be changed at will to handle upgrades. Am I understanding this correctly?

@jaredhirsch
Copy link
Member

I'm happy to take the lead on this, unless somebody with more front-end experience wants to grab it. @benadida do we have specific timelines we need to hit?

@shane-tomlinson - Not to overload this conversation too much, but I think we could still make self-hosted Persona totally async by doing the google analytics queueing trick:

  1. in the head of the page, set up an array called, say, navigator.id.q, or navigator.idq (to avoid getting clobbered when we assign nav.id)
  2. dynamically draw the script tag that loads the RP's self-hosted include.js
  3. navigator.id.q queues up calls until watch is ready: navigator.id.q.push({method: 'watch', loggedInUser: 'foo@bar.com', onlogin: myLoginHandler, onlogout; myLogoutHandler})
  4. when Persona loads, its first action is to pop the queued-up calls off the nav.id.q and execute them
  5. after Persona has loaded, navigator.id.q calls are funneled straight to nav.id

edit: didn't quite finish what I was saying...

@lloyd
Copy link
Contributor

lloyd commented Mar 21, 2013

@6a68 can we start by looking at the types of changes that have been made to include.js and related resources (jschannel and winchan) in the last year, and try to understand the frequency we're making changes?

I'm hoping this hour of analysis can give us a clear idea of the types of changes we'll be able to make without forcing an include.js refresh, and those we can't - so we can reason about the risk of doing this.

I want to hop on board - but I'm still really concerned we're shooting ourselves in the foot here. I have high confidence that this will put us in a position where we cannot change things without breaking high visibility websites. And we might not even know that we are breaking them...

@seanmonstar
Copy link
Contributor

@shane-tomlinson that's what i understood also: an include.js that will pass all parameters of watch() and request() to something we control, like the communication_iframe or something, so that if we need to add or deprecate some in the future, we can, without changing the include.js

@ghost ghost assigned jaredhirsch Mar 21, 2013
@jaredhirsch
Copy link
Member

I'll generate some data around include.js changes over the past year and we can take this conversation to the list.

@jonasschneider
Copy link

Not to hijack this thread, but: isn't not having to trust any third party (including Mozilla) at all the main point of the project? If you are concerned about RP's that want to self-host include.js, I cannot imagine you reacting to sites that maybe don't want to outsource authentication, a hugely critical feature, to a completely uncontrolled yes/no API.

@lloyd
Copy link
Contributor

lloyd commented Mar 24, 2013

Everyone on this thread deeply wants to support self hosting. And nobody on this thread ever wants to break authentication for our users. This thread is about a pragmatic way to achieve both of those things.

You're absolutely right, that striving to support self hosting lets people trust us a little less, which is inline with the projects goals. And that's why this issue exists.

Thanks for speaking up!

On Mar 23, 2013, at 5:33 PM, Jonas Schneider notifications@github.com wrote:

Not to stumble into this thread, but: isn't having to trust any third party (including Mozilla) at all the main point of the project? If you are concerned about RP's that want to self-host include.js, I cannot imagine you reacting to sites that maybe don't want to outsource authentication, a hugely critical feature, to a completely uncontrolled yes/no API.


Reply to this email directly or view it on GitHub.

@callahad
Copy link
Contributor

Hi! To help us better focus, I'm "closing" all issues that have been open for more than six months. These have been tagged "cleanup-2014" so that we can go back and review them in the future.

For more information, check out this thread: http://thread.gmane.org/gmane.comp.mozilla.identity.devel/7394

If you believe this bug is still a major issue for you, please comment, submit a pull request, or discuss it on our mailing list: https://lists.mozilla.org/listinfo/dev-identity

Sorry for GitHub notification spam!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants