flarum / issue-archive

0 stars 0 forks source link

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

Open luceos opened 5 years ago

luceos commented 5 years ago

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:

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 https://github.com/flarum/core/pull/1721#issuecomment-451306374)

Describe alternatives you've considered

🤷‍♀️

luceos commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 4 years ago

So basically, we have three things here, right?

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

luceos commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

askvortsov1 commented 3 years ago

Can we unparse text content? User model slugger?

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

SychO9 commented 3 years ago

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 commented 3 years ago

@"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 commented 3 years ago

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 commented 3 years ago

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

SychO9 commented 3 years ago

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:

Slugs

Other

Anything I'm missing ?

askvortsov1 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

SychO9 commented 3 years ago

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

Ah.. I'd forgotten about post mentions, well that's annoying.. does the username part of the post mention matter at all ? or is it just there for a more user friendly text ?

If the latter, we could do something along the lines of having post mentions as @"Display Name"#p5268 and user mentions @"Display Name"#4589 ? p would allow us to make the difference between the models we're targeting, though I'm not sure anymore.

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.

I mean, couldn't we just use the ID part of the slug ID-Daniël-Klabbers, the rest would just be, a nicety, like we do for discussions I believe ?

tankerkiller125 commented 3 years ago

We can still work with two hashes, so long as we know that the first one is the user id and the second is a post id always. Another option might be using the ! symbold for the user? So it'd become @"Display Name"!5151#51651

SychO9 commented 3 years ago

Well, the thing is in the post mention, I don't believe the user id is necessary at all, at a first glance the current code doesn't need it to retrieve a user, since all it needs is the post id to access the user (author). Hence why I suggested we use a minimal format that would simply allow us to differentiate between user IDs and Post IDs and potentially group IDs.

Any other formats are not really user-friendly.

I'll need to play around with the code to have a better view of the current implementation.

askvortsov1 commented 3 years ago

I agree that including both IDs is reduntant. We just need to ensure that this format makes sense to users.

tankerkiller125 commented 3 years ago

That works for me, I don't care if we use a letter or a symbol to differentiate so long as it works and it's just as easy to use as it is today.

SychO9 commented 3 years ago

Alright then, I'll start moving forward with the implementation of the mentions format, we can discuss it more as we go, and we could potentially ask the community for feedback on this.

dsevillamartin commented 3 years ago

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.

Does this help the issue of user data remaining when the user is deleted? I thought that was one of the main things to tackle, and this description confuses me as to how that is achieved.

Will we be replacing the @"Display Name"#ID in the content of the post itself when saving? Because if not, the user's display name will still be in the post, which is part of their user data we'd want deleted (right?).

askvortsov1 commented 3 years ago

Will we be replacing the @"Display Name"#ID in the content of the post itself when saving? Because if not, the user's display name will still be in the post, which is part of their user data we'd want deleted (right?).

Beautiful thing about Flarum's post system is we don't actually store raw content; we store TextFormatter parsed content. So a post that looks like:

@admin 

will be stored as

<r><p><USERMENTION displayname="admin" id="1" username="admin">@admin</USERMENTION></p> </r>

Therefore, all we'd need to do is change the way tags are unrendered, IIRC.

dsevillamartin commented 3 years ago

If we do that I'd remove the displayname completely - because I assume when the user goes to edit they'd still be seeing the display name of a deleted user, for example ?

askvortsov1 commented 3 years ago

What I linked above is how it's stored now. I agree that cache invalidation is an issue, but imo it's an issue for another day. Probably for a script that would run regularly when load is low, and recalculate display name and username.

tankerkiller125 commented 3 years ago

We can't completely remove display names, that would make things very confusing for end users.

dsevillamartin commented 3 years ago

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.

davwheat commented 3 years ago

I'm personally against the display name part of the mention.

Mentions should stay up to date with whatever is that users latest name in my opinion.

askvortsov1 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

davwheat commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.