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)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
Tracking Status
firefox48 --- fixed

People

(Reporter: mgoodwin, Assigned: johannh)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Depends on: 1257533
Assignee: mgoodwin → jhofmann
Attachment #8743283 - Attachment is obsolete: true
Attachment #8743314 - Flags: review?(mgoodwin)
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+
Comment on attachment 8743314 [details] [diff] [review]
Make services.kinto.clock_skew_seconds non-absolute

Maybe MattN? :)
Attachment #8743314 - Flags: review?(MattN+bmo)
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+
> 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*.
See Also: → 1266235
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.
> 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.
> 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.
Attachment #8743314 - Attachment is obsolete: true
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+
Keywords: checkin-needed
See Also: → 1040741
https://hg.mozilla.org/mozilla-central/rev/4fa5909cf1e7
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.