Mattermost-server: Add a config.json setting for minimum username length

[esethna]

If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide.

---

Notes: Jira ticket

Context: We require usernames to be 3 characters or more because MySQL doesn't support searching for shorter strings by default. Postgres supports them by default and MySQL can be configured to support them, so we could add a config.json setting that the SysAdmin can change in those cases. This would be similar to the Minimum Hashtag Length setting added in Mattermost 5.10.

Use Cases:

1. For CJK teams, 2 letter words are common.
2. Teams use an email/password login where usernames of their team are the user's initials, which can be 1 or 2 characters long.

Community Requests:

• pr-mattermost-webapp-2635
• pr-mattermost-webapp-1806
• https://mattermost.uservoice.com/forums/306457-general/suggestions/16391320-changing-min-username-to-2-characters

Details

1) A MinimumUsernameLength setting added to the config.json, part of the ServiceSettings section.
2) An entry in the System Console to change that setting, added under Site Configuration > Users and Teams, below Teammate Name Display.

Minimum Username Length [ 3 ]
Help Text: "Minimum number of characters in a username. MySQL databases must be configured to support searching strings shorter than three characters, see documentation."

Mattermost docs: https://docs.mattermost.com/install/requirements.html?highlight=ft_min_word_len#database-software
MySQL docs: https://dev.mysql.com/doc/refman/8.0/en/fulltext-fine-tuning.html

3) The USER_NAME_MIN_LENGTH constant should be removed and this setting should be used in its place.

4) Complete coverage through automated unit tests added - given username search occurs throughout the UI, we want to make sure we have adequate testing coverage for it without increasing release testing Mana.

5) Testing across PostgreSQL, MySQL and Elasticsearch.

Notes:

To start the work for this issue, you can take a look at these PRs where @carmo-evan had started the work:

• mattermost-server: https://github.com/mattermost/mattermost-server/pull/11028
• mattermost-webapp: https://github.com/mattermost/mattermost-webapp/pull/2895

[carmo-evan]

I'd like to do this one.

[hanzei]

Thanks @carmo-evan!

[jasonblais]

Hey @carmo-evan - I think you may not be working on this issue anymore so I added Up For Grabs label back - if you are, let us know! :)

[jasonblais]

Sorry! I forgot you had PRs submitted 🙏

[carmo-evan]

I apologize, but the PR is outdated and I currently don't have the capacity to finalize this.

[jasonblais]

@carmo-evan no worries, thanks for the heads up! I've referenced your PRs in the help wanted issue and added Up For Grabs label back. If you'd like, you may close the PRs you opened

[RajatVaryani]

@jasonblais Can I own this up?

[jasonblais]

@RajatVaryani certainly, thank you!

[RajatVaryani]

@jasonblais I did not get the chance to work on this. A PR will be raised pretty soon.

[jasonblais]

@RajatVaryani Thanks for the heads up, looking forward to reviewing the PR! Appreciate your help with this :)

[RajatVaryani]

Hello. Can we please correct the name of the constant to be USER_NAME_MIN_LENGTH in the issue as well as in the jira ticket?

[jasonblais]

@RajatVaryani Thanks! I have corrected it

[RajatVaryani]

As first step of the story I am making changes on server side. To do so I have to remove USER_NAME_MIN_LENGTH constant from mattermost/mattermost-server/model/user.go. There's a method as well IsValidUsername which uses this constant. I somehow have to fetch the value from config. As the method is in the model package I am unable to do so. Can someone help me with a workaround for this? I am thinking of injecting the config value as a parameter to the method.

[jasonblais]

@RajatVaryani would the original PR submitted by a community member be helpful for this case? https://github.com/mattermost/mattermost-server/pull/11028

[RajatVaryani]

@jasonblais I think that is not correctly implemented or my assumptions are incorrect. The method to validate the username is used across many places like importing bulk data of users, while creation of users, Creating and updating incoming webhook for channel. etc. So this config check must be implemented across all these places right? To do so we need to modify the existing method or create a new one and call it from all these places. Please correct me If my assumption is incorrect.

