Add password validation for email auth - #265
Conversation
|
сделай git rebase, надо подтянуть изменения с коммита Артема, который я замерджил вчера p.s. можно и через кнопку update branch (если она есть у тебя, находится внизу пр рядом с галочками all checks passed) сделать это, но тогда проверь что конфликтов кода не должно быть с последним мерджем |
petrCher
left a comment
There was a problem hiding this comment.
отревьюил пока что без тестов, в целом можешь уже начать менять, ревью тестов сейчас тоже уже начну
| """Validate a newly created password according to the Auth API password policy.""" | ||
| if len(value) < PASSWORD_MIN_LENGTH: | ||
| raise ValueError(f"Password must be at least {PASSWORD_MIN_LENGTH} characters long") | ||
| if len(value) > PASSWORD_MAX_LENGTH: |
There was a problem hiding this comment.
здесь лучше elif для более быстрой работы, если len(value)<min то больше max точно не будет
| from pydantic.json_schema import JsonSchemaValue | ||
| from pydantic_core import core_schema | ||
|
|
||
| PASSWORD_MIN_LENGTH = 8 |
There was a problem hiding this comment.
надо эту переменную перенести в settings и здесь ее просто оттуда импортировать
в settings надо, так как все переменные окружения лежат там, чтобы можно было их не прописывать вручную в коде, а дать возможность не меняя код в секретах репозитория в environments values задать то, что хочется. Конечно, можно и не задавать свои значения будет, тогда возьмутся значения дефолта, то есть =8 как ты и пропишешь
также, когда перенесешь это в settings надо еще в github/workflows в test и prod прописать аналогичные команды как здесь https://github.com/profcomff/auth-api/blob/main/.github/workflows/build_and_publish.yml#L147
| from pydantic_core import core_schema | ||
|
|
||
| PASSWORD_MIN_LENGTH = 8 | ||
| PASSWORD_MAX_LENGTH = 32 |
There was a problem hiding this comment.
ниже переменные (с 10 строчки) оставить здесь, их нет смысла переносить в settings
| ``` | ||
|
|
||
| Требования к новому паролю: | ||
| - длина от 8 до 32 символов; |
There was a problem hiding this comment.
от min до max
надо вместо цифр прописать переменные
| ``` | ||
|
|
||
| Требования к новому паролю: | ||
| - длина от 8 до 32 символов; |
There was a problem hiding this comment.
от min до max
надо вместо цифр прописать переменные
| PASSWORD_MIN_LENGTH = 8 | ||
| PASSWORD_MAX_LENGTH = 32 | ||
| PASSWORD_ALLOWED_CHARACTERS = string.ascii_letters + string.digits + string.punctuation | ||
| PASSWORD_PATTERN = r"^[\x21-\x7E]+$" |
There was a problem hiding this comment.
а зачем эта константа если есть PASSWORD_ALLOWED_CHARACTERS
There was a problem hiding this comment.
ты ее в валидации используешь, ее же и используй в классе Password, не надо плодить константы одинаковые по сути
|
|
||
|
|
||
| def create_user(email: str, password: str, session: Session) -> None: | ||
| password = validate_password(password) |
There was a problem hiding this comment.
здесь можно без присваивания, ты же в validate_password никак не меняешь пароль, а всего лишь проверяешь
There was a problem hiding this comment.
плюс в коде ниже есть проверка с print и exit
здесь лучше тоже подобное поставить, чтобы отловить проблему если что
|
|
||
| create-user: | ||
| python -m auth_backend user create --email test-user@profcomff.com --password string | ||
| python -m auth_backend user create --email test-user@profcomff.com --password string12 |
There was a problem hiding this comment.
мб коммент добавить сюда и в изменение ниже, что пароль должен удовлетворять min и max, просто при беглом изучении может быть неочевидно почему именно string12 а не просто string к примеру
There was a problem hiding this comment.
вроде коммент здесь через хэштег
|
|
||
| create-admin: | ||
| source ./venv/bin/activate && python -m auth_backend user create --email test-admin@profcomff.com --password string | ||
| source ./venv/bin/activate && python -m auth_backend user create --email test-admin@profcomff.com --password string12 |
petrCher
left a comment
There was a problem hiding this comment.
все отревьюил, можно исправлять)
| def user_id(client_auth: TestClient, dbsession): | ||
| time = datetime.datetime.utcnow() | ||
| body = {"email": f"user{time}@example.com", "password": "string"} | ||
| body = {"email": f"user{time}@example.com", "password": "string12"} |
There was a problem hiding this comment.
я бы лучше что-то более существенное прописал сюда, например "test-password"
везде ниже тоже
| f"{url}/request", | ||
| headers={"Authorization": auth_token}, | ||
| json={"password": "", "new_password": "changed"}, | ||
| json={"password": "", "new_password": "changed12"}, |
There was a problem hiding this comment.
здесь какой нибудь "changed-pass"
| def test_invalid_password(client_auth: TestClient): | ||
| invalid_passwords = [ | ||
| "short7", | ||
| "a" * 129, |
There was a problem hiding this comment.
лучше делать PASSWORD_MAX_LENGTH + 1
|
|
||
| def test_invalid_password(client_auth: TestClient): | ||
| invalid_passwords = [ | ||
| "short7", |
There was a problem hiding this comment.
здесь лучше тоже как ниже через min делать строку, чтобы не быть привязанным к чему-то конкретному
|
|
||
| def test_invalid_password(client_auth: TestClient): | ||
| invalid_passwords = [ | ||
| "short7", |
There was a problem hiding this comment.
здесь лучше тоже как ниже через min делать строку, чтобы не быть привязанным к чему-то конкретному
|
|
||
|
|
||
| def test_password_accepts_all_ascii_punctuation(): | ||
| value = string.punctuation |
| PasswordModel(password=value) | ||
|
|
||
|
|
||
| def test_password_json_schema_contains_frontend_requirements(): |
There was a problem hiding this comment.
а зачем этот тест не очень понял? разве здесь не будет всегда assert отправлять положительный ответ? ну просто я так понимаю ты кладешь все эти переменные еще в password.py и сейчас сравниваешь сами с собой, в чем я не прав?

Введена новая политика пароля:
Разрешены:
Запрещены пробелы, \n, \t, кириллица, буквы с диакритикой и вообще символы вне ASCII.
При этом я специально не применял новые ограничения к паролю при логине и к полю старого пароля при смене. Это важно для обратной совместимости: пользователь, зарегистрированный раньше с паролем вроде пароль123, по-прежнему сможет войти. Но установить такой пароль заново уже нельзя.
Основные изменения хранятся в auth_backend/schemas/types/password.py - новый файл, где централизовано хранятся условия на пароль: PASSWORD_MIN_LENGTH = 8, PASSWORD_MAX_LENGTH = 32, набор разрешённых символов, validate_password() и Pydantic-тип Password. В OpenAPI добавлены minLength, maxLength, pattern, format=password и текстовое описание требований.
Создание пользователя через CLI теперь тоже не позволяет обойти политику пароля.
Добавлены новые тесты: tests/test_unit/test_password.py - проверяются длина, Unicode, пробел, \n, \t, все ASCII-спецсимволы и данные, которые попадут в OpenAPI; tests/test_routes/test_registration.py и test_change_password.py - добавлены проверки, что короткие и недопустимые пароли дают 422.
Также изменен README и Makefile под новые правила пароля.
Closes #246