April King [:April] Reporter
	Description • 6 years ago

In the current TLS status API, `keaGroupName` and `signatureSchemeName` return the string `"none"` if they are unset. `"none"` is kind of a magic word that doesn't really have any technical meaning, and makes it hard to test for truthiness.

https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/addons/SecurityInfo.jsm#140

My proposal:

If either `keaGroupName` or `signatureSchemeName` are set to `"none"` by `SSLStatus`, set their value to either `null` or the empty string in what is returned by the API.

The first option is more semantically correct, the latter keeps the type the same, but both are easier to test for truthiness.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 1 • 6 years ago

I'm not sure I'm convinced this is an issue.  I'm also unsure about testing this value.  If I'm checking for "none", and someone changes the platform to something else...in any case will put up a patch.

	marius.santa
	Comment 3 • 6 years ago

Since these are possible uplift candidates for 62, can you please provide some STR and the proper webextension(if required)? So that we can get ahead with testing as per when the bug will be fixed.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 4 • 6 years ago

I'm still debating whether/how to automate a test on this.  It is certainly not something easily testable from QA.

	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 5 • 6 years ago
mozreview-review

Comment on attachment 8989253 [details]
Bug 1471959 - leave values undefined if value is none,  rpl

https://reviewboard.mozilla.org/r/254306/#review261912

To be honest I'm not super thrilled by this, it still seems to me that "normalizing 'none' to null/undefined" could be easily done from the extension code, especially given that the implementation is mostly just returning the strings that the internal implementation is giving us and that we don't have any automated test for the property that we are going to "normalize".

Having said that, I completely trust Shane judgment and I'll leave to him the final choice if this is a change that we absolutely want to land.

	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 6 • 6 years ago
mozreview-review

Comment on attachment 8989253 [details]
Bug 1471959 - leave values undefined if value is none,  rpl

https://reviewboard.mozilla.org/r/254306/#review261916

::: commit-message-6a922:1
(Diff revision 1)
> +Bug 1471959 - leave values undefined if value is none, r? rpl

Nit, let's mention somewhere in the commit message that with "values" we are referring to the `keaGroupName` and `signatureSchemeName` properties that are part of the webRequest API.

	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 8 • 6 years ago
mozreview-review

Comment on attachment 8990291 [details]
Bug 1471959 - leave keaGroupName and signatureSchemeName undefined if value is none,

https://reviewboard.mozilla.org/r/255324/#review262174

	Pulsebot
	Comment 9 • 6 years ago

Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/14e28f7332c7
leave keaGroupName and signatureSchemeName undefined if value is none, r=rpl

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 10 • 6 years ago

Comment on attachment 8990291 [details]
Bug 1471959 - leave keaGroupName and signatureSchemeName undefined if value is none,

Approval Request Comment
[Feature/Bug causing the regression]: 1322748
[User impact if declined]: api cleanup before api hits release, would only affect extension devs
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: minor fix
[String changes made/needed]: none

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 11 • 6 years ago

Some way of testing or verifying would be nice before uplift, even for a minor fix. It seems odd to say, we can't test it, and don't have any automated tests, so let's ship it faster.

	Bogdan Tara[:bogdan_tara | bogdant]
	Comment 12 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/14e28f7332c7

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 13 • 6 years ago

Comment on attachment 8990291 [details]
Bug 1471959 - leave keaGroupName and signatureSchemeName undefined if value is none,

Well, let's see how this does on beta 8, since this is a new API, maybe extension authors who are commenting in the meta bug 1322748 will be testing with beta.

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/cb0560aa6642

	Vlad Jiman
	Comment 15 • 6 years ago

If manual testing is not needed please set the “qe-verify -“ flag, otherwise please provide some STR and extension if needed, in order to validate the bug.

Thanks!

	Will Bamberg [:wbamberg]
	Comment 16 • 6 years ago

These properties weren't documented before, I guess because they weren't in the schema. I've added them to https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/webRequest/SecurityInfo, and marked them optional, which implicitly indicates that they are undefined if not specified, which I think is the truth.

I just used the description from the schema: I didn't think it was very helpful but wasn't able to come up with anything better, even after a bit of digging. It seems like they are internal names, rather than names from any standard. But I'd be very happy to accept suggestions for improving this.

Anyway, please let me know if this covers it or we need anything else.

	April King [:April] Reporter
	Comment 17 • 6 years ago

This works for me, thank you so much everyone.  :)