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

fix: generate password with configured validators #343

Closed

Conversation

igobranco
Copy link

@igobranco igobranco commented Sep 13, 2023

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:

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:

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 13, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Sep 13, 2023

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
Copy link
Contributor

robrap commented Sep 13, 2023

@NIXKnight: Would you be able to review this?

@@ -4,17 +4,96 @@

import random
import string
from django.conf import settings

non_ascii_characters = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Sep 13, 2023
@robrap
Copy link
Contributor

robrap commented Sep 13, 2023

@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.

Fix the `generate_password` function, that generates a random password,
by honoring the configured password policy validators
`AUTH_PASSWORD_VALIDATORS` setting.
@igobranco igobranco force-pushed the igobranco/fix_generate_password branch from 9f23b1b to 1758b77 Compare September 13, 2023 21:45
@igobranco
Copy link
Author

@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
Copy link
Contributor

robrap commented Sep 13, 2023

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
Copy link
Author

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:
image
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.

  1. 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
Copy link
Contributor

robrap commented Sep 14, 2023

@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.

@mphilbrick211 mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Sep 15, 2023
@robrap
Copy link
Contributor

robrap commented Sep 15, 2023

@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.

@robrap robrap assigned igobranco and unassigned robrap Sep 26, 2023
@mphilbrick211
Copy link

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

@robrap
Copy link
Contributor

robrap commented Oct 20, 2023

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
Copy link
Contributor

robrap commented Oct 31, 2023

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

@robrap robrap added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Oct 31, 2023
@igobranco
Copy link
Author

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
Copy link
Contributor

robrap commented Nov 9, 2023

@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
Copy link

@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
Copy link

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

@openedx-webhooks
Copy link

@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.

1 similar comment
@openedx-webhooks
Copy link

@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.

@mphilbrick211 mphilbrick211 removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Feb 20, 2024
@mphilbrick211 mphilbrick211 added the closed inactivity PR was closed because the author abandoned it label Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed inactivity PR was closed because the author abandoned it open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants