Closed
Bug 1264914
Opened 8 years ago
Closed 8 years ago
Make the value of services.kinto.clock_skew_seconds non-absolute
Categories
(Cloud Services :: Firefox: Common, defect)
Cloud Services
Firefox: Common
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: mgoodwin, Assigned: johannh)
References
Details
Attachments
(1 file, 2 obsolete files)
5.65 KB,
patch
|
MattN
:
review+
|
Details | Diff | Splinter Review |
It would be helpful if we could give the user more detailed information than "your clock appears to be wrong" when reporting TLS errors. To do this, we need the observed clock skew value to be signed. See bug 712612 for some context.
Assignee | ||
Updated•8 years ago
|
Assignee: mgoodwin → jhofmann
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8743283 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8743314 -
Flags: review?(mgoodwin)
Reporter | ||
Comment 3•8 years ago
|
||
Comment on attachment 8743314 [details] [diff] [review] Make services.kinto.clock_skew_seconds non-absolute Review of attachment 8743314 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. You should also get a review from a module peer.
Attachment #8743314 -
Flags: review?(mgoodwin) → review+
Assignee | ||
Comment 4•8 years ago
|
||
Comment on attachment 8743314 [details] [diff] [review] Make services.kinto.clock_skew_seconds non-absolute Maybe MattN? :)
Attachment #8743314 -
Flags: review?(MattN+bmo)
Comment 5•8 years ago
|
||
Comment on attachment 8743314 [details] [diff] [review] Make services.kinto.clock_skew_seconds non-absolute Review of attachment 8743314 [details] [diff] [review]: ----------------------------------------------------------------- If we're going to use this for a purpose other than kinto code we should consider renaming the pref. The later we rename the harder it is to do so. ::: services/common/tests/unit/test_kinto_updater.js @@ +127,5 @@ > + > + yield updater.checkVersions(); > + > + // How does the clock difference look? > + endTime = Date.now(); Is this actually doing something? It looks unused to me. @@ +130,5 @@ > + // How does the clock difference look? > + endTime = Date.now(); > + clockDifference = Services.prefs.getIntPref(PREF_CLOCK_SKEW_SECONDS); > + // we previously set the serverTime to Date.now() + 10000 ms past epoch > + do_check_eq(clockDifference <= 0 && clockDifference >= -10, true); Use `do_check_true` if you're just passing `true` as the 2nd argument.
Attachment #8743314 -
Flags: review?(MattN+bmo) → review+
Comment 6•8 years ago
|
||
> If we're going to use this for a purpose other than kinto code we should consider renaming the pref. > The later we rename the harder it is to do so. I think it goes beyond that particular pref, so I created a dedicated Bug 1266235 to add a `*.blocklist.*` prefix to all prefs that are manipulated by these blocklist clients. Also I suggest there that we replace the word *kinto* by something more generic like *storage*.
Comment 7•8 years ago
|
||
I guess I'm saying that if we are treating this pref as the canonical clock skew number in Firefox then it shouldn't be under a storage/kinto namespace. For example, it should be under the services or toolkit pref namespace.
Comment 8•8 years ago
|
||
> it should be under the services or toolkit pref namespace.
Yes for that particular use-case it makes sense since the TLS error is not fully related to blocklists.
IMO the idea of prefixing remains relevant though for server url, bucket name, last update etc.
Assignee | ||
Comment 9•8 years ago
|
||
> Is this actually doing something? It looks unused to me. Yup, death by copy and paste. > I guess I'm saying that if we are treating this pref as the canonical clock skew number in Firefox then it shouldn't be under a storage/kinto namespace. For example, it should be under the services or toolkit pref namespace. So I don't know much about the pref system, but doesn't the name also convey a sort of ownership/origin? People might use this pref in a better way if they know this comes from the Kinto pings.
Assignee | ||
Comment 10•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8743314 -
Attachment is obsolete: true
Comment 11•8 years ago
|
||
Comment on attachment 8743713 [details] [diff] [review] Make services.kinto.clock_skew_seconds non-absolute Review of attachment 8743713 [details] [diff] [review]: ----------------------------------------------------------------- Were you wanting another review or did you mean to set checkin-needed? (In reply to Matthew N. [:MattN] (behind on reviews) from comment #5) > If we're going to use this for a purpose other than kinto code we should > consider renaming the pref. The later we rename the harder it is to do so. To be clear, that doesn't need to block this bug as this bug isn't changing the usage but it should be considered for whatever non-kinto consumer wants to use it. (In reply to Johann Hofmann [:johannh] from comment #9) > So I don't know much about the pref system, but doesn't the name also convey > a sort of ownership/origin? People might use this pref in a better way if > they know this comes from the Kinto pings. Well either it's data that can be relied upon for clock skew or it's not. If it is, I think it's better to make that clear so people don't reinvent the wheel thinking it's a private value. Using a pref specific of another specific module normally looks like an abuse of that pref (similar to using a private API of another module). I'm not sure how knowing it's from kinto would affect its use.
Attachment #8743713 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 12•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=25b66b14f924
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/4fa5909cf1e7
Keywords: checkin-needed
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4fa5909cf1e7
You need to log in
before you can comment on or make changes to this bug.
Description
•