[jasonblais]

@mgdelacroix @hmhealey wondering if you had thoughts on the above note from @RajatVaryani?

[hmhealey]

Yeah, we'll likely need to modify or use a new function everywhere that it's used since the current version doesn't have access to the server's config.

I agree that we'll probably need to replace the existing function everywhere that it's called. The easiest way to do that would be to add an app.isValidUsername method like in the previous PR and call it everywhere, but that only works if we're only checking this as part of the app package. Adding a parameter to the existing function would also work since it's like the sanitize that many of the types in the model package have

[RajatVaryani]

@hmhealey Let me do the changes and raise the PR. Thanks.

[RajatVaryani]

Not able to contribute to it. Please release the issue.

[hanzei]

Thanks for the update @RajatVaryani. Making this available to the public again.

[rfoyard]

I'd like to take up this issue.

[jasonblais]

Great, thanks @rfoyard! :tada:

[rfoyard]

Yeah, we'll likely need to modify or use a new function everywhere that it's used since the current version doesn't have access to the server's config.

I agree that we'll probably need to replace the existing function everywhere that it's called. The easiest way to do that would be to add an app.isValidUsername method like in the previous PR and call it everywhere, but that only works if we're only checking this as part of the app package. Adding a parameter to the existing function would also work since it's like the sanitize that many of the types in the model package have

The IsValidUsername function is called in multiple places where an App object is available so we could add a configuration parameter to the function.

But It's also called by CleanUsername, used in GitLabProvider.GetUserFromJson. In order to pass the new parameter we would need to change the OauthProvider interface. Wouldn't this introduce a breaking change (is it used by external plugins, ...) ?

Another issue is that IsValidUsername is called by user's IsValid method, used in SqlUserStore.SaveandSqlUserStore.Update`. Adding a configuration parameter to the store methods would not be ideal in my opinion. Maybe we could pass a validation function instead ?

What are your thoughts on this ?

[jasonblais]

@hmhealey You might be the best person to help with the above questions?

[jasonblais]

@rfoyard Did you get an answer to help you proceed?

[rfoyard]

Hi @jasonblais, I had no information to help me proceed.
I am working on another issue that takes me time, feel free to make this one available.

[jasonblais]

@rfoyard Thanks for letting me know. Let me touch base with the team and someone will get back to you.

[Willyfrog]

regarding oauth provider api, it would be preferable not to change the API. I don't think it is used by plugins, but since I don't really know the plugin ecosystem so well, I can only guess. Anyways, if this is needed, then the API would need to change, and that only means that we should be extra-careful with it and make sure it is well communicated, it wouldn't mean that it would stop it.

Regarding config parameter vs function, I agree that is desirable to have a validation function passed to the store, so we can change it in the future without too much hassle.

[hmhealey]

The OauthProvider is actually for use by the enterprise code (the e in the einterfaces package name stands for "enterprise"), so it would require someone on the core team to make some changes in that repo, but we can change that API whenever we want.

For the validation in the store, I think I'd need to see how passing a validation function would look since I'm not aware of anywhere else that we've done that. It sounds like a good idea in theory though.

[mrjones-plip]

Let's reduce technical debt! I'm looking to level up from my Difficulty/1:Easy tickets I've done to date: I'll take this one ;)

[jasonblais]

Yay, thanks @Ths2-9Y-LqJt6! Let us know if you have any questions as you work through this ticket :)

[mrjones-plip]

Still planning on working on this, but my other ticket which was almost done required me to learn how to write e2e cypress tests (fun \o/ !). I'll head over here when I'm done w/ that!

[jasonblais]

Thanks for the update @Ths2-9Y-LqJt6! Sounds good :)

[mrjones-plip]

Hey again - I'm taking a 1 week break on this ticket ending Mar 25th. I'll be back next week to check in!

[mrjones-plip]

Hey all - I'm no longer working on this so unassigning.