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

`User.create` should encode usernames #5362

Open
jsumners opened this issue Jan 12, 2017 · 17 comments

Comments

Projects
None yet
9 participants
@jsumners
Copy link

commented Jan 12, 2017

jsumners/nodebb-plugin-sso-discord-alt#5

The User.create API method should encode usernames so that names like "Mercurial | corpnewt.com" do not cause an "invalid username" error.

@julianlam

This comment has been minimized.

Copy link
Member

commented Jan 12, 2017

They are encoded for slugs... the issue is that | is not a valid username character 😄

@jsumners

This comment has been minimized.

Copy link
Author

commented Jan 12, 2017

I know that's the issue. I'm saying it shouldn't be an issue:

> encodeURIComponent('Mercurial | corpnewt.com')
'Mercurial%20%7C%20corpnewt.com'
@julianlam

This comment has been minimized.

Copy link
Member

commented Jan 12, 2017

@jsumners Right, but I'd argue that running the input through encodeURIComponent leads to a degraded user experience.

I can almost guarantee that if we do this, we'll get reports about this being a bug, even if intentional 😄 Laymen do not understand encoded character values. Perhaps I am misunderstanding your proposed solution?

We could add | to the character whitelist...

@jsumners

This comment has been minimized.

Copy link
Author

commented Jan 12, 2017

My position is that there shouldn't be any concept of an invalid username. Whatever the person decides to name themselves should be valid. Internally, you can encode them however you like, encodeURIComponent being one way. But when you present the name to a user, it should be decoded to the way the person behind the name originally typed it.

If you are concerned about how it looks in the URL bar as a "slug", then you can map invalid URI characters to something else specifically for that situation.

@dhinakg

This comment has been minimized.

Copy link

commented Jan 12, 2017

I agree with @jsumners. A lot of plugins crash NodeBB because of invalid characters, and encoding should be added somehow.

@pichalite

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2017

@dhinakg which plugins are included in the "lot of plugins"? example?

@dhinakg

This comment has been minimized.

Copy link

commented Jan 12, 2017

For example, nodebb-plugin-sso-discord-alt, and pretty much any other plugin that creates users and can possibly insert an invalid character.

@nodebb-misty

This comment has been minimized.

Copy link
Contributor

commented Jan 20, 2017

Issue closed due to inactivity.


This is an automated message. If you feel this action was in error, please comment on this issue so it can be looked at again

@jsumners

This comment has been minimized.

Copy link
Author

commented Jan 20, 2017

That's a fantastic way to deal with issues -- just automatically close them when you don't deal with them.

@julianlam

This comment has been minimized.

Copy link
Member

commented Jan 20, 2017

@jsumners -- what a horrible sentiment to wake up to on a Friday morning.

I understand how frustrated you feel. Misty is a bot and she closes issues that are inactive in a heavy-handed attempt to ensure that our issue tracker is not overwhelmed. The last thing I want to do is get into a situation where we have ten thousand open issues and anyone who posts an issue knows full well it won't even be looked.

I hope in turn you will understand that NodeBB is a labour of love that we maintain for free. Our client-work is separate from our open-source work and it is discouraging that someone would go as far as to publicly shame someone else for not resolving his issue (for free, mind you) fast enough.

You have to understand that these issues are always on the back of my mind. Even when they are closed by Misty. Nice-to-haves and low priority non-bugs that we will get to in time.

I read through your issue. I gave you an honest argument about how it has been working fine so far, and that a workaround would be to not use the pipe character in a username. Your latest reply made me think that I am incorrect in my assessment, and out of embarrassment, I chose not to reply so I could craft a more meaningful response when I had enough time. I told myself that you were right, and that invalid usernames shouldn't be possible, much like invalid posts aren't a thing in NodeBB. I want to fix this issue, but not anymore.

I'm finished with this issue. You will have to rely on someone else on this project to overhaul the username validator code. Alternatively, you can make the changes yourself and choose whether or not to propagate these changes back to the core.

@julianlam julianlam reopened this Jan 20, 2017

@jsumners

This comment has been minimized.

Copy link
Author

commented Jan 20, 2017

@julianlam I don't care how long an issue takes to resolve. But I do care that a bot is in place to simply close issues due to non-activity. That's not a proper way to resolve an issue.

  1. It makes it look to the filer that you can just ignore issues and they will "go away".
  2. It artificially "improves" your issue count numbers. This makes it look like the project cares more about how well it is perceived to be developed/managed than how it might actually be.
  3. It imposes upon you a time limit to properly resolve issues lest the bot come along and close them without resolution.

If you personally don't want to deal with the issue any longer, that's fine. My post in response to the close was merely a direct response to the way open issues are evidently handled. It's not something I have ever seen with any other project. At the very least, a bot comes along and pings those involved at least once to do something; only after a ping, or several, would the bot close the issue.

If an issue is a "nice to have, maybe later" then simply add an "enhancement" or "request-for-assistance" or some such label to it.

@pitaj

This comment has been minimized.

Copy link
Contributor

commented Jan 20, 2017

Issues get closed due to inactivity because often we reply to an issue and get no further information. Misty does this automatically for precisely this reason, to remind people to continue giving information.

Before I mark this issue with some different label, please give some more examples of different usernames which are currently not allowed and should be by your standard.

@jsumners

This comment has been minimized.

Copy link
Author

commented Jan 20, 2017

@pitaj I don't know of any. But I also don't know what anyone in the world feels like naming themselves on the Internet.

I'm really not sure what other information you need. As @julianlam said, I have given a reasoned description of how I think any username could be handled. Clearly he understood it, he just didn't acknowledge it until today.

Aside from learning your codebase and fix it myself, which I don't really have time to do, I don't know what else to contribute to the issue.

@nodebb-misty

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2017

Issue closed due to inactivity.


This is an automated message. If you feel this action was in error, please comment on this issue so it can be looked at again

@a632079

This comment has been minimized.

Copy link

commented Apr 2, 2017

I find that in 彩阳(新) will lead this problem too.

@Dravere

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2017

I'm really interested in this as well. And I might even have some time to look into this. Do you by chance have some tips/hints for what I should look out. I'm aware of public/src/utils.js with isUserNameValid and slugify. Is there more that should be taken into consideration?

@Dravere

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2017

This is getting more complicated the longer I look at it. The slug is mostly used in URLs which I wouldn't see as a problem. Most browsers try to hide the path of the URL to the users anyway. And most laymen don't even now what the URL or the address bar actually is. In those cases I don't think it would be a problem.

But utils.slugify is also used in different cases. For example to create titles or mentions. Replacing an @ mentioning with encoded URI components definitely would hurt the user experience. On the other hand one could ask why is the mention even replaced with the slug?

Also I see some problems in backwards compatibility. I think I found places in the code where the slug is regenerated instead of taken from the database. Or the slug is compared in renames to check if a different name was entered.

This is kind of sad. I was really hoping this would be easier to introduce. Especially the backwards compatibility worries me a bit.

@julianlam What are the policies for backward compatibility? Could a ./nodebb upgrade update all slugs of every user? Or would that be out of scope?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.