949260 - sync id manager should get .identity dynamically

Status: VERIFIED WONTFIX

(Cloud Services :: Server: Firefox Accounts, defect)

Description

[Brian Warner [:warner :bwarner]]

Attachment: service.diff

ckarlof wrote this patch, saying:

"Refactored the way that the sync id manager is instantiated to do it dynamically depending on whether we are using fx accounts; fixed a couple of tests"

Comment 1

[Chris Karlof [:ckarlof]]

Attachment: Update patch to remove whitespace differences

Comment 2

[Chris Karlof [:ckarlof]]

This patch is a simple way to enable us to (safely) dynamically set the (single) Sync identity manager currently stored in Status._authManager. Previously, Service.identity would refer to a statically fetched and cached instance of the _authManager at Sync initialization in Status. This patch introduces a Service.identity getter that dynamically returns Status._authManager. There are probably more elegant solutions to this problem, but this is likely one of the lowest impact ones.

Comment 3

[Chris Karlof [:ckarlof]]

The reason we want to do this is so we can dynamically set Status._authManager to either Sync Identity module or the new one that supports FxA after Sync has initialized.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8346877 [details] [diff] [review]
Update patch to remove whitespace differences

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

You should probably inline identity() in modules/stages/cluster.js.

But this patch doesn't do what it seems to. If the identity changes at runtime, the server URLs and cached keys won't.

You need to *at least* call service._updateCachedURLs, invalidate CollectionKeys, etc. if the return value of .identity will ever change.

At that point you might as well not do this patch, because you're already having to proactively respond to the change. Is there a reason you chose to go down this path instead of re-initing Sync when there's a new identity?

Comment 5

[Chris Karlof [:ckarlof]]

I agree this might be a bigger hammer than we need, and we're aware of the issues you raise. We'll just do Service.identity = Status._authManager = <myNewIdentity> when we swap out the Sync Identity module.

Comment 6

[James Bonacci [:jbonacci]]

OK.