Opened 10 months ago

Closed 2 weeks ago

Last modified 9 days ago

#30237 closed enhancement (implemented)

Tor Browser: Improve TBB UI of hidden service client authorization

Reported by: asn Owned by: mcs
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: network-team-roadmap-september, TorBrowserTeam202001R network-team-roadmap-2020Q1, 9.5a5
Cc: gaba, pospeselr, tbb-team, sysrqb Actual Points: 16.2
Parent ID: #30000 Points: 17
Reviewer: pospeselr, sysrqb Sponsor: Sponsor27-must

Description

This is the parent ticket for Sponsor27 Objective1 Activity1 about improving the UI of client authorization.

This used to be #14389, but it contained too many network-team-specific information so I repurposed #14389 to be about the network-team side of things, and please use this ticket for the Tor Browser side of things.

Resources about setting up client auth:
https://2019.www.torproject.org/docs/tor-onion-service.html.en#CookieAuthentication
https://lists.torproject.org/pipermail/tor-project/2019-January/002180.html
and https://github.com/torproject/tor/blob/7741b21d0e3afbfc6d60a852fce6992724c4ae71/doc/tor.1.txt#L3021
and https://github.com/torproject/tor/blob/7741b21d0e3afbfc6d60a852fce6992724c4ae71/doc/tor.1.txt#L1122

Child Tickets

Attachments (12)

30237-1.png (207.7 KB) - added by antonela 10 months ago.
30237-2.png (214.2 KB) - added by antonela 10 months ago.
30237-3.png (188.1 KB) - added by antonela 10 months ago.
30237-1.1.png (198.3 KB) - added by antonela 10 months ago.
30237-2.1.png (269.6 KB) - added by antonela 10 months ago.
http-auth.png (557.1 KB) - added by antonela 9 months ago.
client-auth-notification-mockup-1.png (102.2 KB) - added by mcs 9 months ago.
30237-prompt.1.png (183.6 KB) - added by antonela 9 months ago.
30237-xhtml.1.png (192.8 KB) - added by antonela 9 months ago.
30237-prompt.2.png (169.9 KB) - added by antonela 9 months ago.
client auth prompt rev1.png (56.1 KB) - added by mcs 8 months ago.
client auth prompt rev2.png (44.5 KB) - added by mcs 3 months ago.

Change History (70)

Changed 10 months ago by antonela

Attachment: 30237-1.png added

Changed 10 months ago by antonela

Attachment: 30237-2.png added

Changed 10 months ago by antonela

Attachment: 30237-3.png added

comment:1 Changed 10 months ago by antonela

Hi, I'm working back on this ticket. I listed some user stories to make sure that we are handling these various user flows with the implementation:

As a user, I want to access to an authenticated .onion. I type the .onion address at the URL bar, and I get a user/password prompt. I fill the user/password field to access the onion website. I succeed.

This UI will looks like

https://trac.torproject.org/projects/tor/raw-attachment/ticket/30237/30237-1.png

As a user, I want to access to an authenticated .onion. I type the .onion address at the URL bar, and I get a user/password prompt. I fill the user/password field to access the onion website. I fail.

For users who cancel the prompt or fail with the credentials, the default ux is very sad. Could we think together about how we can allow users to recover from those situations? Is a password error message like "Enter a valid password" doable? What happens if users enter a non-existent user name in the user field? Are these situation able to validate? Is that part of this scope?

https://trac.torproject.org/projects/tor/raw-attachment/ticket/30237/30237-2.png


As a recurrent user, I want to save the authenticated .onion credentials. I type the .onion address at the URL bar, and a get a password prompt. I succeed. I want to save these credentials in the browser password manager.

As suggested in #14389, we could explore how to use default Firefox save password flow to allow users to save these credentials. After the user succeed on accessing the .onion, the password saving will prompt.

https://trac.torproject.org/projects/tor/raw-attachment/ticket/30237/30237-3.png

Last edited 10 months ago by antonela (previous) (diff)

Changed 10 months ago by antonela

Attachment: 30237-1.1.png added

Changed 10 months ago by antonela

Attachment: 30237-2.1.png added

comment:2 Changed 10 months ago by antonela

Following the specs, we don't have user/password, end-users just have a private key. So, I updated the first user story's UI.

https://trac.torproject.org/projects/tor/raw-attachment/ticket/30237/30237-1.1.png

For the second user story, we may want to consider a different kind of validation scenarios. Following Firefox Photon UI, user input errors should be exposed at the UI as an in-line validation. And, system errors should be presented with a message bar. Both use cases are rendered here

