Closed Bug 1253741 Opened 4 years ago Closed 3 years ago

[kinto] Hook up Firefox Accounts

Categories

(WebExtensions :: Untriaged, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1253740

People

(Reporter: andy+bugzilla, Assigned: glasserc)

References

Details

(Whiteboard: [storage]triaged)

Hook up Firefox Accounts to chrome.storage.sync so that we store data for that user in that users bucket and not someone elses.
Summary: Hook up Firefox Accounts → [kinto] Hook up Firefox Accounts
Working on this now. Particularly, need to figure out how to use FxA as an auth server for Kinto on the server side, and how to set up integration tests for this configuration.
Priority: -- → P2
Whiteboard: [storage] → [storage]triaged
Hey Ryan, Natim we could probably use your help here, to see what's the best path forward
Flags: needinfo?(rhubscher)
Flags: needinfo?(rfkelly)
Basically there is an authentication policy plugin that let you authenticate on Kinto using Firefox Account: https://github.com/mozilla-services/cliquet-fxa/

You can try it on the development server there: http://natim.github.io/get_fxa_token/

You can use it with the Firefox Account development server by creating a client id and client secret here: https://oauth-stable.dev.lcip.org/console/clients

The you just need to use a bearer token.

Authorization: Bearer 1234abcd908...

If you want to store a bucket per user and a collection per addon, you will use the default_bucket plugin (so that each user have got an isolated bucket): http://kinto.readthedocs.io/en/stable/api/1.x/buckets.html#personal-bucket-default

Do not hesitate to reach to me if you need any information.
Flags: needinfo?(rhubscher)
Let's make sure we have a clear picture of the desired user experience and security model here.

On the client side: Since this is for `chrome.storage.sync`, do we want it to be intimately tied to the user's sync login state?  Does logging in to sync in the browser cause your `chrome.storage.sync` to start working automatically?  Does logging out of sync cause it to stop?  Will we ever expect a user to want to log in or out of this experience independently of sync?

On the server side: is there a dedicated Kinto endpoint that we'll be using to support this service, or is the storage space shared with other services?  I assume it accepts Bearer tokens for auth as described above.  Should we create a dedicated OAuth scope just for `chrome.storage.sync` data?

If I understand all the moving parts correctly, the best way forward it probably to:

* Define a dedicated OAuth scope for this, e.g. "sync:chrome-storage"
* Use the browser's sync login state to automagically get OAuth tokens with this scope, e.g. by calling [1] on desktop
* Pass that OAuth token to the Kinto client code for authentication

Unless we have a specific reason to need it, I'd suggest that we *don't* provision new OAuth client credentials for Kinto here, but rather re-use the browser's own FxA oauth client integration.

:michielbdejong, :natim, let me know if you'd like to get together to chat about this synchronously.

[1] https://dxr.mozilla.org/mozilla-central/source/services/fxaccounts/FxAccounts.jsm#1298
Flags: needinfo?(rfkelly)
Yes, that all sounds about right. So far, I've been using 'kinto' as the scope name in my PoC code[1], and this all seemed to work pretty much out of the box when :natim and I tried it against the FxA/Kinto dev servers.

[1] https://github.com/michielbdejong/gecko-dev/commit/f200a077b7bdb700d60ee0885c5aee169f137057#diff-0306de4a3aaf00775cc25304fffe8117R241
:markh, /cc FYSA
Thanks Ryan.

(In reply to Michiel de Jong [:michielbdejong] from comment #5)
...
> https://github.com/michielbdejong/gecko-dev/commit/
> f200a077b7bdb700d60ee0885c5aee169f137057#diff-
> 0306de4a3aaf00775cc25304fffe8117R241

I'm not sure this is the right bug to discuss this, but I see some of it in the above commit, so thought it worth bringing up. I think we should consider relatively tight integration with Sync itself, particularly its scheduler, logging and interactions with the main browser UI. For example, the browser exposes a couple of ways to "sync now", offers UI when the account is in a bad state, has a scheduler that knows to not Sync when in a bad state, some support for smart scheduling (eg, immediately after a wake or coming online, supports backoff/error intervals, etc) - which could be improved, but we might as well only improve it once! Logging integrates with Log.jsm and supports log file rotation, a log file viewer etc, we are working in techniques to making it easy for users to share their logs, and are looking into our own telemetry pings with relatively rich information from the client.

I don't think this would be particularly painful - we'd probably just want to tell Sync that a new "stub engine" exists - one that doesn't use existing Sync storage etc, yet still slots into desktop Sync. While the above is all referencing desktop Sync, I assume the same considerations would also apply for mobile - the choice of when and how often to Sync, detection of new states that might change the sync behaviour and account state, how to log, how to surface UI, etc, all seem best left under the control of the existing Sync code.

(To be clear, I'm not suggesting we want this to be a "legacy sync engine" - more that Sync itself should become less opinionated about how abstract "engines" do what they do, so new features like this can be slotted in a sane way)
I've been looking over Michiel's patch with the intent of revamping it and I'm a little stuck on how best to do "real" integration testing on it. I guess one option is to run against the Kinto dev server, but that risks failing the tests if the dev server ever goes down (or even when it's flushed at 7AM every day). Besides that, syncing is a background process and I'm not sure how best to expose its output to testing code. I tried looking at how FF Sync is tested and I think there's a complicated test framework in something called `tps`. Writing something like this for storage.sync seems like overkill, but maybe it's the only way. Alternately, maybe I could add something to the build scripts to make it pull down a real Kinto server and test against that? That seems like it would add a bunch of dependencies for anyone who wants to hack on Gecko. Any thoughts?
In general, Sync tests in 2 ways:

* It uses hack httpd.js servers in xpcshell tests - in effect we've reimplemented the sync http apis with varying degrees of sophistication depending on the test requirements. We do this exactly for the reasons above - standing up a "real" sync server for in-tree testing isn't really viable. I expect you are going to want some in-tree tests for this, so something similar sounds necessary.

* TPS is a test environment that we are slowing bringing back to life, and it runs against production servers. I think reinventing this for this requirement would be the wrong thing to do, but integrating it into the existing TPS sounds like the right thing - as I mention in comment 7, I expect users will want to see this as part of "sync" - mashing "sync now" should do the right thing, etc. IOW, TPS is trying to test the real user-facing aspects of Sync, and it seems difficult to argue that this shouldn't be part of that even if the underlying implementation if different.
Depends on: 1285315
OK. It seems like the tests start up a server and populate it with canned responses using httpd.js, and then points the syncing code at localhost:8888. That seems pretty reasonable and I can do that too. In my case, the actual syncing logic exists within a separate library, which I can also set up stubs/mocks for. That might be better for smaller-scale unit tests.

I agree that integrating this into the Sync framework is the right choice. I have been reading the sync code slowly but surely to try to figure out how much work this is going to be and how I'm going to do it. I think it makes sense for this work to happen in another bug. I have therefore created bug 1285835 to work on this. I am new to this part of the codebase but I will take a swing at this.
Sorry, I mean 1285315!
Assignee: nobody → eglassercamp
It seems like the majority of the work that this bug represents has been subsumed in bug 1253740. I am closing this one as a duplicate.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1253740
(In reply to Ethan Glasser-Camp (:glasserc) from comment #12)
> It seems like the majority of the work that this bug represents has been
> subsumed in bug 1253740. I am closing this one as a duplicate.
> 
> *** This bug has been marked as a duplicate of bug 1253740 ***

As such, this should now stop blocking bug 1220494.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.