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

refactor: Patch user verification code #160

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

apexDev37
Copy link

@apexDev37 apexDev37 commented Jun 29, 2024

Description

This branch introduces patches and enhancements to the current accounts app. This PR's primary goal was to address the logic to generate a user verification code using the standard Python lib random which is discouraged for potentially sensitive or confidential data. For more details, see PEP506

Fixes: N/A

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Decisions

This section outlines notable considerations for developers.

  • Currently, the util function to generate a user verification_code is coupled to the secrets lib. This is a minor tradeoff to
    ensure the random lib is not used under the hood. A subsequent patch to decouple the util will be added soon.

Testing

CI Django & Postgres Tests Jambo

Test Configuration:

The following asserts newly added tests pass:

~ ### Commands for working with a compose setup.~ 
➜  mastori git:(patch-user-verification-code) ✗ docker compose exec -it application python3 manage.py test accounts.tests.test_utils
Found 2 test(s).
System check identified no issues (0 silenced).
..
----------------------------------------------------------------------
Ran 2 tests in 0.008s

OK

The following asserts all project tests pass:

~ ### Commands for working with a compose setup.~ 
➜  mastori git:(patch-user-verification-code) ✗ docker compose exec -it application python3 manage.py test
Found 7 test(s).
Creating test database for alias 'default'...
System check identified no issues (0 silenced).
.. Initial code is 867662 vs updated code is 911270
./env/lib/python3.10/site-packages/whitenoise/base.py:109: UserWarning: No directory at: /app/staticfiles/
  warnings.warn(f"No directory at: {root}")
....
----------------------------------------------------------------------
Ran 7 tests in 0.697s

OK
Destroying test database for alias 'default'...

Additional

General:

  • PEP (Python Enhancement Proposals) docs.
  • [Decision required] The entire implementation of user verification needs to be revised and implemented based on the following code section:
is_verified = models.BooleanField(
    default=False
)  # to be set up later in views to change if user verified
  • [Maintainers] I noticed various areas for improvement when trying to set up locally, especially with Compose. I can make another PR to merge these patches to improve the developer experience.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

- Add dedicated test module for account utils.
- Refactor and extract all verification code tests from `test_models`.

[Reason]
This designs requires a resultant `utils` module which increases SoC.

[Goals]
- test `secrets` module usage under the hood.
- test code uses the expected sequence and length

[Note]
This test introduces a dependency to the `secrets` module.
This a trade-off to ensure secure and cryptographic code generation.
Replace the usage of `random` to generate confidential data.

[Reason]
- `secrets` is properly seeded to generate cryptographic random data.

[Chore]
- Format module with tool `black`.
Order imports with tool `isort`.
- Update docstring for the `User` model.

[Docs]
See, `secrets` library:
https://docs.python.org/3/library/secrets.html
See, `PEP506`:
https://peps.python.org/pep-0506/
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello Contributor,👋👋 Thank You For Opening This Pull Request 🎉🎉

Welcome to SpaceYaTech

We are the fastest growing Africa Open-Source Community Looking To Change The Way Young Africans Get Started In Technology.

JOIN US | THRIVE | GROW

SpaceYaTech Server
SpaceYaTech LinkedIn
SpaceYaTech Twitter
Website

It's great having you contribute to this project

Welcome to the community 🤓 🍿 **Fun facts** - we eat bugs 🐛🐛🐛🐛 for breakfast 🥣

This Pull request has been queued for `review`

Sit tight the maintainers are on your case.

Soon the maintainers/owner will review it and provide you with feedback suggestions.
If you think it's something urgent, feel free to reach out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant