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

Registered usernames that are duplicates or invalid should trigger aggressive warning #119

Closed
holmesworcester opened this issue Jul 20, 2021 · 18 comments

Comments

@holmesworcester
Copy link
Contributor

holmesworcester commented Jul 20, 2021

We should validate that data written to the user table meets our criteria, and show an aggressive warning if it does not.

  1. All users should validate that username entries are correct, by the same criteria as the registrar frontend (alphanumeric, character limits, etc.)
  2. All users should validate that there are not duplicate username registrations.
  3. Seeing a message from a user with a duplicate registration signed by the admin should trigger a warning, even if there is only one registration in the user table. This does not apply to unregistered users, or an unregistered user having the same name of a registered user. This aggressive warning should only show when we have two messages from registered users signed by different public keys with the same name.
  4. The warning should show on startup or, in the future, whenever someone switches to the community.
  5. The warning should include a "leave community" button.

https://www.figma.com/file/TV9pF84Ob8pLYRLu83gNol/Joining-when-owner-is-offline?type=design&node-id=311-6979&mode=design&t=eS9WDLYpQNod1Acl-4

Image

@holmesworcester holmesworcester changed the title Validate username (lowercase, just alphanumeric, character limit, etc.) on waggle side Validate username (lowercase, just alphanumeric, character limit, etc.) on waggle/nectar side Aug 31, 2021
@holmesworcester holmesworcester changed the title Validate username (lowercase, just alphanumeric, character limit, etc.) on waggle/nectar side Validate username on waggle/nectar side, in registrar and clients Aug 31, 2021
@ghost ghost unassigned EmiM Dec 14, 2021
@holmesworcester holmesworcester changed the title Dec 14, 2021
@vinkabuki vinkabuki transferred this issue from TryQuiet/waggle Jan 4, 2022
@holmesworcester holmesworcester moved this to Backlog - Desktop & Backend in Quiet Apr 5, 2023
@holmesworcester holmesworcester moved this from Backlog - Desktop & Backend to Next Sprint in Quiet Aug 7, 2023
@holmesworcester holmesworcester changed the title Any duplicate or invalid usernames should trigger aggressive warning Registered usernames that are duplicates or invalid should trigger aggressive warning Aug 7, 2023
@EmiM
Copy link
Contributor

EmiM commented Sep 14, 2023

@holmesworcester

  1. I don't see designs for this "prominent non-closeable warning banner". I only see designs for closeable modal.
    There is a comment Warn user of duplicate official name #1647 (comment) saying "I think it makes sense to redesign this to be an alert banner that displays all the time rather than a full screen thing. Or it could be a full screen warning followed by an alert banner that displays all the time. Let's knock this back to 'needs design' and move it out of the sprint.". Since this task is marked as duplicate of 119 and I don't see a redesign I assume that we still need a design and a decision.

  2. [EPIC] User should be able to join when owner is not online (discuss together and close) #1340 (comment) You wrote that the modal should be displayed on every restart. I wonder about this case:

  • New unregistered user joins but does not connect to owner, connects to other unregistered users
  • Turns out that new user chose username that is already taken
  • The unregistered user is marked as "duplicated", other users see the modal(?)
  • The unregistered user can ignore the fact that has duplicated username or simply close the Quiet and never come back
  • The rest of community will be receiving "WARNING" popup on each Quiet start up indefinitely?
  1. What do I see as a user with duplicated username? Modal? Popup? Said banner (AD 1)?

@holmesworcester
Copy link
Contributor Author

holmesworcester commented Sep 14, 2023

sorry for the lack of clarity.

re: alert banners, the closeable design here is our final decision. note that the alert text in figma is slightly different than the screenshot here. I clarified the issue description above.

If a registered user sees a duplicate username with another registered user we can show them the same warning as anybody else because it's strong enough, though in an ideal world we would tell them "hey, somebody is impersonating you."

@vinkabuki
Copy link
Contributor

vinkabuki commented Sep 18, 2023

@holmesworcester

All users should validate that username entries are correct, by the same criteria as the registrar frontend (alphanumeric, character limits, etc.)

  1. What is registrar frontend? We use registrar name to reference backend module for issuing certificates. Do you mean UI step when user types desired username?

Seeing a message from a user with a duplicate registration signed by the admin should trigger a warning, even if there is only one registration in the user table

  1. What does user table refers to? Users with certificates? Anyway "only one registration in the user table" - I understand it that if we have registered and unregistered users with the same username we should show warning, right?

This does not apply to unregistered users, or an unregistered user having the same name of a registered user

  1. This means that we should not show the warning if there are two unregistered users with the same username of there is registered and unregistered user with the same username, right?

It's strongly emphasized that we should ignore actual certificates or certificate requests duplicates if there are no messages signed by those and do not show the warning. Just making sure it's the case.

@Kacper-RF Kacper-RF moved this from Next Sprint to Sprint in Quiet Sep 18, 2023
@Kacper-RF Kacper-RF moved this from Sprint to In progress in Quiet Sep 18, 2023
@Kacper-RF Kacper-RF self-assigned this Sep 18, 2023
@holmesworcester
Copy link
Contributor Author

  1. All registered names should meet the same validation criteria we use when registering. If not, warning.

  2. If we have registered and unregistered users with the same name we should not show this warning. Only if we have registered users (since then something is very wrong, i.e. why has the registrar registered the same name for two people)

  3. Correct. We should not show the warning for a registered and unregistered user with the same name, or for two unregistered users with the same name (because this could happen naturally and does not imply that the registrar is broken or compromised.)

It's strongly emphasized that we should ignore actual certificates or certificate requests duplicates if there are no messages signed by those and do not show the warning. Just making sure it's the case.

Do you mean you recommend this? Can you tell me more? Two registered certificates with the same name is also a problem.

If it's hard for some reason we could create an issue for this case and address it later. But I want to cover it because it indicates the registrar is misbehaving.

@vinkabuki
Copy link
Contributor

@holmesworcester
No, I mean it is emphasized by you twice here, that's why I am asking - in terms of implementation it's 10x easier to rely on certificates and do not care if there are or aren't any messages:
Seeing a message from a user with a duplicate registration signed by the admin should trigger a warning, even if there is only one registration in the user table. This does not apply to unregistered users, or an unregistered user having the same name of a registered user. This aggressive warning should only show when we have two messages from registered users signed by different public keys with the same name.

@holmesworcester
Copy link
Contributor Author

holmesworcester commented Sep 18, 2023

The important thing for security is how easy it is for the registrar to spoof messages.

Is there a case where the owner could send spoofed messages without triggering the warning?

@vinkabuki
Copy link
Contributor

no - owner has no such power - it has the power of breaking our rules for certificates as we talked - issuing more than one cert for same username etc. Thanks for clarification

@vinkabuki
Copy link
Contributor

vinkabuki commented Sep 19, 2023

@holmesworcester one more question:
What shoud happen to messages sent by users with duplicate username after changing username ?
Also "changing" is very imprecise word for this - it's entirely new user, same as someone fresh joining.

@holmesworcester
Copy link
Contributor Author

holmesworcester commented Sep 19, 2023

Let's mark them with a red badge that says Warning and links to the same warning modal.

Can we keep track of which one the user has seen first? If so we could hide messages from the other one entirely.

@Kacper-RF Kacper-RF moved this from In progress to Waiting for review in Quiet Sep 19, 2023
@vinkabuki
Copy link
Contributor

vinkabuki commented Sep 19, 2023

@holmesworcester

Can we keep track of which one the user has seen first? If so we could hide messages from the other one entirely.

That doesn't make sense to me - if I try to register user 'bartek' and it's taken, then I register user 'bart', if someone saw bartek first they will hide messages from bart, is that right?

@holmesworcester
Copy link
Contributor Author

Why would bart and bartek be duplicates?

@vinkabuki
Copy link
Contributor

vinkabuki commented Sep 19, 2023

There is some misunderstanding, I am preparing user stories atm that cover all the cases.

