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

Invite by mxId - Don't add unrecognised mxId in the ‘suggestions’ section #25446

Closed
giomfo opened this issue May 25, 2023 · 23 comments · Fixed by matrix-org/matrix-react-sdk#11055
Assignees
Labels
A-Invite O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect

Comments

@giomfo
Copy link
Member

giomfo commented May 25, 2023

Steps to reproduce

Scenario 1:

  • Start a new DM by filling a non-existent MatrixId

Scenario 2:

  • Open an existing group chat
  • Invite a new member by filling a non-existent MatrixId

Outcome

What did you expect?

if we do not recognise the MXID (= the getProfile failed (for example: M_NOT_FOUND error, or 502 code)) we should not show it in the ‘suggestions’ section

What happened instead?

The unrecognised mxId is added in suggestions:
Screenshot 2023-05-24 at 08 56 11

Operating system

No response

Browser information

No response

URL for webapp

No response

Application version

Element version: 1.11.31
Olm version: 3.2.14

Homeserver

No response

Will you send logs?

No

@weeman1337 weeman1337 added S-Minor Impairs non-critical functionality or suitable workarounds exist A-Invite O-Occasional Affects or can be seen by some users regularly or most users rarely labels May 30, 2023
@t3chguy
Copy link
Member

t3chguy commented May 30, 2023

if we do not recognise the MXID (= the getProfile failed (for example: M_NOT_FOUND error, or 502 code)) we should not show it in the ‘suggestions’ section

Synapse has settings which make it hide profiles unless you share a room, e.g. #23403 so doing that would prevent users starting DMs even if they had shared MXIDs out-of-band. Doubt this is what we want and is inherently only privacy by obscurity.

@t3chguy t3chguy added the X-Needs-Product More input needed from the Product team label May 30, 2023
@giomfo
Copy link
Member Author

giomfo commented May 30, 2023

@t3chguy in that case, the "Invite" button is still available to start DM or invite the unrecognised Id
(cc @daniellekirkwood )

@daniellekirkwood
Copy link
Contributor

I don't follow the technical constraint here --

If the MXID is invalid/unknown, we should hide the "suggestions" section.

@daniellekirkwood
Copy link
Contributor

Sorry, somehow managed to click too many buttons at once on the keyboard... Found some new shortcuts though 🤦

If you share a room with the user, it makes sense that that profile shows up in "suggestions". If you don't share a room or can't recognise the user it doesn't make sense that there's a "suggestion"...

Happy to loop in design here as they may have other ideas, or a way to rename the section from "Suggestion" to something more meaningful (as suggestion is not accurate).

Removing "Suggestions" doesn't stop the user from clicking invite?

@t3chguy
Copy link
Member

t3chguy commented May 30, 2023

I don't follow the technical constraint here --

If the MXID is invalid/unknown, we should hide the "suggestions" section.

The MXID isn't invalid, it's unknown because servers won't tell us definitively whether an MXID exists for privacy reasons.

@daniellekirkwood
Copy link
Contributor

but when you do know the MXID, it renders with a display name instead?

@t3chguy
Copy link
Member

t3chguy commented May 30, 2023

@daniellekirkwood Yes, if the server wishes to give you a profile then we will render it

@daniellekirkwood
Copy link
Contributor

daniellekirkwood commented May 30, 2023

can we have: if there's a profile then show 'SUGGESTIONS' else show something else ✨ ?

@t3chguy
Copy link
Member

t3chguy commented May 30, 2023

Sure

@daniellekirkwood
Copy link
Contributor

Have reached out to @nadonomy for a design consult. I think we either hide suggestions if there's no profile or we rename it to something more appropriate.

Alfie also had a bit of feedback on this topic so I will pass along there also :)

@untildo
Copy link

untildo commented May 30, 2023

I agree that hiding Suggestions that failed to retrieve a profile would be sensible. It doesnt make sense to suggest an unknown MXID, an invalid MXID, or a user with a irretrievable profile.