https://trac.torproject.org/projects/tor/raw-attachment/ticket/30237/30237-2.1.png

We should work on this validation. I made a first approach for this content, please feel free to suggest/add/remove any of this lines:

Type Error UI Message
User Input incomplete form Please, enter your private key
User Input over/under character or word count Must have 52 characters
System Error misspelled errors The private key is wrong. Try again.
System Error connectivity issues There was an error. Check your internet connection and try again.
System Error failure to load There was an error handling your request. Try again.

Which else error scenario should we consider?

For the 3rd user story, we don't have a proper password, so saving the privkey in the password manager seems weird. Perhaps, we can think about it for the next iteration.

comment:3 in reply to:  2 ; Changed 10 months ago by asn

Replying to antonela:

Following the specs, we don't have user/password, end-users just have a private key. So, I updated the first user story's UI.

Looks good. Not sure how we can do better than "Private Key" but perhaps we should. Perhaps we can write "Personal key" or "key" and then have more info in a "?" box?
No idea...

We should work on this validation. I made a first approach for this content, please feel free to suggest/add/remove any of this lines:

Type Error UI Message
User Input incomplete form Please, enter your private key
User Input over/under character or word count Must have 52 characters
System Error misspelled errors The private key is wrong. Try again.
System Error connectivity issues There was an error. Check your internet connection and try again.
System Error failure to load There was an error handling your request. Try again.

Which else error scenario should we consider?

The first two errors can indeed be detected and IMO should be detected.

The misspelled errors can be detected (by checking whether we could decrypt the descriptor with the key), but depending on how communication channel between TB<->Tor works, we might not learn the result on time to keep the auth dialog open. So we would have to respawn the auth dialog to throw the error. Does this make sense, and is it ok?

Same for connectivity issues and failure to load but perhaps this should be handled the same way as #30025?

comment:4 Changed 10 months ago by gk

Keywords: TorBrowserTeam201905 added; TorBrowserTeam201904 removed

Moving tickets to May

comment:5 in reply to:  3 Changed 9 months ago by antonela

Replying to asn:

Looks good. Not sure how we can do better than "Private Key" but perhaps we should. Perhaps we can write "Personal key" or "key" and then have more info in a "?" box?

Private Key seems the most appropriate term for what users should input there. I like the idea of having a ?. We should figure out which content we would like to have at the ?. I think we should link to something like support.torproject.org/onionservices/auth where end-users will have information about what the key is, how they can get it.

The misspelled errors can be detected (by checking whether we could decrypt the descriptor with the key), but depending on how communication channel between TB<->Tor works, we might not learn the result on time to keep the auth dialog open. So we would have to respawn the auth dialog to throw the error. Does this make sense, and is it ok?

Yes, exactly. Those two different kind of errors are defined by how the UI react to it. For System Errors we must to reload the auth dialog. Users will click Done and then the dialog box will reload with the error.

comment:6 Changed 9 months ago by mcs

Status: newneeds_information

The mockups from comment:2 show a prompt that is contained entirely within the content area. How concerned should we be about the "line of death" issue (https://textslashplain.com/2017/01/14/the-line-of-death/)? It seems like a bad idea to implement a prompt that any site could easily spoof, but there are tradeoffs to consider.

This question came up as Kathy and I looked at various options within the Firefox codebase for implementing the client auth prompt. We might be able to use a doorhanger that includes an arrow that overlaps the chrome area (thus avoiding the "line of death" problem). But doorhangers within Firefox are designed for optional interactions and entering a key for client auth is not optional.

We could use the prompt service (which is what HTTP basic auth uses), but the prompts that are available to us are not very flexible. It might be a lot of work to achieve the look we want; for example, I am not sure how to implement the inline validation requirement. A final option is to just implement an xhtml page (similar to what Firefox uses for network error pages) where the entire prompt is contained within the content area. That would give us the most flexibility, but of course "line of death" is an issue.

Antonela and others: what do you think?

comment:7 in reply to:  6 Changed 9 months ago by acat

Replying to mcs:

The mockups from comment:2 show a prompt that is contained entirely within the content area. How concerned should we be about the "line of death" issue (https://textslashplain.com/2017/01/14/the-line-of-death/)? It seems like a bad idea to implement a prompt that any site could easily spoof, but there are tradeoffs to consider.

This question came up as Kathy and I looked at various options within the Firefox codebase for implementing the client auth prompt. We might be able to use a doorhanger that includes an arrow that overlaps the chrome area (thus avoiding the "line of death" problem). But doorhangers within Firefox are designed for optional interactions and entering a key for client auth is not optional.

We could use the prompt service (which is what HTTP basic auth uses), but the prompts that are available to us are not very flexible. It might be a lot of work to achieve the look we want; for example, I am not sure how to implement the inline validation requirement. A final option is to just implement an xhtml page (similar to what Firefox uses for network error pages) where the entire prompt is contained within the content area. That would give us the most flexibility, but of course "line of death" is an issue.

Antonela and others: what do you think?

Interesting read :)

How difficult would it be to have a new kind of prompt/modal that mimics HTTP auth behaviour, but with the style/layout of the Onion Auth mockups? For behaviour I mean darkening the background (also above line of death) and blocking the browser UI.

Changed 9 months ago by antonela

Attachment: http-auth.png added

comment:8 Changed 9 months ago by gk

It seemed we are on the same page here that we don't want to have line-of-death-issues. Thus, this already cuts down possible options. Not sure how much we still have available though.

I am not sure how to deal with user story 3 either as by default saving things to disk is not allowed in Tor Browser. I think the risks are in particular evident for private keys. So, we'd have to think about how we would offer users this option if at all.

Changed 9 months ago by mcs

comment:9 Changed 9 months ago by mcs

Kathy and I have been exploring whether we can use Mozilla's notifications module (toolkit/modules/PopupNotifications.jsm) to implement the onion services authentication prompt. This would produce a doorhanger and would look a lot like the password manager's prompts. Here is a mockup:

https://trac.torproject.org/projects/tor/raw-attachment/ticket/30237/client-auth-notification-mockup-1.png

Antonela, Georg, and others: do you think this is an acceptable approach?

A side note: if it is important, we can move the "Learn more…" link up near "Onion Authentication"; we get it "for free" in the location shown because that is how Mozilla positions it within their other prompts.

comment:10 Changed 9 months ago by gk

Hah, almost the same idea as I had tonight. :) Yes, I think we should try to use that one and style it so nobody thinks it is a browser username/password prompt. And I think we should make it modal. That should give another hint about the dialog being not a usual username/password prompt. "modal" not in the sense of the HTTP authentication dialog which pops up if you load a page but need to wait and get bored switching to a different tab and suddenly an authentication prompt pops up out of nowhere. We should not do that. But "modal" in the sense that you can't click it away if you are on the page unless you either press Cancel or Done.

Last edited 9 months ago by gk (previous) (diff)

comment:11 Changed 9 months ago by cypherpunks

What is this feature? Looks like something new. Could you document it?

comment:12 in reply to:  9 Changed 9 months ago by asn

Replying to mcs:

Kathy and I have been exploring whether we can use Mozilla's notifications module (toolkit/modules/PopupNotifications.jsm) to implement the onion services authentication prompt. This would produce a doorhanger and would look a lot like the password manager's prompts. Here is a mockup:

https://trac.torproject.org/projects/tor/raw-attachment/ticket/30237/client-auth-notification-mockup-1.png

Antonela, Georg, and others: do you think this is an acceptable approach?

A side note: if it is important, we can move the "Learn more…" link up near "Onion Authentication"; we get it "for free" in the location shown because that is how Mozilla positions it within their other prompts.

Yes I think this is totally an acceptable approach. And I was not aware of the line-of-death issue but it does make sense.

Changed 9 months ago by antonela

Attachment: 30237-prompt.1.png added

Changed 9 months ago by antonela

Attachment: 30237-xhtml.1.png added

comment:13 Changed 9 months ago by antonela

We talked a bit about the notification prompt, and I think it is a possible solution. It works per site, and the key icon is straightforward to identify for end-users as a password need. The downside of it is how confusing it is when the public key is not a password. And, as mentioned in 6 prompts are usually optional. If a user cancels the prompt, will we render an error page? How can we allow users to recover from that flow to successfully add a key and access the website?.

https://trac.torproject.org/projects/tor/raw-attachment/ticket/30237/30237-prompt.1.png

The primary use of the password manager notification is for saving passwords. Therefore, are we going to allow users to save their used key?. Logins in Firefox has two general settings in Preferences. The first will enable users to remember logins for sites, and the second allows users to use or not a master password. What happens if users have "Remember logins for sites" disabled? What happens if users are using a master password?

Mixing private keys and passwords seems a complicated idea.

We can still use the notification doorhanger UI, tho. We need to be careful about to mimic the password behavior for a different flow. We should consider a scenario when the input fails, or the users have disabled the prompts at the General Preferences. If we allow users to re-use their saved key, we will need a specific section at Preferences to manage the saved private keys.


When I realized about the sad error recovering flow that the native HTTP Auth box is offering 1, I also thought about to take over the DOM via an XHTML page. Back then, I made a mockup of it. The main question remains in how spoofable it is from a random website.

https://trac.torproject.org/projects/tor/raw-attachment/ticket/30237/30237-xhtml.1.png


So, we have four routes for this UI implementation:

  1. an HTTP Auth UI + custom errors
  2. a notification doorhanger
  3. a XHTML page
  4. a custom modal

Which of them is more comfortable for users to understand what is the task there? Which of them can empower users to recover from an error? Which of them allow users to re-use their key? From the development perspective, which of these options is good enough for managing errors, scalable, and doable in the timeframe we have set for this scope?

comment:14 Changed 9 months ago by pospeselr

Cc: pospeselr added

comment:15 in reply to:  13 ; Changed 9 months ago by brade

Replying to antonela:

We talked a bit about the notification prompt, and I think it is a possible solution. It works per site, and the key icon is straightforward to identify for end-users as a password need. The downside of it is how confusing it is when the public key is not a password. And, as mentioned in 6 prompts are usually optional. If a user cancels the prompt, will we render an error page? How can we allow users to recover from that flow to successfully add a key and access the website? ...

I agree that the key icon is not necessarily ideal but I assume it would be straightforward to use our own icon for onion services. Is changing the icon in the URL bar and the icon in the door hanger sufficient to set apart our set of prompts from login manager prompts?

Yes, if a user cancels the prompt, they will see an error page. They will need to reload the page to see the prompt again (I believe there is a "Try again" button on the error page).

The primary use of the password manager notification is for saving passwords. Therefore, are we going to allow users to save their used key?. Logins in Firefox has two general settings in Preferences. The first will enable users to remember logins for sites, and the second allows users to use or not a master password. What happens if users have "Remember logins for sites" disabled? What happens if users are using a master password?

Mixing private keys and passwords seems a complicated idea.

We can still use the notification doorhanger UI, tho. We need to be careful about to mimic the password behavior for a different flow. We should consider a scenario when the input fails, or the users have disabled the prompts at the General Preferences. If we allow users to re-use their saved key, we will need a specific section at Preferences to manage the saved private keys.

I'm not sure if we should mix the saving of keys with the saving of passwords. Mark and I were thinking that we could provide a "Remember this key" checkbox in the prompt and if checked the key would be stored in the user's torrc file. Building an interface to managed stored client auth keys probably deserves its own ticket; I don't see us getting to that during this first round (before the end of June).

So, we have four routes for this UI implementation:

  1. an HTTP Auth UI + custom errors
  2. a notification doorhanger
  3. a XHTML page
  4. a custom modal

Which of them is more comfortable for users to understand what is the task there? Which of them can empower users to recover from an error? Which of them allow users to re-use their key? From the development perspective, which of these options is good enough for managing errors, scalable, and doable in the timeframe we have set for this scope?

Mark and I think that options 1 and 4 will require a lot of work to achieve a good looking prompt and that the approach is too modal (we really want a prompt that is modal to the tab but not to the entire window).

Option 3 will result in a prompt that is easy for a phishing site to spoof.

We think we can achieve a good experience with option 2. We can prevent the notification prompt from disappearing until the user clicks "Cancel" or "Done" and we can customize the appearance as needed.

Changed 9 months ago by antonela

Attachment: 30237-prompt.2.png added

comment:16 in reply to:  15 ; Changed 9 months ago by antonela

Replying to brade:

We think we can achieve a good experience with option 2. We can prevent the notification prompt from disappearing until the user clicks "Cancel" or "Done" and we can customize the appearance as needed.

All good. We should make sure that we are not mixing the password/auth flow. Maybe, we can achieve that by removing the key icon at the URL bar.

We will not have an inline validation for user input errors; all the errors will render in an error page. Am I right?

I'll work on the error page in the meantime. Do we have different errors? Which errors we are going to handle?

Personally, I'd prefer to get closer to the HTTP auth standard instead of introducing a new user flow. If the doorhanger implementation is easier for doing it than the regular popup, it is good.

https://trac.torproject.org/projects/tor/raw-attachment/ticket/30237/30237-prompt.2.png

comment:17 in reply to:  16 Changed 9 months ago by brade

Replying to antonela:

...
We will not have an inline validation for user input errors; all the errors will render in an error page. Am I right?

I'll work on the error page in the meantime. Do we have different errors? Which errors we are going to handle? ...

Errors specifically related to user input of a key will be displayed in a doorhanger-like prompt, and Mark and I think we should be able to do inline validation there. For this bug we are focused on error codes 0xF4 and 0xF5 as described in this page:

https://lists.torproject.org/pipermail/tor-dev/2019-May/013835.html

Let's move the discussion about other errors to ticket #30025. Those errors can be displayed in the content area. We haven't done detailed research yet, but we are hoping to build on Firefox's ESR68 error pages.

That said, Mark and I are trying to stay focused on client auth so we can get something working before we must switch to ESR68 work. In fact, it probably makes sense for you to give us another week to determine if the doorhanger-like approach we are currently pursuing will work out before you invest more time in UX design for this ticket.

comment:18 Changed 8 months ago by gk

Keywords: TorBrowserTeam201906 added; TorBrowserTeam201905 removed

Moving tickets to June

Changed 8 months ago by mcs

Attachment: client auth prompt rev1.png added

comment:19 Changed 8 months ago by mcs

Status: needs_informationnew

Kathy and I pushed our "work in progress" patches for tor-browser, Torbutton, and Tor Launcher so that anyone who is interested can look at what we have so far:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug30237-01&id=d92a5bd3fd961b7b9f05383501b1bc3ed6391ecb
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug30237-01&id=a7e7992a9fa749e99bfad1a724672b6091cba1ad
https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug30237-01&id=44931dac3bb51fc45aacc53c6a27e2983e84c7ac

Here is a screenshot (yes, it will look better without the onion name redacted):

https://trac.torproject.org/projects/tor/raw-attachment/ticket/30237/client%20auth%20prompt%20rev1.png

Significant loose ends include:

  • Our tor-browser changes do not support localization (see the many places that are marked with L10n). Kathy and I would like to understand our localization strategy in a post-Torbutton world before we work on L10n support.
  • We need to insert the correct "Learn More" URL.
  • The text/copy will need some work. Some concepts are difficult to communicate clearly. For example, if the user enters a key of the wrong length or format, we currently show the somewhat cryptic validation error that is visible in the screenshot I posted.
  • We need artwork for the onion icon that is used within the prompt (and if it would be better to do so, we could remove it). As a placeholder, we are using a plain onion image that we downloaded from https://media.torproject.org/image/Onion%20Icon/
  • We would like Antonela to give us feedback about the UX flow and overall experience. For example, while the prompt is displayed we show a blank page behind it. Is that okay?

Using the above patches, we created some builds for testing. We used a tor daemon based on David's ticket30381_042_01 branch from https://git.torproject.org/user/dgoulet/tor.git (which includes the work-in-progress patches for both #30381 and #30382). Please look at the README.txt file that is in the same directory as the builds for a helpful tip about reducing your SocksTimeout setting. The builds are here:

https://people.torproject.org/~brade/testbuilds/clientauth-2019-06-27/

Last edited 8 months ago by mcs (previous) (diff)

comment:20 Changed 8 months ago by mcs

I forgot to mention that our implementation does not yet provide an option to store private keys persistently.

comment:21 Changed 8 months ago by asn

This looks really good!

comment:22 in reply to:  19 ; Changed 8 months ago by antonela

Thanks mcs and brade, it looks great! Will go inline:

  • Our tor-browser changes do not support localization (see the many places that are marked with L10n). Kathy and I would like to understand our localization strategy in a post-Torbutton world before we work on L10n support.
  • We need to insert the correct "Learn More" URL.

Yes. I can coordinate it with the community team. The link should give users more info about why they need a key and how they can find it. Filed #31069 to make it happen.

  • The text/copy will need some work. Some concepts are difficult to communicate clearly. For example, if the user enters a key of the wrong length or format, we currently show the somewhat cryptic validation error that is visible in the screenshot I posted.

I was able to reproduce two kinds of errors: "Please enter a valid key" and "Unable to configure Tor with your key". Are more errors coming?

Could we use a class to make those errors look more like an error? We can rely on the firefox suggestions -> https://design.firefox.com/photon/patterns/errors.html

I think we can use Firefox's login/key icon. Do we want a custom icon here?
https://design.firefox.com/icons/icons/desktop/login-16.svg

  • We would like Antonela to give us feedback about the UX flow and overall experience. For example, while the prompt is displayed we show a blank page behind it. Is that okay?

I think the white page is good for now, I'll think more about it. We should have a better error page when users click "Cancel". I'll make sure we have it in consideration for #30025.

Using the above patches, we created some builds for testing. We used a tor daemon based on David's ticket30381_042_01 branch from https://git.torproject.org/user/dgoulet/tor.git (which includes the work-in-progress patches for both #30381 and #30382). Please look at the README.txt file that is in the same directory as the builds for a helpful tip about reducing your SocksTimeout setting.

Thanks a lot for doing this!

comment:23 Changed 8 months ago by gk

Keywords: TorBrowserTeam201907 added; TorBrowserTeam201906 removed

Moving tickets to July

comment:24 in reply to:  22 Changed 7 months ago by gk

Replying to antonela:

I think we can use Firefox's login/key icon. Do we want a custom icon here?
https://design.firefox.com/icons/icons/desktop/login-16.svg

I'd be fine with that icon. On the other hand maybe we can make some combo of a key and a onion to make sure users understand that this the key for the .onion service.

comment:25 Changed 7 months ago by gaba

Keywords: network-team-roadmap-september added

comment:26 in reply to:  22 Changed 6 months ago by mcs

Replying to antonela:

I was able to reproduce two kinds of errors: "Please enter a valid key" and "Unable to configure Tor with your key". Are more errors coming?

Just those two for now. Later we can add a specific error message to cover the case where the key provided by the user does not work (NS_ERROR_TOR_ONION_SVC_BAD_CLIENT_AUTH). In English, that message might be something like "The private key you provided for thing.onion is not valid. Please enter a new key".

Could we use a class to make those errors look more like an error? We can rely on the firefox suggestions -> https://design.firefox.com/photon/patterns/errors.html

Yes, Kathy and I will take a look after the ESR68 crunch is past us.

comment:27 Changed 5 months ago by gk

FWIW, this is concerned with the desktop part. We don't have a ticket for the mobile side yet. It could be #31672, though.

comment:28 Changed 4 months ago by mcs

Keywords: TorBrowserTeam201911 added; TorBrowserTeam201907 removed

comment:29 Changed 3 months ago by mcs

Keywords: TorBrowserTeam201911R added; TorBrowserTeam201911 removed
Status: newneeds_review

This is ready for review; it would be nice to get it into a nightly build soon so people can experiment with the UX. We split the Torbutton changes into two commits (both pushed to brade's bug30237-02 branch):

https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug30237-02&id=b31352c01427ec3a70d0564ec683b227720998a7

https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug30237-02&id=b9851162c1c5dd356b01b868ade183a163894b26

There is also one tor-browser commit:

https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug30237-02&id=dfdf76258eadaba5acbbc74229aa5d66e8b7c033

We were able to adapt some CSS that we found in Firefox in order to match the Photon error look mentioned by Antonela in comment:22. We also switched to Firefox's authentication icon for now (the key).

I will post a screenshot that shows the new look for the error display.

Changed 3 months ago by mcs

Attachment: client auth prompt rev2.png added

comment:30 Changed 3 months ago by mcs

Sample prompt:

https://trac.torproject.org/projects/tor/raw-attachment/ticket/30237/client%20auth%20prompt%20rev2.png

(we can also work on improving the copy; for example, it would be great if the error message shown here fit on one line).

comment:31 Changed 3 months ago by mcs

Cc: tbb-team added
Owner: changed from tbb-team to mcs
Status: needs_reviewassigned

comment:32 Changed 3 months ago by mcs

Status: assignedneeds_review

comment:33 Changed 3 months ago by pospeselr

Keywords: TorBrowserTeam201911 added; TorBrowserTeam201911R removed
Status: needs_reviewneeds_revision

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.

comment:34 Changed 3 months ago by gk

I have not looked at the code yet, just let me throw in #26184 which seems is in need to get resolved to make sure where we are heading wrt to coding style. I've seen, when doing code reviews lately, that there is a fraction of us at least that likes the proposal of that bug, but I won't drive that forward. Someone from the team should pick that up and then we/you need to move forward with the decision made. Not sure if *now* would be a good time for that but I just got reminded at that by skimming pospeselr's review comments.

comment:35 Changed 3 months ago by mcs

Keywords: TorBrowserTeam201911R added; TorBrowserTeam201911 removed
Status: needs_revisionneeds_review

Thanks again for the review. We addressed most of the issues mentioned in comment:33 and fixed a couple of other issues we found. We did not add const everywhere; Kathy and I are still not sure if that is a good idea given the trade-offs. As Georg mentioned in comment:34, we should have the #26184 discussion (and probably a general coding style discussion) within the team. The bulk of the work we did was to make things more modular, moving most of the code into a new onionservices browser component.

Here are two new Torbutton commits (both pushed to brade's bug30237-03 branch):
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug30237-03&id=f774d8858b4c52dcdadc7f7d67c5cb4d8a21527d

https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug30237-03&id=4edfada7d1da1f19403eb9a52d372793a1f6945f

(we did not make any changes to the second one).

And here is a revised tor-browser commit (pushed to brade's bug30237-03 branch):
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug30237-03&id=509999fe8ef29fa3cb2915bdfb050d4512014081

comment:36 Changed 3 months ago by pospeselr

The updated changes look good to me!

comment:37 Changed 3 months ago by gk

Status: needs_reviewneeds_information

The torbutton changes look good to me. While reading over the browser changes and thinking about upcoming releases: what tor version do we need for this feature? In case there is no released version yet, what happens if the code changes in this bug land in a Tor Browser that does not have the corresponding features in tor it ships yet? What is the user experience then? Do things break?

comment:38 Changed 3 months ago by acat

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?

I think in this case it made sense to move it out to a separate module and just have init and uninit. Probably it always make sense to do this when the new code is a reasonably self-contained component.

Not so sure about having a general rule, I think there are many cases where the changes are fairly small and doing includes might be a bit "artificial". For example: https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-68.2.0esr-9.5-1&id=15738bd0be40f8ee3f33b5da7d548c2b9066bce4. Here I think it makes more sense to have these two new toolbarbutton inline rather than putting them on a separate file and include it. For me it's more readable this way. Same for the new code in CustomizableUI.jsm, you can see that there is a kTorVersion variable which is analogous to Firefox's kVersion because it's next to it, and then the migration code for Tor Browser is next to the other Firefox's migration code. I just think there are cases where it's more readable to not do includes, but perhaps what I'm saying is obvious and pospeselr didn't actually mean these cases.

Anyway, I think it's a good idea to try to keep modifications to Firefox sources as minimal as possible, and use includes and imports whenever it feels "natural" to do so. It's a bit vague, but I guess there will always be subjectiveness in code reviews :)

comment:39 in reply to:  37 Changed 3 months ago by mcs

Status: needs_informationneeds_review

Replying to gk:

The torbutton changes look good to me. While reading over the browser changes and thinking about upcoming releases: what tor version do we need for this feature? In case there is no released version yet, what happens if the code changes in this bug land in a Tor Browser that does not have the corresponding features in tor it ships yet? What is the user experience then? Do things break?

Good question. Things should not break. Because an older tor will not send any new SOCKS error codes, the browser will continue to exhibit the "old" behavior (unknown site) when the user tries to connect to a v3 onion that requires client authentication.

And I keep forgetting that the tor patches have not been merged yet. The necessary patches are in review and #14389 is the tracking bug. I hope they land soon.

Finally, your comment reminded me that Kathy and I forgot to post the Tor Launcher patch that is needed to enable the extended SOCKS errors feature. Here it is:
https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug30237-03&id=7821bd58b69ed99c1a9bf86e102279c5bc0472c2

We should not ship this Tor Launcher patch until we start to ship a tor that includes the #14389 fixes. It should be OK to merge the Torbutton and Tor Browser patches earlier, but maybe we should wait until the #14389 patches are available on tor master.

comment:40 Changed 3 months ago by gk

Cc: sysrqb added

Okay, I went forward with applying the torbutton parts (commits 46efc92348dbed06fc31ddfb0a5ac2e4e8554de2 and fa05e61cac497a7fbe3050ee8a9f4f6ec39be3a4) both because starting the translations process early is a good idea and the controller related refactoring is pretty harmless and can get some testing in the upcoming nightlies.

As I said, I'll leave the decision to sysrqb as to what to do with the large changes in tor-browser: that is whether we should wait for the tor to even start shipping those changes for nightly builds and the upcoming alpha.

comment:41 Changed 3 months ago by gk

Keywords: TorBrowserTeam201912R added; TorBrowserTeam201911R removed

We are in December now.

comment:42 Changed 3 months ago by pili

Reviewer: pospeselr

comment:43 Changed 3 months ago by pili

Reviewer: pospeselrsysrqb

comment:44 Changed 3 months ago by pili

Reviewer: sysrqbpospeselr, sysrqb

comment:45 Changed 3 months ago by antonela

#32562 is done. Will we include it in the UI? What is needed on my side?

comment:46 in reply to:  45 ; Changed 3 months ago by mcs

Replying to antonela:

#32562 is done. Will we include it in the UI? What is needed on my side?

Yes, we plan to add a simple checkbox below the key input field. We need to agree on the label; probably something like Remember this key. What do you think?

comment:47 in reply to:  46 Changed 2 months ago by antonela

Replying to mcs:

Replying to antonela:

#32562 is done. Will we include it in the UI? What is needed on my side?

Yes, we plan to add a simple checkbox below the key input field. We need to agree on the label; probably something like Remember this key. What do you think?

Works for me. Thanks!

comment:48 Changed 6 weeks ago by sysrqb

Keywords: TorBrowserTeam202001R added; TorBrowserTeam201912R removed

comment:49 Changed 5 weeks ago by mcs

Points: 1

Added an estimate for the remaining points (1).

comment:50 Changed 5 weeks ago by mcs

Actual Points: 16
Points: 117

Updated points and actual points to reflect actual time spent on this ticket so far.

comment:51 Changed 5 weeks ago by mcs

Kathy and I rebased the patches to the tor-browser-68.4.1esr-9.5-1 branch. The patch is here:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug30237-04&id=6cac185d0c10e4f26ca7eaf000c31fae36d13bfc

We also rebased the Tor Launcher change so it is based off current master:
https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug30237-04&id=8726dc9a06524edc03d053a3258652dd9337ef78

Finally, we made a couple of small improvements to the control port module in Torbutton; these are new changes that should be reviewed:
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug30237-04&id=9dbc7d20a0efbe6d6d085950c937ed223176e6fa

comment:52 Changed 4 weeks ago by pospeselr

The control port changes look good to me.

comment:53 Changed 4 weeks ago by mcs

Actual Points: 1616.1

comment:54 Changed 4 weeks ago by gaba

Keywords: network-team-roadmap-2020Q1 added

comment:55 in reply to:  51 Changed 2 weeks ago by sysrqb

Replying to mcs:

Kathy and I rebased the patches to the tor-browser-68.4.1esr-9.5-1 branch. The patch is here:
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug30237-04&id=6cac185d0c10e4f26ca7eaf000c31fae36d13bfc

+    let retval = {
+      learnMore: getString("torPreferences.learnMore", "Learn More"),
+      learnMoreURL: `https://2019.www.torproject.org/docs/tor-manual-dev.html.${getLocale()}#_client_authorization`,

I know that's waiting on #31069 but :(

+   *          - [optional] leaveOpen (boolean): If this is true, the notification
+   *            will not be removed after running the callback.
    *          - [optional] disableHighlight (boolean): If this is true, the button
    *            will not apply the default highlight style.
    *        If null, the notification will have a default "OK" action button
@@ -1863,6 +1865,10 @@ PopupNotifications.prototype = {
         this._dismiss();
         return;
       }
+
+      if (action.leaveOpen) {
+        return;

I wonder if we can uplift this.

We also rebased the Tor Launcher change so it is based off current master:
https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug30237-04&id=8726dc9a06524edc03d053a3258652dd9337ef78

Looks good. ExtendedErrors was released in 0.4.3.1-alpha, so we can try this in the next release.

Finally, we made a couple of small improvements to the control port module in Torbutton; these are new changes that should be reviewed:
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug30237-04&id=9dbc7d20a0efbe6d6d085950c937ed223176e6fa

Looks good to me.

I didn't see any major problems in these patches. Thanks for all the work you put into this! Let's see how this looks on nightly.

tor-browser: 28641dbae674f58429aa3518342555a6b977342e
tor-launcher: 8726dc9a06524edc03d053a3258652dd9337ef78
torbutton: 9dbc7d20a0efbe6d6d085950c937ed223176e6fa

comment:56 Changed 2 weeks ago by sysrqb

Resolution: implemented
Status: needs_reviewclosed
Type: defectenhancement

comment:57 Changed 13 days ago by sysrqb

Keywords: 9.5a5 added

comment:58 Changed 9 days ago by mcs

Actual Points: 16.116.2
Note: See TracTickets for help on using tickets.