Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Improve error messages when password doesn't meet policy requirements. #8661

Closed
3 tasks done
TonyLovesDevOps opened this issue Oct 8, 2021 · 23 comments
Closed
3 tasks done

Comments

@TonyLovesDevOps
Copy link
Contributor

TonyLovesDevOps commented Oct 8, 2021

Preflight Checklist

Describe the Bug

Right now, when a user offers a password that doesn't meet the password complexity requirements, the error messages returned to the user could be more helpful.

For example, when accepting an invite, the user sees an Unexpected Error:
Unexpected error

And when resetting a password, they receive the much better but still not super clear Value doesn't have the correct format:
Value doesn't have the correct format

To Reproduce

Scenario 1: Accept invite

  1. Choose an Auth Password Policy other than None on the /admin/settings/project page;
  2. Send an email invite to a new user;
  3. Click the invite link and enter a password that doesn't meet the requirements;

Scenario 2: Reset password

  1. Choose an Auth Password Policy other than None on the /admin/settings/project page;
  2. Log in as an existing user and attempt to reset their password to a new one that doesn't meet the policy requirements.

Expected: User receives an error message stating that their password does not meet the password policy.
Actual: User receives Unexpected Error in scenario 1, and Value doesn't have the correct format in scenario 2.

In both cases, the directus logs contain messages showing Provided password doesn't match password policy -- can we return that to the user in both cases instead of the cryptic messages?

What version of Directus are you using?

v9.0.0-rc.96

What version of Node.js are you using?

v16.10.0 (from directus/directus:9.0.0-rc.96 docker image)

What database are you using?

MariaDB 10.3.23

What browser are you using?

Chrome 94.0.4606.71

What operating system are you using?

macOS

How are you deploying Directus?

Docker

@TonyLovesDevOps
Copy link
Contributor Author

One idea around this would be to add an element to the Auth Password Policy section with an optional Custom error message or Description of policy field that could be shown to the users in this case, in addition to or instead of the Provided password doesn't match password policy message, e.g. Password must be a minimum of 20 characters in length with at least one number, one uppercase letter, and one symbol.

@azrikahar
Copy link
Contributor

This was actually my concern over at #8526 (comment) 🙈

In both cases, the directus logs contain messages showing Provided password doesn't match password policy -- can we return that to the user in both cases instead of the cryptic messages?

We might need to change the exception thrown here:

throw new FailedValidationException({
message: `Provided password doesn't match password policy`,
path: ['password'],
type: 'custom.pattern.base',
context: {
value: password,
},
});

so that we can translate the message on the app 🤔

One idea around this would be to add an element to the Auth Password Policy section with an optional Custom error message or Description of policy field that could be shown to the users in this case,

Interesting idea!

@joselcvarela

This comment has been minimized.

@rijkvanzanten

This comment has been minimized.

@rijkvanzanten
Copy link
Member

@azrikahar Adding a new "PasswordPolicyValidationException" would make the most sense to me. As for the translated message, I agree we can't just show "Password should match /(?=^.{8,}$)(?=.*\d)(?=.*[a-z])(?=.*[A-Z])(?=.*[!@#$%^&*()_+}{';'?>.<,])(?!.*\s).*$/", as nobody would understand what that means.. Just showing "Password isn't strong enough" wouldn't really help much neither.

Adding a custom message would be nice, but would still require you to explain the regex you entered again. Technically speaking, we have all the information we need in the regex itself:

/(?=^.{8,}$)(?=.*\d)(?=.*[a-z])(?=.*[A-Z])(?=.*[!@#$%^&*()_+}{';'?>.<,])(?!.*\s).*$/

Part Description
(?=^.{8,}$) At least 8 characters
(?=.*\d) Needs to have a number
(?=.*[a-z]) Needs to have a lowercase character
(?=.*[A-Z]) Needs to have an uppercase character
(?=.*[!@#$%^&*()_+}{';'?>.<,]) Needs to have one of !@#$%^&*()_+}{';'?>.<,
(?!.*\s) Can't have any spaces

I'm wondering if we could pull a regex101.com, and basically come up with a human readable description of the given regex 🤔

@TonyLovesDevOps
Copy link
Contributor Author

TonyLovesDevOps commented Oct 12, 2021

I'm wondering if we could pull a regex101.com, and basically come up with a human readable description of the given regex 🤔

🧐 regex101.com does have a public API that could even be used to create a regex and then point the user to a URL that would include the Explanation -- though the verbosity of the explanation is probably almost as impenetrable as a regex to a normal user (not to mention creating a dependency on a third-party service with zero availability guarantees). I took a quick look for libraries that turn a regex into something human readable and didn't see anything promising at first glance, and whatever regex101 uses is not open source.

This is where I thought a custom error message might come in handy... if you're the admin setting the password policy, you're going to want to communicate it to your users somehow, unless you're a BOFH in which case Unexpected Error seems a perfect way to punish your (l)users. 😈

@rijkvanzanten
Copy link
Member

🧐 regex101.com does have a public API that could even be used to create a regex and then point the user to a URL that would include the Explanation -- though the verbosity of the explanation is probably almost as impenetrable as a regex to a normal user (not to mention creating a dependency on a third-party service with zero availability guarantees).

Yeah I wouldn't want to actually point to regex101, I meant that it would be nice if we could do a similar trick where we auto-generate a human readable explanation 🙂

@azrikahar
Copy link
Contributor

Adding a new "PasswordPolicyValidationException" would make the most sense to me.

@rijkvanzanten Yea that's what I had in mind as well. Might be the "MVP" approach in this case.

I'm wondering if we could pull a regex101.com, and basically come up with a human readable description of the given regex

That's super interesting! But it is uncharted territory for me 😄 Maybe we can somehow do something with the AST we get after using regexp-tree?

I've also stumbled upon this site: https://regexper.com/. The railroad diagram approach somewhat make things clearer from a technical perspective, but I also understand that this is no where near understandable for non-technical end users.

@joselcvarela
Copy link
Member

I keep questioning myself why do websites require to have special characters, uppercase, lowercase, numbers for passwords.
If the image below tells the truth we could only ask user to put a long password.
For example, I have some accounts on Google using a long string all lowercase.
On this cases, we could suggest users to put a sentence from a lyrics they love 😀

About the regex, maybe we could use named groups somehow: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions/Groups_and_Ranges#using_named_groups

@paescuj
Copy link
Member

paescuj commented Oct 13, 2021

Might be interesting as well: https://github.com/VerbalExpressions/JSVerbalExpressions

@benhaynes
Copy link
Sponsor Member

benhaynes commented Oct 13, 2021

All good stuff! I'd like to add the recommendation from @TonyLovesDevOps — let's add a Password Policy Error Message field (STRING) that allows entering a custom message. The default value here should be:

Doesn't fulfill password policy requirements

The input should be full width, and below the other two fields.

@paescuj
Copy link
Member

paescuj commented Oct 13, 2021

All good stuff! I'd like to add the recommendation from @TonyLovesDevOps — let's add a Password Policy Error Message field (STRING) that allows entering a custom message. The default value here should be:

Doesn't fulfill password policy requirements

The input should be full width, and below the other two fields.

While a message like this is way better than Unexcepted Error, it still isn't very helpful for users as they don't know how to full-fill the requirements.
It would be awesome to have an interface to define policies which are then automatically turned into human readable messages 😄
I also stumbled upon the follwing libraries which might be helpful:

@benhaynes
Copy link
Sponsor Member

benhaynes commented Oct 13, 2021

While a message like this is way better than Unexcepted Error, it still isn't very helpful for users as they don't know how to full-fill the requirements.

Why doesn't this direction solve for that? The placeholder value is only because it's not dynamic... but the admin would change it to a specific that matches the Policy/RegEx. So if they make some crazy policy, they can say:

Must contain 3 multi-case letters, 4 numbers, 6 special characters, and an emoji.

To me, this adds the most freedom and avoids us building/integrating/maintaining a complex regex decoder.

@paescuj
Copy link
Member

paescuj commented Oct 13, 2021

While a message like this is way better than Unexcepted Error, it still isn't very helpful for users as they don't know how to full-fill the requirements.

Why doesn't this direction solve for that? The placeholder value is only because it's not dynamic... but the admin would change it to a specific that matches the Policy/RegEx. So if they make some crazy policy, they can say:

Must contain 3 multi-case letters, 4 numbers, 6 special characters, and an emoji.

To me, this adds the most freedom and avoids us building/integrating/maintaining a complex regex decoder.

True, should have read @TonyLovesDevOps proposal more carefully 🙈👍

@rijkvanzanten
Copy link
Member

One thing to keep in mind with the custom error message is i18n... 😬

@benhaynes
Copy link
Sponsor Member

True, but that will be a bigger task with the "string" approach... as we don't support it for the "Public Note" either.

@aidenfoxx
Copy link
Contributor

aidenfoxx commented Oct 13, 2021

You could also just break down the components of password complexity into configurable options in the UI. Realistically that should serve 90% of users, and you have fixed parameters to return a message by. E.g:

  • Minimum length input
  • Require numeric checkbox
  • Require uppercase checkbox
  • Require symbol checkbox

That would also allow for a nicer error interface à la:

image

@TonyLovesDevOps
Copy link
Contributor Author

@aidenfoxx @rijkvanzanten @paescuj do any of you have code brewing for this, or an idea of when you might spend time on it? Alternatively, I could take a stab at the Password Policy Error Message implementation, which isn't as fancy as some of the other cool ideas posted here, but would definitely work for my purposes.

@paescuj
Copy link
Member

paescuj commented Oct 18, 2021

@TonyLovesDevOps Not from my side... Would love to see a PR from your side 😃❤️‍🔥

@rijkvanzanten
Copy link
Member

Likewise, I don't have anything in works at the moment 👍🏻

@aidenfoxx
Copy link
Contributor

Nothing from me. Go for it! 👍 😄

@TonyLovesDevOps
Copy link
Contributor Author

TonyLovesDevOps commented Oct 19, 2021

Cool, I opened a PR for this. I'd value reviews/input on the PR, especially if someone knows of a boilerplate unit test I can copy as a starting point for this - I didn't see any existing tests that set up an auth_password_policy scenario, and I don't have time right now to bang my head against an unfamiliar unit testing framework. 😭

Here's what it looks like with a custom message when accepting an invite / changing a password:

accept-invite-validation-error

change-password-validation-error

@TonyLovesDevOps
Copy link
Contributor Author

So #8946 appears to be in PR purgatory. Is there anything I can do to help get it merged?

I'd happily support a better alternative implementation but we'll soon need to consider the regrettable decision to fork so that our users have a better experience when accepting an email invite, as almost no one meets our policy requirements on the first try.

@directus directus locked and limited conversation to collaborators Apr 15, 2022
@rijkvanzanten rijkvanzanten converted this issue into discussion #12788 Apr 15, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
7 participants