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

Increase username flexibility/privacy with mentions and user profile #254

Open
luceos opened this issue Jan 25, 2019 · 55 comments
Open

Increase username flexibility/privacy with mentions and user profile #254

luceos opened this issue Jan 25, 2019 · 55 comments
Assignees

Comments

@luceos
Copy link
Member

luceos commented Jan 25, 2019

Feature Request

Is your feature request related to a problem? Please describe.

When you have a forum that tries to protect usernames (as they are used for logging in too) by adding the display_name (eg I've migrated that column onto the users table), the profile url and mentioning logic should be able to use that column instead of the username column.

Describe the solution you'd like
I'm pretty blocked right now and would like your input for the best solution:

  • Dispatch events on those specific controllers to allow "finding" the user.
  • Dispatch an event or allow configuration of the field to use for users (this is most likely prone to error).

Justify why this feature belongs in Flarum's core, rather than in a third-party extension

Ease of configuration and protecting user privacy. This might also relate to the PR Toby did flarum/framework#1721 (see point two in flarum/framework#1721 (comment))

Describe alternatives you've considered

🤷‍♀️

@luceos
Copy link
Member Author

luceos commented Jan 25, 2019

@tobscure you might have an opinion on this as you might have already ran into this with that client, maybe I missed something that core allows here.

@tobyzerner
Copy link

My feeling is that usernames should always be considered public, they are like a friendly "user ID". We could add an option to only allow logging in via email address for enhanced security?

@luceos
Copy link
Member Author

luceos commented Apr 11, 2019

Doesn't that complicate the idea of display- vs username? Isn't displayname supposed to be public, not the other way around? Using email as sole authorization might not be the best bet either, communities might allow multiple users with the same mail address (role playing boards for instance).

@tobyzerner
Copy link

tobyzerner commented May 13, 2019

Not necessarily, the distinction is not intended to be public vs. private but just machine-friendly (something that can be used for @mentions - eg. Toby001) vs human-friendly (eg. a full name like Toby Zerner)

Using email as sole authorization might not be the best bet either, communities might allow multiple users with the same mail address (role playing boards for instance).

I don't know if we should allow this, people can always sign up multiple accounts by using + in their email address (eg. example+account1@example.com). It's pretty commonplace that emails are unique for accounts across the internet

@luceos
Copy link
Member Author

luceos commented May 15, 2019

I think we differ in opinion where we talk about "we should allow this", in my opinion I would formulate this "we should allow this by default".

Display name really feels like something publicly usable and visible, whereas username is something that could/should be private (by default).

@tobyzerner
Copy link

tobyzerner commented May 15, 2019

If username is private, how do you @mention a user? Typing their display name is not an option as it could have whitespace in it. The other option is to use their user ID (@123) but that's not very nice. (One other option is to implement a rich text editor by default which can do fancy highlighted @mentions like Facebook does them, showing the display name but storing the user ID internally, but that is very complicated and a large amount of dev work.) Hence why I think we should just keep it simple, make usernames always public, and display names are just an extra thing on top of that if you want to allow people to show their full name etc.

@luceos
Copy link
Member Author

luceos commented May 17, 2019

Let's agree to disagree 👍

To me the Facebook way of mentioning is what I had in mind. We can always revisit this later.

@tobyzerner
Copy link

To me the Facebook way of mentioning is what I had in mind

Actually I agree if we were to implement that then we no longer need usernames to be public. It's just a challenging thing to implement, but we should aim for it in the long-term.

@franzliedke
Copy link

franzliedke commented Jan 3, 2020

So basically, we have three things here, right?

  • Login identifier (username or email)
    • should be kept private if possible
    • username serves as fallback for display name, as that is probably optional (especially for BC)
  • Display name
    • public
    • allow as many characters as possible
    • can be used in autocompletion for mentions
  • Slug

Does this cover all the aspects mentioned here and in related issues?

@luceos
Copy link
Member Author

luceos commented Jan 8, 2020

According to our code, a login identifier is either a username or email address. So your summary requires specification. I personally think both of them should be kept private.

@dsevillamartin
Copy link
Member

I think it'd be best to use Discord's approach, where the actual content of mentions is <@ID> but the user sees @NICKNAME#DISCRIM. In other words, the mention gets converted to a user ID reference in the DB but the user never sees it.

@askvortsov1
Copy link
Sponsor Member

If we're changing this, should we consider storing it as <@U:ID> to provide flexibility for group mentions down the line?

@askvortsov1
Copy link
Sponsor Member

Can we unparse text content?
User model slugger?

@<"Daniel Klabbers"(12434234324)> potential format?

@SychO9
Copy link
Member

SychO9 commented Mar 22, 2021

Trying to catch up with this issue. Whichever format we choose, it'll essentially be what the user sees in the editor correct ? I think we should probably aim for the most user friendly format possible, couldn't we go for something more simple like: @"Daniël Klabbers"#3968 or do we see potential parsing issues with that, hence why the suggested @<"Daniel Klabbers"(12434234324)> ?

@askvortsov1
Copy link
Sponsor Member

@"Daniël Klabbers"#3968

That format makes sense to me, I think I was being a bit too safe with my suggestion. As long as it can be parsed (which surrounding the display name part with parentheses accomplishes) it should be good.

One thing I'd like to make sure is that the format we choose hypothetically supports groups, but what you proposed should work:

@g"Mods"#12 (or some other modification that either goes before the hash or after the @.

@tankerkiller125
Copy link

tankerkiller125 commented Mar 23, 2021

I think that works well enough. I can provide regex for those if it's needed, in my head at the moment regex seems like the easiest method to parse that?

@askvortsov1
Copy link
Sponsor Member

We'll just need to modify the current system, which uses regex yes.

@SychO9
Copy link
Member

SychO9 commented Mar 23, 2021

One thing I'd like to make sure is that the format we choose hypothetically supports groups, but what you proposed should work:

Yeah, for groups I would personally go for something like @"Mods"#g256, but the choice of format for groups is of course irrelevant right now, as long as it is supported by whichever format we choose. And I believe the above does.

I can provide regex for those if it's needed

I'm sure there is regex involved yes, so feel free!


So essentially this is what needs to be done:

Mentions:

  • Change the format to @"Display Name"#ID.
  • Make sure the backend translates old mentions to the new format, that'll also apply for when admins change display name drivers, the mentions will only be updated when the user edits and saves a post.

Slugs

  • Introduce an ID only slug for users.
  • Introduce a slugger for nicknames, in the format ID-Daniël-Klabbers ?

Other

  • Hide the username from the relevant payloads.

Anything I'm missing ?

@SychO9 SychO9 self-assigned this Mar 23, 2021
@askvortsov1
Copy link
Sponsor Member

Introduce a slugger for nicknames, in the format ID-Daniël-Klabbers ?

Why? Isn't the whole point of this that we include the ID in mentions so that we can entirely rely on that, and an arbitrary display name can be injected?

Also, one thing we're missing here is a syntax for post mentions. Not sure if 2 pound signs in a row would be that good a UI.

@SychO9
Copy link
Member

SychO9 commented Mar 23, 2021

Why? Isn't the whole point of this that we include the ID in mentions so that we can entirely rely on that, and an arbitrary display name can be injected?

I didn't say we'd be relying on it for mentions, it's only a nice-to-have since hiding usernames will mean using the ID only slug for URLs, it's not a necessity so we don't have to do it for now, but it's easy to implement anyway.

Also, one thing we're missing here is a syntax for post mentions. Not sure if 2 pound signs in a row would be that good a UI.

2 pound signs ? what's that..

@askvortsov1
Copy link
Sponsor Member

I didn't say we'd be relying on it for mentions, it's only a nice-to-have since hiding usernames will mean using the ID only slug for URLs, it's not a necessity so we don't have to do it for now, but it's easy to implement anyway.

Thing is, nicknames are not required to be unique. Even if that option is enabled, it's possible that non unique nicknames have been used.

2 pound signs ? what's that..

Hash signs (#), sorry. This symbol has too many names lol.

Rigjt now post mentions are @username#POSTID. If we switch to the proposed system, we'd get @"Display Name"#UserID#PostID

@askvortsov1
Copy link
Sponsor Member

I didn't say to remove them for users. We'd be removing them in the backend, and then using the syntax like @"Display Name"#ID on the frontend so the user still knows who they're talking about. I think this should be possible as we load the users that are mentioned.

This would use latest display name for user & show [deleted] for deleted users.

I don't recall if that's how it currently works (it might be since we're setting this stuff on both render and on tag definition). However, if it's not how we currently do it, I'd want to be very careful: we could be adding a lot of new queries if we just recalculate every mentioned user on every post load.

@dsevillamartin
Copy link
Member

Why would we need to recalculate every mentioned user on every post load? I didn't say anything about recalculating. The post content will contain its ID, and the post_mentions table contains the relationship - the users are loaded with the post now, and so we'd use that information to display it to the user.

@davwheat
Copy link
Member

davwheat commented Mar 24, 2021

Ah, so we store it as just the user ID on the backend and then convert it to include the display name as we send it to the front-end?

I don't think that we should start adding display names that stay the same if a user changed username. That seems like an easy way to confuse people looking at older posts.

If I changed my name to MrJeeves on a forum, I'd expect previous mentions to update too. If it has davwheat in some places and MrJeeves in others, that's asking for confusion.

@askvortsov1
Copy link
Sponsor Member

I have confirmed from testing that the displayed mention will automatically update when the display name is changed. It needs an adjustment to handle deleted users, but that should be pretty easy.

@dsevillamartin
Copy link
Member

When you talk about displayed you're talking about a normal post right? If the user edits the post though, don't they see their original text with an old username/display name?

@davwheat
Copy link
Member

If that's how that works, what's the point of including the display name in the first place? Can't that be easily fetched using the user ID?

@dsevillamartin
Copy link
Member

If that's how that works, what's the point of including the display name in the first place? Can't that be easily fetched using the user ID?

I think for the user to know who they're mentioning, but we'd want to parse this out in the DB and then replace the mention w/ USER ID with the syntax we choose for mentions when showing he post content when editing (or hitting the endpoint for normal content field I guess?)

@askvortsov1
Copy link
Sponsor Member

When you talk about displayed you're talking about a normal post right? If the user edits the post though, don't they see their original text with an old username/display name?

Ah, I see what you mean now. Yeah that's something we'd want to adjust in the new system.

@dsevillamartin
Copy link
Member

@askvortsov1 Yeah, sorry I explained myself poorly. I realized late that we were talking about different things - rendered post VS editing.

@davwheat
Copy link
Member

So can't we just store the mention with the user ID in the DB, then add in their current display name when we send it off to the front end? Would that be too difficult?

@askvortsov1
Copy link
Sponsor Member

askvortsov1 commented Mar 24, 2021

So can't we just store the mention with the user ID in the DB, then add in their current display name when we send it off to the front end? Would that be too difficult?

We'd need some way to hook into the unparsing process. We already wrap unparsing in our own method (https://github.com/flarum/core/blob/d642fb531c1335c33ff7515da367f6815ea93b12/src/Formatter/Formatter.php#L97-L106), so we could insert an extender there, allow a chance to edit the raw XML before proceeding? The utils we use for rendering should work there too.

@matteocontrini
Copy link

It needs an adjustment to handle deleted users, but that should be pretty easy.

Just chiming in to say that this is also a privacy concern. If you are requested to remove PII for a user there's currently no way to make the username disappear from mentions, which could be the full name of the person, for example when using Facebook login.

@askvortsov1
Copy link
Sponsor Member

It needs an adjustment to handle deleted users, but that should be pretty easy.

Just chiming in to say that this is also a privacy concern. If you are requested to remove PII for a user there's currently no way to make the username disappear from mentions, which could be the full name of the person, for example when using Facebook login.

Absolutely agreed. In planning discussions for the GDPR extension's erasure feature this was discussed as a necessity, but if we can get it implemented in the mention extension itself (which we should be able to do), that'd be even cleaner.

@SychO9
Copy link
Member

SychO9 commented Apr 12, 2021

Assuming someone has a display name of Display "#p9 Name makes things a bit more complicated, we'd have to explicitly remove that from display names when formatting, it's a very special case scenario.

@askvortsov1
Copy link
Sponsor Member

Assuming someone has a display name of Display "#p9 Name makes things a bit more complicated, we'd have to explicitly remove that from display names when formatting, it's a very special case scenario.

We could just escape double quotes, maybe?

@SychO9
Copy link
Member

SychO9 commented Apr 12, 2021

I'm afraid that would make the regular expressions more complicated because of how complex escaping really is, we'd also have to account for escaping at the start and at the end of the real double quotes.

@askvortsov1
Copy link
Sponsor Member

Since the User serializer now contains slug and display name, would the next step be here? I suppose we should stop serializing the username unless the actor has the edit credentials permission, but there are still several bits that rely on that, such as the post/discussion filters.

@SychO9
Copy link
Member

SychO9 commented Apr 21, 2021

Yes, what would we replace the username filter with ? I think the slug is what makes sense here ?

@askvortsov1
Copy link
Sponsor Member

Yes, what would we replace the username filter with ? I think the slug is what makes sense here ?

Slug makes sense because it's a unique identifier, but it's not ideal because we'd have to make one query per-username, whereas currently we can wrap everything up into a single query.

@tankerkiller125
Copy link

Would something like user:"Display Name" not work?

@askvortsov1
Copy link
Sponsor Member

Would something like user:"Display Name" not work?

Display names are not a one to one relation. Also, that still has the issue of needing to query users individually.

What we could do is support both ID and slug, and convert any slugs to IDs?

Alternatively, people are unlikely to search for THAT many authors at once, so the multiple queries could be fine for slug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants