Closed Bug 960887 Opened 6 years ago Closed 6 years ago

Use verbatim api_endpoint as returned from tokenserver

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: rfkelly, Assigned: ckarlof)

References

Details

(Whiteboard: [qa+])

Attachments

(1 file)

The tokenserver returns an opaque "api_endpoint" url which the client is supposed to use directly to access the service.  It makes no guarantees about the internal structure of this url.

Firefox currently appears to be parsing this URL and re-constructing its own version.  So if the tokenserver sends an api_endpoint that looks like this:

   https://sync1.dev.lcip.org/1.5/8

Then Firefox re-interprets it as a sync1.1 clusterURL and tries to sync at:

   https://sync1.dev.lcip.rg/1.1/8

(See Bug 960816 for a concrete example of this happening)

I guess this is because that's how the internals of the current sync client code work, e.g clusterURL doesn't include the path component.  But it's technically against the spec we're committing to in the new server infrastructure.

Ideal solution:  use the url verbatim as returned by tokenserver

Workable temporary solution:  remember the version number component from the url and put it back in as you found it
OS: Windows 7 → All
Hardware: x86_64 → All
Whiteboard: [qa+]
Duplicate of this bug: 964925
Blocks: 963251
Blocks: 964922
No longer blocks: 963251
Comment on attachment 8367684 [details] [diff] [review]
0001-prepare-for-storage-endpoints-return-by-1.5-token-se.patch

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

lgtm

::: services/sync/modules/browserid_identity.js
@@ +481,5 @@
> +          let endpoint = token.endpoint;
> +          // For Sync 1.5 storage endpoints, we use the base endpoint verbatim.
> +          // However, it should end in "/" because we will extend it with
> +          // well known path components. So we add a "/" if it's missing.
> +          if (endpoint.charAt(endpoint.length-1) !== "/") {

if (!endpoint.endsWith("/")) {

::: services/sync/modules/service.js
@@ +154,5 @@
>  
>      return Utils.catch.call(this, func, lockExceptions);
>    },
>  
> +  get userBaseURL() this._clusterManager ? this._clusterManager.getUserBaseURL() : null,

Multi-line with early return would be clearer, I think.
Attachment #8367684 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/integration/fx-team/rev/a76b3c746743
Assignee: nobody → ckarlof
Status: NEW → ASSIGNED
Re-landed since this patch wasn't the bustinator (I ran xpcshell locally).

https://hg.mozilla.org/integration/fx-team/rev/a4638840181d
Target Milestone: --- → mozilla29
https://hg.mozilla.org/mozilla-central/rev/a4638840181d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.