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.

Sign up for GitHub

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

Jump to bottom

Room names are calculated incorrectly #13887

Closed
jmastr opened this issue Jun 2, 2020 · 25 comments · Fixed by matrix-org/matrix-js-sdk#2094
Closed

Room names are calculated incorrectly #13887

jmastr opened this issue Jun 2, 2020 · 25 comments · Fixed by matrix-org/matrix-js-sdk#2094
Assignees
@t3chguy
Labels
O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect

Comments

@jmastr
Copy link

jmastr commented Jun 2, 2020

Description

Alright, that looks like a riot-web bug, then. They are not following the spec by not ignoring alt_aliases

Sorunome/mx-puppet-slack#50
Sorunome/mx-puppet-slack#72

Steps to reproduce

  • /

Describe how what happens differs from what you expected.

Logs being sent: yes/no

Version information

  • Platform: web and desktop

For the web app:

For the desktop app:

  • OS: macOS 10.15.4
  • Version: 1.6.2
@jmastr jmastr added the T-Defect label Jun 2, 2020
@t3chguy
Copy link
Member

t3chguy commented Jun 2, 2020

its a js-sdk bug if anything, but can stay here for visibility

@jmastr
Copy link
Author

jmastr commented Jun 2, 2020

@t3chguy is that the same reason why it works in the iOS app correctly?

@t3chguy
Copy link
Member

t3chguy commented Jun 2, 2020

the iOS app is an entirely separate codebase

@jmastr
Copy link
Author

jmastr commented Jun 2, 2020

Thought so. The maintainer mx puppet Slack bridge says: “spec says that alt_aliases must be ignored when calculating the room name, which doesn't seem to be the case here”. Does that mean anything to you? Not following the specs in this case?

@t3chguy
Copy link
Member

t3chguy commented Jun 2, 2020

The spec says Note that clients should avoid using alt_aliases when calculating the room name.
Note it does not say must so it is not in breach of the spec.

@jmastr
Copy link
Author

jmastr commented Jun 2, 2020

Thanks for clarifying. Will forward the information.

@jmastr jmastr closed this as completed Jun 2, 2020
@turt2live
Copy link
Member

Note it does not say must so it is not in breach of the spec.

The intention is that it's a strong recommendation due to trust issues. We really shouldn't be relying upon them.

@t3chguy
Copy link
Member

t3chguy commented Jun 2, 2020

Indeed, am not saying it shouldn't be changed, am just saying its not a spec compliance breach

@t3chguy t3chguy reopened this Jun 2, 2020
@t3chguy
Copy link
Member

t3chguy commented Jun 2, 2020

trust issues

what trust issues though? isn't alt_aliases controlled by room admins?

@t3chguy
Copy link
Member

t3chguy commented Jun 2, 2020 •
edited
Loading

Very simple to implement by removing https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/models/room.js#L1843-L1849 - this needs a product decision and to be consistent between Riot apps.

@t3chguy t3chguy added the X-Needs-Product More input needed from the Product team label Jun 2, 2020
@turt2live
Copy link
Member

what trust issues though? isn't alt_aliases controlled by room admins?

Moderators, but yes. The challenge is that they are expected to be on different servers which can more easily change where they point without ability to update them, leading to inaccuracies. It's a similar problem with the canonical alias, though much less likely (as it'd only drift in the case of abuse or server admin action).

There's also a theory that if you want your room name to match the alt_aliases, you should make one of them canonical.

@jmastr
Copy link
Author

jmastr commented Jun 3, 2020

Thanks for all the effort!

and to be consistent between Riot apps.

Don't know where to find it in source code, but like I mentioned above Riot iOS app has this behaviour already.

@matejdro
Copy link

matejdro commented Jun 4, 2020 •
edited
Loading

Just want to mention that RiotX on android also handles that properly.

Maybe this trust issue could be resolved by giving user a toggle in Room Settings that allows him to decide whether to use alt aliases or not? Of course, step further from this would be https://github.com/vector-im/riot-web/issues/3130

@jmastr
Copy link
Author

jmastr commented Jun 8, 2020

@t3chguy is this enough in terms of product decision?

@t3chguy
Copy link
Member

t3chguy commented Jun 8, 2020

@jmastr sorry, is what enough in terms of a product decision?

@jmastr
Copy link
Author

jmastr commented Jun 8, 2020

@t3chguy sorry, highlighted the wrong text:

and to be consistent between Riot apps

is there any more apps I am not aware of?

@t3chguy
Copy link
Member

t3chguy commented Jun 8, 2020

Right, but the mobile apps made the decision autonomously and it may be undesirable, so product has to make the decision which all Riot clients should follow

@seanfarley
Copy link

Thanks for starting the conversation, @jmastr; I, too, am seeing this behavior.

@Sorunome
Copy link

please note that leaving alias as null and providing an alt_alias gives bridges an easy way to e.g. give a DM room an alias and not have riot FUBAR the room name

@jmastr
Copy link
Author

jmastr commented Jun 23, 2020

@t3chguy @turt2live What is the process of getting a product decision?

@t3chguy
Copy link
Member

t3chguy commented Jun 23, 2020

Waiting. Its on their TODO.

@yousefamar
Copy link

I was wondering if anything has happened on this front? Thanks

@fancypantalons
Copy link

Agreed, I'd really like to see movement on this. Right now the Slack puppet bridge is basically unusable for me due to the broken group chat names that are a consequence of this issue.

@dreamflasher
Copy link

Currently there's an inconstency between riot web and mobile. @t3chguy If it's that easy to implement, can we just remove those lines, to make it consistent, and whenever the product people get the time (seriously they weren't able to decide this within one year‽) they can revert it.

@t3chguy t3chguy self-assigned this Jan 7, 2022
@kittykat kittykat removed the T-Defect label Jan 7, 2022
@kittykat kittykat added O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect and removed X-Needs-Product More input needed from the Product team labels Jan 7, 2022
@t3chguy t3chguy mentioned this issue Jan 7, 2022
Merged
@kittykat
Copy link
Contributor

kittykat commented Jan 7, 2022

Thank you all for the reports and comments. The team at Element are making an active effort to triage and resolve these older issues and your patience is appreciated.

su-ex added a commit to SchildiChat/matrix-js-sdk that referenced this issue Jan 17, 2022
@su-ex
Merge tag 'v15.4.0' into sc
5c6b029 
* Don't consider alt_aliases when calculating room name ([\matrix-org#2094](matrix-org#2094)). Fixes element-hq/element-web#13887.
* Load room history if necessary when searching for MSC3089 getFileEvent() ([\matrix-org#2066](matrix-org#2066)).
* Add support for MSC3030 `/timestamp_to_event` ([\matrix-org#2072](matrix-org#2072)).
* Stop encrypting redactions as it isn't spec compliant ([\matrix-org#2098](matrix-org#2098)). Fixes element-hq/element-web#20460.
* Fix more function typings relating to key backup ([\matrix-org#2086](matrix-org#2086)).
* Fix timeline search in MSC3089 getFileEvent() ([\matrix-org#2085](matrix-org#2085)).
* Set a `deviceId` for VoIP example and use `const`/`let` ([\matrix-org#2090](matrix-org#2090)). Fixes matrix-org#2083. Contributed by @SimonBrandner.
* Fix incorrect TS return type for secret storage and key backup functions ([\matrix-org#2082](matrix-org#2082)).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@t3chguy t3chguy

Labels
O-Frequent Affects or can be seen by most users regularly or impacts most users' first experience S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Don't consider alt_aliases when calculating room name matrix-org/matrix-js-sdk
10 participants
@seanfarley @dreamflasher @kittykat @fancypantalons @matejdro @yousefamar @turt2live @t3chguy @Sorunome @jmastr