Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security] Uphold linking modals should be combined and have privacy warning #11431

Closed
diracdeltas opened this issue Aug 25, 2020 · 10 comments · Fixed by brave/brave-core#6926
Closed
Assignees
Labels
feature/rewards needs-discussion Although the issue is clear, we haven't yet reached a decision about the right solution. OS/Desktop priority/P2 A bad problem. We might uplift this to the next planned release. privacy QA Pass-Linux QA Pass-macOS QA Pass-Win64 QA/Yes release-notes/include security

Comments

@diracdeltas
Copy link
Member

Discussed in https://github.com/brave/security/issues/72:

We currently show two different modals in brave://rewards for uphold wallet linking, depending on whether you already have a verified uphold account or if you have at least 25 BAT.

25 BAT screen:

2020-08-25-213956_676x486_scrot

already have a verified wallet:

Screen Shot 2020-08-25 at 2 30 40 PM

  1. We should consolidate these so that users see the same message no matter how they link their wallet.
  2. The consolidated modal needs to have a message along the lines of, "If you verify your wallet, Uphold is able to see your tipping activity."

cc @NejcZdovc @evq

@diracdeltas diracdeltas added OS/Android Fixes related to Android browser functionality OS/Desktop security privacy feature/rewards labels Aug 25, 2020
@NejcZdovc NejcZdovc removed the OS/Android Fixes related to Android browser functionality label Aug 26, 2020
@mandar-brave
Copy link

@diracdeltas the second login only works for users that have an Uphold account; the entire idea of the second dialog is if they have had previously created the account.

When one links the wallet, the user explicitly sees the authorization - that is what Tom and we agreed to do clearly suggesting to the user that a) you are creating an account on Uphold as them being a Custodian and they have fiduciary duties and b) you are authorizing Brave to get XYZ access to your Uphold account.

@NejcZdovc @LaurenWags if you have the authorize screen handy, can you add to this ticket?

The message on image-1 needs a redo and I can get going on that; it makes sense.

I am trying to understand why have the same exact message when they re-login in to the system; may be change the Login to "Re-login". We can add the message but we can't have the same screen when the use cases are very different and are meant to do a different thing.

@NejcZdovc
Copy link
Contributor

@mandar-brave did you mean this image?

image

@mandar-brave
Copy link

its the entire auth aspect including the scopes @NejcZdovc - this is just the text part of it.
thanks.

@diracdeltas
Copy link
Member Author

@mandar-brave understood; regardless I think it's not clear to the user when they link their Uphold wallet (regardless of whether they already have an Uphold account) that this allows Uphold to see tipping activity. This has important privacy implications and should be clearly presented to the user.

@NejcZdovc NejcZdovc added the needs-discussion Although the issue is clear, we haven't yet reached a decision about the right solution. label Aug 27, 2020
@mandar-brave
Copy link

mandar-brave commented Aug 27, 2020

@diracdeltas nothing against the ask; wondering how we can pack each such transaction over time in to the UI, and then as we add more custodian providers.

The re-login page does force the user to go through an authorization page explicitly via Uphold (and that may be a better place to fix all text as well), so if they have not authenticated the Brave Browser one time, they have to irrespective of which modal they clicked in. So if the user signed up on Uphold but never authorized the Brave Browser, they have to do that. Fair call on the text, just feel its an overkill with multiple pages in play. And understand the hosted authorization page is in Uphold Custody.

My suggestion is we have a hosted redirect page that does this vs. having to stuff this in to modals which cannot take ever expanding context. the hosted page will allow us to control the text and story for each custodian globally with clear call outs for all of the pieces.

It has significant funnel conversion issues, but at a minimum we can reduce friction in terms of having to update new features in to the modal.

thoughts? cc @evq per chat yesterday

@diracdeltas
Copy link
Member Author

My suggestion is we have a hosted redirect page that does this vs. having to stuff this in to modals which cannot take ever expanding context. the hosted page will allow us to control the text and story for each custodian globally with clear call outs for all of the pieces.

sgtm from a privacy/consent perspective

@mandar-brave
Copy link

@jenn-rhim @bradleyrichter lets talk

@pes10k pes10k added the priority/P2 A bad problem. We might uplift this to the next planned release. label Sep 1, 2020
@srirambv srirambv changed the title Uphold linking modals should be combined and have privacy warning [Desktop] Uphold linking modals should be combined and have privacy warning Sep 9, 2020
@NejcZdovc NejcZdovc self-assigned this Oct 21, 2020
@NejcZdovc NejcZdovc added this to the 1.18.x - Nightly milestone Oct 21, 2020
NejcZdovc added a commit to brave/brave-core that referenced this issue Oct 22, 2020
@LaurenWags
Copy link
Member

@NejcZdovc @mandar-brave to confirm, this issue only covers the directive to have a note from 2 in the description

  1. The consolidated modal needs to have a message along the lines of, "If you verify your wallet, Uphold is able to see your tipping activity."

This issue does not cover a consolidated modal for the two different scenarios, correct?

@LaurenWags LaurenWags changed the title [Desktop] Uphold linking modals should be combined and have privacy warning [Desktop] [Security] Uphold linking modals should be combined and have privacy warning Nov 20, 2020
@NejcZdovc
Copy link
Contributor

Correct at the end we didn't merge them, we just added notes

@LaurenWags
Copy link
Member

LaurenWags commented Nov 25, 2020

Verified passed with

Brave | 1.18.62 Chromium: 87.0.4280.67 (Official Build) dev (x86_64)
-- | --
Revision | 0e5d92df40086cf0050c00f87b11da1b14580930-refs/branch-heads/4280@{#1441}
OS | macOS Version 10.14.6 (Build 18G6042)

Verified test plan from brave/brave-core#6926

Confirmed able to see the note on the Uphold 25 BAT verification message from both the panel and brave://rewards page:
Screen Shot 2020-11-25 at 8 14 58 AM
Screen Shot 2020-11-25 at 8 15 13 AM

Confirmed able to see the same message on the Verify Wallet modal:
Screen Shot 2020-11-25 at 8 16 08 AM


Verification passed on


Brave | 1.18.66 Chromium: 87.0.4280.67 (Official Build) dev (64-bit)
-- | --
Revision | 0e5d92df40086cf0050c00f87b11da1b14580930-refs/branch-heads/4280@{#1441}
OS | Windows 10 OS Version 2004 (Build 19041.630)


Verified test plan from brave/brave-core#6926

Confirmed able to see the note on the Uphold 25 BAT verification message from both the panel and brave://rewards page:
image
image

Confirmed able to see the same message on the Verify Wallet modal:
image


Verification passed on

Brave 1.18.62 Chromium: 87.0.4280.67 (Official Build) dev (64-bit)
Revision 0e5d92df40086cf0050c00f87b11da1b14580930-refs/branch-heads/4280@{#1441}
OS Ubuntu 18.04 LTS

Verified test plan from brave/brave-core#6926

Confirmed able to see the note on the Uphold 25 BAT verification message from both the panel and brave://rewards page:
image
image

Confirmed able to see the same message on the Verify Wallet modal:

image

@rebron rebron changed the title [Desktop] [Security] Uphold linking modals should be combined and have privacy warning [Security] Uphold linking modals should be combined and have privacy warning Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/rewards needs-discussion Although the issue is clear, we haven't yet reached a decision about the right solution. OS/Desktop priority/P2 A bad problem. We might uplift this to the next planned release. privacy QA Pass-Linux QA Pass-macOS QA Pass-Win64 QA/Yes release-notes/include security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants