[tor-bugs] #30237 [Applications/Tor Browser]: Tor Browser: Improve TBB UI of hidden service client authorization

[Tor Bug Tracker & Wiki]

#30237: Tor Browser: Improve TBB UI of hidden service client authorization
-------------------------------------------------+-------------------------
 Reporter:  asn                                  |          Owner:  mcs
     Type:  defect                               |         Status:
                                                 |  needs_revision
 Priority:  Medium                               |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  TorBrowserTeam201911, network-team-  |  Actual Points:
  roadmap-september                              |
Parent ID:  #30000                               |         Points:
 Reviewer:                                       |        Sponsor:
                                                 |  Sponsor27-must
-------------------------------------------------+-------------------------
Changes (by pospeselr):

 * keywords:  TorBrowserTeam201911R, network-team-roadmap-september =>
     TorBrowserTeam201911, network-team-roadmap-september
 * status:  needs_review => needs_revision


Comment:

 Review!

 == tor-button:

 {{{
   let cmd = "onion_client_auth_add " + hsAddress + " " + keyType + ":"
             + b64PrivateKey;
 }}}

 I think we should prefer using the new template literal syntax here (and
 in general) over string concatenation:

 {{{ let cmd = `onion_client_auth_add ${hsAddress}
 ${keyType}:${b64PrivateKey}`; }}}

 Why the comma operator here? Having looked up the rules for comma operator
 in JS, this ''looks'' right but I'd rather not have to do that :)

 {{{
   let dest = (controlPortInfo.ipcFile)
                          ? "unix:" + controlPortInfo.ipcFile.path
                          : controlPortInfo.host + ":" +
 controlPortInfo.port,
       maybeController = tor.controllerCache[dest];
 }}}

 Ayway, using template literal syntax:

 {{{
   const dest = (controlPortInfo.ipcFile)
                ? `unix:${controlPortInfo.ipcFile.path}`
                : `${controlPortInfo.host}:${controlPortInfo.port}`;
 }}}

 So this whole section seems a bit too long/complicated to use the
 conditional operator:

 {{{
       maybeController = tor.controllerCache[dest];
   return (tor.controllerCache[dest] =
            (maybeController && maybeController.isOpen()) ?
              maybeController :
              tor.controller(controlPortInfo.ipcFile, controlPortInfo.host,
                             controlPortInfo.port,
 controlPortInfo.password,
                             onError));
 }}}

 How about something like this?

 {{{
   const maybeController = tor.controllerCache[dest];
   if (!maybeController || !maybeController.isOpen()) {
      tor.controllerCache[dest] = tor.controller(controlPortInfo.ipcFile,
 controlPortInfo.host,
                            controlPortInfo.port, controlPortInfo.password,
                            onError));
   }
   return tor.controllerCache[dest];
 }}}

 == tor-browser:

 So my preference here would be to move the new
 TorOnionServicesAuthPromptHelper object, the new css and the new xul into
 separate js/css/xul.inc files in some separate component, similar to how
 SecurityLevel and TorPreferences are setup.

 First of all, because browser.js and browser.css are already monster
 files. Secondly, I think (though those with actual experience re--basing
 would need to confirm) that having minimal functionality actually in
 vanilla Firefox sources is a good idea to make updating to ESR's less
 painful. And third it makes working with the added functionality a bit
 easier since you can just open only code relevant to the component without
 necessarily needing to have all of browser.js and friends open.

 So, if I had my way, very little apart from init/uninit's and
 module+script includes would need to be added to vanilla Firefox sources
 to get the additional Tor-Browser functionality. That said I don't
 actually know if this pattern is a good idea or not in practice.

 Do you have any opinions on this **acat**, **sysrqb**, **gk**?

 === TorOnionServicesAuthPrompt

 In general this looks good. I'd like to see a few things that are more
 stylistic than functional:
  - prefer const over let where possible
  - storing names (element ids, message ids, etc) in an object and
 referring to them by symbol, rather than string literals (I would prefer
 to get a compiler error when mistyping a symbol name, rather than a
 runtime error when mistyping a string literal name)

 === tab-content.js

 I'd prefer to see the addMessageListener encapsulated in a function on
 TorOnionServicesAuthPromptHelper for code locality reasons mentioned
 above.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/30237#comment:33>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online