I think that's the moment we stopped to understand each other.

What shoud happen to messages sent by users with duplicate username after changing username ?

That sentence means:

  1. There is someone with bartek username in community already
  2. I try to join with bartek username, because I do not know that
  3. I send messages
  4. I am prompted to take other username after replicating certificates
  5. I take username "waclaw" now, but I also gave some important information to my collegues as unregistered duplicated bartek - which is now dropped and replaced on my side by entirely new identity.

@leblowl
Copy link
Collaborator

leblowl commented Sep 19, 2023

I think the public key should identify a user. To change a username, you could just reissue a certificate signing request with the same key. Once the certificate is issued and signed by a trusted source, then a new username is now associated with that public key. From a client's perspective all messages signed by that user's key could have the new username.

@Kacper-RF
Copy link
Contributor

#1831

@Kacper-RF Kacper-RF moved this from Waiting for review to Merged (develop) in Quiet Oct 5, 2023
@Kacper-RF Kacper-RF moved this from Merged (develop) to Ready for QA in Quiet Oct 9, 2023
@kingalg
Copy link
Collaborator

kingalg commented Oct 31, 2023

Version: 2.0.3-alpha.0

I found two issues with how it's currently working:

  1. I was able to create a situation in which I get two users registered with the same username and I saw our aggressive warning.
  2. On mobile popup with warning is impossible to close - there is a close button but popup just blink and appear again.

Second problem should be easy to fix but first one is disconcerting. I get the popup and two users registered with the same username by:

  1. I've created owner and joined with one user.
  2. I've closed the owner.
  3. I've joined registered user with two mobiles and one desktop giving them the same username "test".
  4. I've opened the owner.
    What happened - one mobile user get a popup informing that their username is already taken, desktop and second mobile both registered with username "test".
Screenshot 2023-10-31 at 11 37 23

This version is the first one in which I get to see this popup also I've tested it in previous versions. There are two possible why I only see it now:

  • this is the first time when I've joined with 3 users with the same username when owner was offline (I've previously joined with 3 users with the same username but in my test first user was joining when the owner was online and two others joined when the owner was offline)
  • we resolved issue Duplicate - Unregistered users mixed up #1981 (comment) and the solution that we've implemented may have an impact on how this part of the app is working now.

As for design implementation from Figma - they are implemented correctly.
Screenshot_20231031-113437
image (3)
Screenshot 2023-10-31 at 11 36 23

@Kacper-RF Kacper-RF moved this from Ready for QA to In progress in Quiet Oct 31, 2023
@Kacper-RF
Copy link
Contributor

PR for On mobile popup with warning is impossible to close - there is a close button but popup just blink and appear again.

#2023

@Kacper-RF
Copy link
Contributor

#2026

@Kacper-RF Kacper-RF moved this from In progress to Waiting for review in Quiet Nov 6, 2023
@Kacper-RF Kacper-RF moved this from Waiting for review to Merged (master) in Quiet Nov 7, 2023
@Kacper-RF Kacper-RF moved this from Merged (master) to Merged (develop) in Quiet Nov 7, 2023
@Kacper-RF Kacper-RF moved this from Merged (develop) to Ready for QA in Quiet Nov 8, 2023
@Kacper-RF Kacper-RF moved this from Ready for QA to In progress in Quiet Nov 8, 2023
@Kacper-RF Kacper-RF moved this from In progress to Ready for QA in Quiet Nov 9, 2023
@kingalg
Copy link
Collaborator

kingalg commented Nov 9, 2023

Version: 2.0.3-alpha.2
mobile@2.0.3-alpha.2 (iOS 329)

The issue with impersonation attack has been solved. I can no longer make this popup appear or register two users with the same username. While fixing this issue another thing break - new users joining while the Owner is offline don't get "Unregistered" tag anymore (it was working in previous versions). New issue is opened for that - #2050 as per discussion with @Kacper-RF

@kingalg kingalg closed this as completed Nov 9, 2023
@kingalg kingalg moved this from Ready for QA to Done in Quiet Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

No branches or pull requests

6 participants