1595635 - listAttachedOAuthClients should use a server timestamp.

Status: RESOLVED FIXED

(Firefox :: Firefox Accounts, defect, P3)

Description

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

In bug 1548404 we are changing listAttachedOAuthClients to return the last access time of the client expressed as "days ago". This is landing using Date.now(), but clock skew means this might be wrong. We should calculate this based on the timestamp in the server response.

Comment 1

[Chaitanya Mittal]

Hey Mark,

I'm new here and would love to help out with this. Already have gone through bug 1548404 and the discussion that followed. Do you have any other pointers for me?

Comment 2

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

Hey Chaitanya, that's great news!

I assume you've already gone through https://developer.mozilla.org/en-US/docs/Introduction. The bug should be relatively easy, but it will be complicated by the structure of FxAccountsClient.jsm - you will probably want the attachedClients() method to return all headers, from which you can parse an X-Timestamp header for the timestamp of the server - however, that uses the internal _request method, which is used by many other functions which do not want the headers. So maybe something like a new _requestWithHeaders method, which is called by _request() (but which just ignores the headers) and attachedClients() calls that. Or maybe _requestWithTimestamp and do the timestamp parsing in that module? We'll also need some basic tests.

Good luck, and let me know if you get stuck!

Comment 3

[Chaitanya Mittal]

Yes Mark, I have gone through the introductory resources. Just downloading the source and building as of now, but it sounds like adding another method might be a good fit for this problem.
Let me get back to you.
P.S. could you help me out with what kind of tests would be required for such a change?

Thanks.

Comment 4

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

I think modifying the test at https://searchfox.org/mozilla-central/rev/8bc24752246aeac8a9aed566cf1caccf88d97d11/services/fxaccounts/tests/xpcshell/test_accounts.js#1607 is probably enough (and would be necessary anyway!). We probably would want to fall back to a local time if the server didn't supply the correct header, so duplicating part of this test for that case might also make sense.

Comment 5

[Chaitanya Mittal]

I will keep that in mind.
Had a small doubt, my OS(Parrot) is not supported by the bootstrap script can I just download the source and run /mach bootstrap?

Comment 6

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

(In reply to Chaitanya Mittal from comment #5)

Had a small doubt, my OS(Parrot) is not supported by the bootstrap script can I just download the source and run /mach bootstrap?

I seriously doubt it :(

Comment 7

[Chaitanya Mittal]

Hey Mark,

Hit some roadblocks on the way, sorry for the delay. I have downloaded the source through the mercurial bundle and init my local working repository. I am unable to build because the bootstrap is incomplete. I will let you know when I make progress.

Comment 8

[Martin Sand]

(In reply to Mark Hammond [:markh] [:mhammond] from comment #2)

... method to return all headers, from which you can parse an X-Timestamp header for the timestamp of the server
From where do I get the timestamp of the server?

Comment 9

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

The response will have an X-Timestamp header which is the server timestamp.

Comment 10

[Zoltán Szatmáry]

Attachment: Bug 1595635 - FxA's device list now uses the server time for last accessed. r=markh

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f0f53d27ebd
FxA's device list now uses the server time for last accessed. r=markh

Comment 12

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/1f0f53d27ebd