mmctl: create user command does not provide a helpful error message if the password does not comply with settings

[mattermod]

In case a provided password does not comply with the PasswordSettings, the error message misleads the user. The error message reads "Login failed because of invalid password." as it sounds like the user tried to login to the server.

The issue could be resolved with updating the translation api.user.check*user_password.invalid.app*error to be more explanatory. And the HTTP StatusCode should be “Bad Request” here: https://github.com/mattermost/mattermost-server/blob/b66fd1f89efa698fba5fc5e1c5dad350fa4f44f2/app/authentication.go#L81 for consistency.

https://github.com/mattermost/mmctl/issues/399

---

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.

JIRA: https://mattermost.atlassian.net/browse/MM-39919

[KevinSJ]

Hello, I would like to take this one, please assign to me.

[KevinSJ]

HTTP StatusCode should be “Bad Request” here: https://github.com/mattermost/mattermost-server/blob/b66fd1f89efa698fba5fc5e1c5dad350fa4f44f2/app/authentication.go#L81

Hello, after looking through the code a bit, I believe the description for this issue is not quite right. That part of the code is checking if the password is correct, not whether the password satisfy the set requiremnt -- it is calling https://github.com/mattermost/mattermost-server/blob/b66fd1f89efa698fba5fc5e1c5dad350fa4f44f2/app/users/password.go#L14-L15 which is calling ComparePassword https://github.com/mattermost/mattermost-server/blob/92e647f1b9226d00bf6ac1423b63e822cda88476/app/users/password.go#L32-L33

Therefore, my conclusion is that this part of the code should be left as is.

My plan for resolving this issue is to modify the following lines: https://github.com/mattermost/mattermost-server/blob/92e647f1b9226d00bf6ac1423b63e822cda88476/app/user.go#L239-L240

We will need replace the string with the id in the respective error returned from the password validation function: https://github.com/mattermost/mattermost-server/blob/b66fd1f89efa698fba5fc5e1c5dad350fa4f44f2/app/users/password.go#L51-L57 https://github.com/mattermost/mattermost-server/blob/92e647f1b9226d00bf6ac1423b63e822cda88476/i18n/en.json#L8935-L8941

I'm not sure who to tag specifically, but my understanding on Golang is limited, so would be good if someone can confirm if my understanding and plan before I start?

[amyblais]

HTTP StatusCode should be “Bad Request” here: https://github.com/mattermost/mattermost-server/blob/b66fd1f89efa698fba5fc5e1c5dad350fa4f44f2/app/authentication.go#L81

Hello, after looking through the code a bit, I believe the description for this issue is not quite right. That part of the code is checking if the password is correct, not whether the password satisfy the set requiremnt -- it is calling

https://github.com/mattermost/mattermost-server/blob/b66fd1f89efa698fba5fc5e1c5dad350fa4f44f2/app/users/password.go#L14-L15

which is calling ComparePassword https://github.com/mattermost/mattermost-server/blob/92e647f1b9226d00bf6ac1423b63e822cda88476/app/users/password.go#L32-L33

Therefore, my conclusion is that this part of the code should be left as is.

My plan for resolving this issue is to modify the following lines:

https://github.com/mattermost/mattermost-server/blob/92e647f1b9226d00bf6ac1423b63e822cda88476/app/user.go#L239-L240

We will need replace the string with the id in the respective error returned from the password validation function:

https://github.com/mattermost/mattermost-server/blob/b66fd1f89efa698fba5fc5e1c5dad350fa4f44f2/app/users/password.go#L51-L57

https://github.com/mattermost/mattermost-server/blob/92e647f1b9226d00bf6ac1423b63e822cda88476/i18n/en.json#L8935-L8941

I'm not sure who to tag specifically, but my understanding on Golang is limited, so would be good if someone can confirm if my understanding and plan before I start?

cc @isacikgoz

[isacikgoz]

Thanks for taking care of this @KevinSJ, the issue doesn't tell the logic is incorrect, we just want the error message to be accurate. And you have a good point on changing the error id in: https://github.com/mattermost/mattermost-server/blob/5bd223c836a71c1148a3b5990fde6f22f2fdc1ed/app/user.go#L240

[escofresco]

Mind if I give this one a go?

[hanzei]

Surely, @escofresco ! Thanks :+1:

[hanzei]

@escofresco Are you still working on the ticket?

[escofresco]

Yes! Sorry for the delay

[TealWater]

@hanzei So... to move this forward all we have to do is reproduce KevinSJ's PR on the new branch from the current master branch?

[hanzei]

Yes, that would be a great starting point @TealWater :+1:

[TealWater]

@hanzei may I have this assignment

[hanzei]

Sure thing @TealWater :+1: