fix: generate password with configured validators

[igobranco]

Description:

In short this PR fixes the generate_password function.

This PR fixes the registration use case when the platform has a stronger password requirements and the user registers its account using a third party method.
The error that is shown to the user is "This password must contain at least ", where N is the minimum number of characters that the password need to have for that T type and the T is uppercase, lowercase, punctuation, numbers or symbol.

If an installation has a stronger security, like requiring the users to have at least one upper case letter, one lower case letter, one number and one punctuation characters the generated password would be invalid. The edx-platform setting that is used to configure the stronger password requirement is the AUTH_PASSWORD_VALIDATORS.

This can happen when an user register its account on the platform using a third party SAML or OAuth service, like Google, Facebook, etc., blocking him from creating the account using that 3rd party method.

Solution:
This PR changes the generate_password function, if the AUTH_PASSWORD_VALIDATORS setting is configured, then the generate_password function will honor the configured password policy validators - generating a random password based on the configured policy.

Dependencies:

No dependencies. It's also back work compatible.

Merge deadline:

Before next Open edX release.

Installation instructions:

The see the error you need to have a not default AUTH_PASSWORD_VALIDATORS setting value.
For example:

AUTH_PASSWORD_VALIDATORS:
  - NAME: django.contrib.auth.password_validation.UserAttributeSimilarityValidator
  - NAME: common.djangoapps.util.password_policy_validators.MinimumLengthValidator
    OPTIONS:
      min_length: 8
  - NAME: common.djangoapps.util.password_policy_validators.MaximumLengthValidator
    OPTIONS:
      max_length: 75
  - NAME: common.djangoapps.util.password_policy_validators.UppercaseValidator
    OPTIONS:
      min_upper: 1
  - NAME: common.djangoapps.util.password_policy_validators.LowercaseValidator
    OPTIONS:
      min_lower: 1
  - NAME: common.djangoapps.util.password_policy_validators.NumericValidator
    OPTIONS:
      min_numeric: 1
  - NAME: common.djangoapps.util.password_policy_validators.PunctuationValidator
    OPTIONS:
      min_punctuation: 1

Administrative action, configure a third party authentication method, for example Google, Facebook, Linkedin, etc.

Testing instructions:

Register a new account the configured third party service.

Reviewers:

• @felipemontoya

Merge checklist:

• All reviewers approved
• CI build is green
• Version bumped
• Changelog record added
• Documentation updated (not only docstrings)
• Commits are squashed

Post merge:

• Create a tag
• Check new version is pushed to PyPi after tag-triggered build is
  finished.
• Delete working branch (if not needed anymore)

Author concerns:

Any.

We have this fix on FCCN edx-platform from at least the Juniper release:

• fccn/edx-platform@e835f61

[openedx-webhooks]

Thanks for the pull request, @igobranco! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[robrap]

@NIXKnight: Would you be able to review this?

[robrap]

Just a passing comment, but it seems odd that this is a fixed list of characters. Where did this come from? Can it be commented?

[igobranco]

That were a static list of symbols that were used if the password policy validators include at least 1 symbol (common.djangoapps.util.password_policy_validators.SymbolValidator) with a min_symbol option.

Viewing your comment, I decide to replace that static list with a function that iterates all chars and use all symbols. The impact is a small slower performance.

def _symbols():
    """Get all valid symbols"""
    symbols = []
    for c in map(chr, range(sys.maxunicode + 1)):
        if 'S' in unicodedata.category(c):
            symbols.append(c)
    return symbols

[robrap]

@igobranco: Another high-level comment is that I think the PR description describes the problem, but unless I am just missing something, it doesn't seem to describe in text the proposed solution (i.e. fix). Also, in addition to updating the PR description, it would be great to ultimately get this information into the commit comment as well. Thank you.

[igobranco]

@igobranco: Another high-level comment is that I think the PR description describes the problem, but unless I am just missing something, it doesn't seem to describe in text the proposed solution (i.e. fix). Also, in addition to updating the PR description, it would be great to ultimately get this information into the commit comment as well. Thank you.

Added a Solution comment on the PR description and it was also included a similar text in the commit message.

[robrap]

Thanks @igobranco. Some follow-up questions that are less about the PR specifically, and more about this work in general.

1. Is this password used anywhere, or is it just so we have a valid password to store with the account?
2. If not used at all, why is a password needed? Is it because a user without a password is how we designate an inactive user? I'm just curious if this is related to this issue: [DEPR]: User email activation check in user.is_active public-engineering#165

[igobranco]

Thanks @robrap with your comments.

1. Is this password used anywhere, or is it just so we have a valid password to store with the account?

The generate_password function is used to assign a random password for an user account, so we could store a valid password with the account.

You can found the references to this function using the this search link:
https://github.com/search?q=repo%3Aopenedx%2Fedx-platform%20generate_password&type=code

From my understanding the some use cases are:

• reference on the support djangoapp

Allows support staff to disable a user's account

• user retirement
• third party authentication

I've come across issues on the last one, the 3rd party authentication, when the user is using a SAML or OAuth service.
And I think this is related with the different level of password security that we have on our platform - nau.edu.pt, than comparing with the edx.org.
We require at least:

• 1 upper case letter
• 1 lower case letter
• 1 numeric
• 1 punctuation character
• with a minimum length of 8 characters

On the edx.org I've captured this screenshot:

that requires only at least 1 letter and 1 number and a minimum length of 8.

With the current implementation it could happen to generate a password with only numbers or letters that would break the validation on edx.org, but it is low probability. On our case all generated password would break the configured password validation.

2. If not used at all, why is a password needed? Is it because a user without a password is how we designate an inactive user? I'm just curious if this is related to this issue: [DEPR]: User email activation check in user.is_active public-engineering#165

Yes, it is needed, because when an user registers using a third party service the user doesn't input a password, the openedx edx platform assigns a random password.
This isn't related user.is_active described on openedx/public-engineering#165.

[robrap]

@igobranco: Thanks for all of that detailed information. I appreciate it.

when an user registers using a third party service the user doesn't input a password, the openedx edx platform assigns a random password.

We don't have to get into it here, but if the password is never meant to be used, one could consider whether it wouldn't be simpler to just leave the account password-less instead of generating a password. That said, I think we use password-less accounts to disable users, and we don't have an alternative mechanism at this time (is_active or otherwise), so we can't just do this simpler fix.

There is no need to respond to this first part as far as this PR is concerned.

With the current implementation it could happen to generate a password with only numbers or letters that would break the validation on edx.org, but it is low probability.

I'm curious both how this could happen, and how this could be avoided. Where does this happen exactly? At a minimum, I'd want to add observability around when this is happening and what was the effect. However, would it be simple to just update the code to retry if an invalid password were generated?

Again - I have not dug into this - and I'm just asking questions based on your responses.

Thank you.

[robrap]

@NIXKnight: Would you be able to review this?

[inform] In response to this request, I was reminded that this code was originally in edx-platform, and the initial commit in this repo was just pulling this code across repos.

[mphilbrick211]

Hi @igobranco and @robrap - just checking in on this :)

[robrap]

Thanks @mphilbrick211.

@igobranco: This still needs to be addressed. I'm splitting it out from the earlier comment.

With the current implementation it could happen to generate a password with only numbers or letters that would break the validation on edx.org, but it is low probability.

I'm curious both how this could happen, and how this could be avoided. Where does this happen exactly? At a minimum, I'd want to add observability around when this is happening and what was the effect. However, would it be simple to just update the code to retry if an invalid password were generated?

Again - I have not dug into this - and I'm just asking questions based on your responses.

[robrap]

@igobranco: How would you like to proceed? See earlier open comments. Thanks.

[igobranco]

Thanks @mphilbrick211.

@igobranco: This still needs to be addressed. I'm splitting it out from the earlier comment.

With the current implementation it could happen to generate a password with only numbers or letters that would break the validation on edx.org, but it is low probability.

I'm curious both how this could happen, and how this could be avoided. Where does this happen exactly? At a minimum, I'd want to add observability around when this is happening and what was the effect. However, would it be simple to just update the code to retry if an invalid password were generated?
Again - I have not dug into this - and I'm just asking questions based on your responses.

@robrap: I don't know, but I had the feeling that it could happen. Probably is just a feeling and not something that I tested.
On our installation we have greater security password requirements, we don't see sporadic problem, we see that all generated passwords don't meet the password requirements. Probably, I just generalized...

[robrap]

@igobranco: How willing would you be to introduce a temporary setting toggle that switched between generate_password_original() and generate_password_fixed()? I'm struggling with the fact that I think this PR is ok, but if it causes any unexpected issues, then rolling back a deployment or leaving errors in Production until I'm able to get a revert PR through the pipeline adds more risk. If I have a quick way to turn on/off independently from it going to Production, that greatly reduces risk for me. Thoughts?

[mphilbrick211]

@igobranco: How willing would you be to introduce a temporary setting toggle that switched between generate_password_original() and generate_password_fixed()? I'm struggling with the fact that I think this PR is ok, but if it causes any unexpected issues, then rolling back a deployment or leaving errors in Production until I'm able to get a revert PR through the pipeline adds more risk. If I have a quick way to turn on/off independently from it going to Production, that greatly reduces risk for me. Thoughts?

Hi @igobranco - friendly ping on this!

[mphilbrick211]

I'm going to close this pull request for now - we can reopen in the future if you wish to pursue. Thanks!

[openedx-webhooks]

@igobranco Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

[openedx-webhooks]

@igobranco Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.