If the MXID is unknown to your homeserver then you should just manually type it out fully then click invite, as you would with email. If the MXID is known, a partial match is known, or a profile could be retrieved, then it would make sense to provide a suggestion.

If I search for the user @a:b, Element Web will suggest it above all other suggestions. While it is technically possible for @a:b to be a valid MXID, we all know it isnt and the current UI just creates ambiguity.

@t3chguy
Copy link
Member

t3chguy commented May 30, 2023

If the MXID is unknown to your homeserver

That isn't the issue here, your own server can hide all other users from you for privacy. You wouldn't want matrix.org to enumerate all users on the server to you as this is abusable.

we all know it isnt

Why isn't it? Private DNS is a thing, Element has no way of knowing whether your server is on a private subnet or not, 1 char localparts are a thing too

@untildo
Copy link

untildo commented May 30, 2023

That isn't the issue here, your own server can hide all other users from you for privacy. You wouldn't want matrix.org to enumerate all users on the server to you as this is abusable.

Yes sorry, poor wording on my part there but I think you get my point? The current behaviour of only displaying known (to you not your homeserver) is fine.

Why isn't it? Private DNS is a thing, Element has no way of knowing whether your server is on a private subnet or not, 1 char localparts are a thing too

Ok, poor wording again. We dont ALL know its invalid but I certainly do because I do not have a private DNS resolving the b domain. The point is that Element has no way of knowing so should not make suggestions about things it does not know.

@nadonomy
Copy link
Contributor

nadonomy commented Jun 1, 2023

@daniellekirkwood I stepped through this and I'm coming to the conclusion that the original suggestion from @giomfo is the best one - we should just omit a result if we don't know it, and the entire section if there's no content to display within it.

I looked at if we should rename suggestions to something else, but I think we'd end up re-tweaking this when we zoom into contact discovery and FTUE.

@daniellekirkwood
Copy link
Contributor

OK, that feels like the plan ahead then ☝️

@giomfo is this confirmed part of the retainer time?

@daniellekirkwood daniellekirkwood removed X-Needs-Design X-Needs-Product More input needed from the Product team labels Jun 5, 2023
@giomfo
Copy link
Member Author

giomfo commented Jun 5, 2023

is this confirmed part of the retainer time?

Yes I will ask @weeman1337 to work on it

@richvdh
Copy link
Member

richvdh commented Jun 13, 2023

Transferring some context from the pull request:

This reverts a change originally made to fix #12440. @turt2live gave some more context around the reasons for that change in this comment. In summary:

  • There are many good reasons why a /profile request may fail for a genuine user. A successful /profile request is a good indication that a user exists, but an unsuccessful one doesn't tell us a whole lot.
  • The fact that you can still click "Invite" when the user doesn't appear in the list is hard to discover.

I realise this thread has been around this loop before, but I just wanted to double-check that we still think this is a correct change to make in light of the fact that we've deliberately made the opposite decision in the past.

I don't have strong feelings either way, but I just want to be sure that the decisions are being made with all the information.

@nadonomy, @daniellekirkwood could you confirm, please?

@daniellekirkwood
Copy link
Contributor

I think our options are still the 2 highlighted above... Rename the section, or remove it. I'm happy with either one of these but I understand how some folks can see how the "invite" button might not be discoverable.

Suggestion:
I created this posthog graph that we can use to monitor any drop in create room and revert this change if needed
https://posthog.hss.element.io/insights/GS7jX5qe

@daniellekirkwood
Copy link
Contributor

Having read @turt2live 's feedback on the PR I'd like to re-iterate that the issue is mainly that Element "suggests" MXIDs that are invalid. Changing the title of the section when there's no profile found would help to clear up the confusion.

I do not agree that that "Invite" button is hard to find but as we're not looking at updating this screen for a little while, renaming the section would be a suitable solution for me atm.

👀 @nadonomy

@weeman1337
Copy link
Contributor

Some users have a closed network. They have profiles enabled and do not federate with the world. For them it would be good not to display invalid MXIDs. I can also understand that this may not work great in the context of (unknown) federation.

→ What about considering this as a config option? The option should not be visible to the user. It can just be in the JSON shipped with the client. This would meet the demands of users in a known, closed network.

@t3chguy
Copy link
Member

t3chguy commented Jun 14, 2023

It can just be in the JSON shipped with the client.

That'd require building & signing & notarising your own desktop apps. IMO this should be done at the spec level with a proper way to query whether an MXID exists which has a tri-state response. Yes|No|Unknown - where open federation servers return Yes|Unknown only depending on public presence, closed federations can switch to Yes|No if they deem it to be what they desire.

@daniellekirkwood
Copy link
Contributor

Confirmed with @nadonomy that we're comfortable removing the "Suggested" section (as planned) for all and keeping an eye on the PH graph to ensure users are still able to 'see the invite button'.

I believe there's a PR already going @weeman1337 ? Maybe @nadonomy and I just need to add our reviews there so we can move forwards here?

@richvdh
Copy link
Member

richvdh commented Jun 14, 2023

No need; thanks for confirming!

su-ex added a commit to SchildiChat/element-web that referenced this issue Feb 22, 2024
* Remove `feature_favourite_messages` as it is has been abandoned for now ([\#11097](matrix-org/matrix-react-sdk#11097)). Fixes element-hq#25555.
* Don't setup keys on login when encryption is force disabled ([\element-hq#11125](matrix-org/matrix-react-sdk#11125)). Contributed by @kerryarchibald.
* OIDC: attempt dynamic client registration ([\element-hq#11074](matrix-org/matrix-react-sdk#11074)). Fixes element-hq#25468 and element-hq#25467. Contributed by @kerryarchibald.
* OIDC: Check static client registration and add login flow ([\element-hq#11088](matrix-org/matrix-react-sdk#11088)). Fixes element-hq#25467. Contributed by @kerryarchibald.
* Improve message body output from plain text editor ([\element-hq#11124](matrix-org/matrix-react-sdk#11124)). Contributed by @alunturner.
* Disable encryption toggle in room settings when force disabled ([\element-hq#11122](matrix-org/matrix-react-sdk#11122)). Contributed by @kerryarchibald.
* Add .well-known config option to force disable encryption on room creation ([\element-hq#11120](matrix-org/matrix-react-sdk#11120)). Contributed by @kerryarchibald.
* Handle permalinks in room topic ([\element-hq#11115](matrix-org/matrix-react-sdk#11115)). Fixes element-hq#23395.
* Add at room avatar for RTE ([\element-hq#11106](matrix-org/matrix-react-sdk#11106)). Contributed by @alunturner.
* Remove new room breadcrumbs ([\element-hq#11104](matrix-org/matrix-react-sdk#11104)).
* Update rich text editor dependency and associated changes ([\element-hq#11098](matrix-org/matrix-react-sdk#11098)). Contributed by @alunturner.
* Implement new model, hooks and reconcilation code for new GYU notification settings ([\element-hq#11089](matrix-org/matrix-react-sdk#11089)). Contributed by @justjanne.
* Allow maintaining a different right panel width for thread panels ([\element-hq#11064](matrix-org/matrix-react-sdk#11064)). Fixes element-hq#25487.
* Make AppPermission pane scrollable ([\element-hq#10954](matrix-org/matrix-react-sdk#10954)). Fixes element-hq#25438 and element-hq#25511. Contributed by @luixxiul.
* Integrate compound design tokens ([\element-hq#11091](matrix-org/matrix-react-sdk#11091)). Fixes vector-im/internal-planning#450.
* Don't warn about the effects of redacting state events when redacting non-state-events ([\element-hq#11071](matrix-org/matrix-react-sdk#11071)). Fixes element-hq#8478.
* Allow specifying help URLs in config.json ([\element-hq#11070](matrix-org/matrix-react-sdk#11070)). Fixes element-hq#15268.
* Fix error when generating error for polling for updates ([\element-hq#25609](element-hq#25609)).
* Fix spurious notifications on non-live events ([\element-hq#11133](matrix-org/matrix-react-sdk#11133)). Fixes element-hq#24336.
* Prevent auto-translation within composer ([\#11114](matrix-org/matrix-react-sdk#11114)). Fixes element-hq#25624.
* Fix caret jump when backspacing into empty line at beginning of editor ([\#11128](matrix-org/matrix-react-sdk#11128)). Fixes element-hq#22335.
* Fix server picker not allowing you to switch from custom to default ([\element-hq#11127](matrix-org/matrix-react-sdk#11127)). Fixes element-hq#25650.
* Consider the unthreaded read receipt for Unread dot state ([\element-hq#11117](matrix-org/matrix-react-sdk#11117)). Fixes element-hq#24229.
* Increase RTE resilience ([\element-hq#11111](matrix-org/matrix-react-sdk#11111)). Fixes element-hq#25277. Contributed by @alunturner.
* Fix RoomView ignoring alias lookup errors due to them not knowing the roomId ([\element-hq#11099](matrix-org/matrix-react-sdk#11099)). Fixes element-hq#24783 and element-hq#25562.
* Fix style inconsistencies on SecureBackupPanel ([\element-hq#11102](matrix-org/matrix-react-sdk#11102)). Fixes element-hq#25615. Contributed by @luixxiul.
* Remove unknown MXIDs from invite suggestions ([\element-hq#11055](matrix-org/matrix-react-sdk#11055)). Fixes element-hq#25446.
* Reduce volume of ring sounds to normalised levels ([\element-hq#9143](matrix-org/matrix-react-sdk#9143)). Contributed by @JMoVS.
* Fix slash commands not being enabled in certain cases ([\element-hq#11090](matrix-org/matrix-react-sdk#11090)). Fixes element-hq#25572.
* Prevent escape in threads from sending focus to main timeline composer ([\element-hq#11061](matrix-org/matrix-react-sdk#11061)). Fixes element-hq#23397.
su-ex added a commit to SchildiChat/element-desktop that referenced this issue Feb 22, 2024
* Remove `feature_favourite_messages` as it is has been abandoned for now ([\#11097](matrix-org/matrix-react-sdk#11097)). Fixes element-hq/element-web#25555.
* Use brand and help url from config ([\element-hq#1008](element-hq#1008)).
* Don't setup keys on login when encryption is force disabled ([\#11125](matrix-org/matrix-react-sdk#11125)). Contributed by @kerryarchibald.
* OIDC: attempt dynamic client registration ([\#11074](matrix-org/matrix-react-sdk#11074)). Fixes element-hq/element-web#25468 and element-hq/element-web#25467. Contributed by @kerryarchibald.
* OIDC: Check static client registration and add login flow ([\#11088](matrix-org/matrix-react-sdk#11088)). Fixes element-hq/element-web#25467. Contributed by @kerryarchibald.
* Improve message body output from plain text editor ([\#11124](matrix-org/matrix-react-sdk#11124)). Contributed by @alunturner.
* Disable encryption toggle in room settings when force disabled ([\#11122](matrix-org/matrix-react-sdk#11122)). Contributed by @kerryarchibald.
* Add .well-known config option to force disable encryption on room creation ([\#11120](matrix-org/matrix-react-sdk#11120)). Contributed by @kerryarchibald.
* Handle permalinks in room topic ([\#11115](matrix-org/matrix-react-sdk#11115)). Fixes element-hq/element-web#23395.
* Add at room avatar for RTE ([\#11106](matrix-org/matrix-react-sdk#11106)). Contributed by @alunturner.
* Remove new room breadcrumbs ([\#11104](matrix-org/matrix-react-sdk#11104)).
* Update rich text editor dependency and associated changes ([\#11098](matrix-org/matrix-react-sdk#11098)). Contributed by @alunturner.
* Implement new model, hooks and reconcilation code for new GYU notification settings ([\#11089](matrix-org/matrix-react-sdk#11089)). Contributed by @justjanne.
* Allow maintaining a different right panel width for thread panels ([\#11064](matrix-org/matrix-react-sdk#11064)). Fixes element-hq/element-web#25487.
* Make AppPermission pane scrollable ([\#10954](matrix-org/matrix-react-sdk#10954)). Fixes element-hq/element-web#25438 and element-hq/element-web#25511. Contributed by @luixxiul.
* Integrate compound design tokens ([\#11091](matrix-org/matrix-react-sdk#11091)). Fixes vector-im/internal-planning#450.
* Don't warn about the effects of redacting state events when redacting non-state-events ([\#11071](matrix-org/matrix-react-sdk#11071)). Fixes element-hq/element-web#8478.
* Allow specifying help URLs in config.json ([\#11070](matrix-org/matrix-react-sdk#11070)). Fixes element-hq/element-web#15268.
* Fix error when generating error for polling for updates ([\#25609](element-hq/element-web#25609)).
* Fix spurious notifications on non-live events ([\#11133](matrix-org/matrix-react-sdk#11133)). Fixes element-hq/element-web#24336.
* Prevent auto-translation within composer ([\#11114](matrix-org/matrix-react-sdk#11114)). Fixes element-hq/element-web#25624.
* Fix caret jump when backspacing into empty line at beginning of editor ([\#11128](matrix-org/matrix-react-sdk#11128)). Fixes element-hq/element-web#22335.
* Fix server picker not allowing you to switch from custom to default ([\#11127](matrix-org/matrix-react-sdk#11127)). Fixes element-hq/element-web#25650.
* Consider the unthreaded read receipt for Unread dot state ([\#11117](matrix-org/matrix-react-sdk#11117)). Fixes element-hq/element-web#24229.
* Increase RTE resilience ([\#11111](matrix-org/matrix-react-sdk#11111)). Fixes element-hq/element-web#25277. Contributed by @alunturner.
* Fix RoomView ignoring alias lookup errors due to them not knowing the roomId ([\#11099](matrix-org/matrix-react-sdk#11099)). Fixes element-hq/element-web#24783 and element-hq/element-web#25562.
* Fix style inconsistencies on SecureBackupPanel ([\#11102](matrix-org/matrix-react-sdk#11102)). Fixes element-hq/element-web#25615. Contributed by @luixxiul.
* Remove unknown MXIDs from invite suggestions ([\#11055](matrix-org/matrix-react-sdk#11055)). Fixes element-hq/element-web#25446.
* Reduce volume of ring sounds to normalised levels ([\#9143](matrix-org/matrix-react-sdk#9143)). Contributed by @JMoVS.
* Fix slash commands not being enabled in certain cases ([\#11090](matrix-org/matrix-react-sdk#11090)). Fixes element-hq/element-web#25572.
* Prevent escape in threads from sending focus to main timeline composer ([\#11061](matrix-org/matrix-react-sdk#11061)). Fixes element-hq/element-web#23397.
su-ex added a commit to SchildiChat/matrix-react-sdk that referenced this issue Feb 22, 2024
* Remove `feature_favourite_messages` as it is has been abandoned for now ([\matrix-org#11097](matrix-org#11097)). Fixes element-hq/element-web#25555.
* Don't setup keys on login when encryption is force disabled ([\matrix-org#11125](matrix-org#11125)). Contributed by @kerryarchibald.
* OIDC: attempt dynamic client registration ([\matrix-org#11074](matrix-org#11074)). Fixes element-hq/element-web#25468 and element-hq/element-web#25467. Contributed by @kerryarchibald.
* OIDC: Check static client registration and add login flow ([\matrix-org#11088](matrix-org#11088)). Fixes element-hq/element-web#25467. Contributed by @kerryarchibald.
* Improve message body output from plain text editor ([\matrix-org#11124](matrix-org#11124)). Contributed by @alunturner.
* Disable encryption toggle in room settings when force disabled ([\matrix-org#11122](matrix-org#11122)). Contributed by @kerryarchibald.
* Add .well-known config option to force disable encryption on room creation ([\matrix-org#11120](matrix-org#11120)). Contributed by @kerryarchibald.
* Handle permalinks in room topic ([\matrix-org#11115](matrix-org#11115)). Fixes element-hq/element-web#23395.
* Add at room avatar for RTE ([\matrix-org#11106](matrix-org#11106)). Contributed by @alunturner.
* Remove new room breadcrumbs ([\matrix-org#11104](matrix-org#11104)).
* Update rich text editor dependency and associated changes ([\matrix-org#11098](matrix-org#11098)). Contributed by @alunturner.
* Implement new model, hooks and reconcilation code for new GYU notification settings ([\matrix-org#11089](matrix-org#11089)). Contributed by @justjanne.
* Allow maintaining a different right panel width for thread panels ([\matrix-org#11064](matrix-org#11064)). Fixes element-hq/element-web#25487.
* Make AppPermission pane scrollable ([\matrix-org#10954](matrix-org#10954)). Fixes element-hq/element-web#25438 and element-hq/element-web#25511. Contributed by @luixxiul.
* Integrate compound design tokens ([\matrix-org#11091](matrix-org#11091)). Fixes vector-im/internal-planning#450.
* Don't warn about the effects of redacting state events when redacting non-state-events ([\matrix-org#11071](matrix-org#11071)). Fixes element-hq/element-web#8478.
* Allow specifying help URLs in config.json ([\matrix-org#11070](matrix-org#11070)). Fixes element-hq/element-web#15268.
* Fix spurious notifications on non-live events ([\matrix-org#11133](matrix-org#11133)). Fixes element-hq/element-web#24336.
* Prevent auto-translation within composer ([\matrix-org#11114](matrix-org#11114)). Fixes element-hq/element-web#25624.
* Fix caret jump when backspacing into empty line at beginning of editor ([\matrix-org#11128](matrix-org#11128)). Fixes element-hq/element-web#22335.
* Fix server picker not allowing you to switch from custom to default ([\matrix-org#11127](matrix-org#11127)). Fixes element-hq/element-web#25650.
* Consider the unthreaded read receipt for Unread dot state ([\matrix-org#11117](matrix-org#11117)). Fixes element-hq/element-web#24229.
* Increase RTE resilience ([\matrix-org#11111](matrix-org#11111)). Fixes element-hq/element-web#25277. Contributed by @alunturner.
* Fix RoomView ignoring alias lookup errors due to them not knowing the roomId ([\matrix-org#11099](matrix-org#11099)). Fixes element-hq/element-web#24783 and element-hq/element-web#25562.
* Fix style inconsistencies on SecureBackupPanel ([\matrix-org#11102](matrix-org#11102)). Fixes element-hq/element-web#25615. Contributed by @luixxiul.
* Remove unknown MXIDs from invite suggestions ([\matrix-org#11055](matrix-org#11055)). Fixes element-hq/element-web#25446.
* Reduce volume of ring sounds to normalised levels ([\matrix-org#9143](matrix-org#9143)). Contributed by @JMoVS.
* Fix slash commands not being enabled in certain cases ([\matrix-org#11090](matrix-org#11090)). Fixes element-hq/element-web#25572.
* Prevent escape in threads from sending focus to main timeline composer ([\matrix-org#11061](matrix-org#11061)). Fixes element-hq/element-web#23397.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Invite O-Occasional Affects or can be seen by some users regularly or most users rarely S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants