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

♻️ Refactored code to use encryption algorithm name from settings for consistency #1160

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

Conversation

sameeramin
Copy link

No description provided.

@sameeramin sameeramin force-pushed the encryption-algorithm branch 2 times, most recently from 324a7d9 to 4dd0555 Compare April 8, 2024 06:42
@sameeramin sameeramin changed the title 🔧 Refactored code to use encryption algorithm name from settings for consistency ♻️ Refactored code to use encryption algorithm name from settings for consistency Apr 8, 2024
Copy link

@menkotoglou menkotoglou left a comment

Choose a reason for hiding this comment

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

I like it!

@sameeramin
Copy link
Author

Thank you @menkotoglou, please let me know what else I need to do in order to get this PR merged.

@menkotoglou
Copy link

@sameeramin unfortunately I cannot do anything about it, since I'm not a repo maintainer.

Copy link
Owner

@tiangolo tiangolo left a comment

Choose a reason for hiding this comment

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

Thank you for the interest!

I don't think the algorithm should be configurable in the settings, it shouldn't really be variable. Let's better keep it in security.py.

But I agree that the backend utils should use the same variable from security.py. 🤓 Could you please update that?

@@ -103,14 +103,16 @@ def generate_password_reset_token(email: str) -> str:
encoded_jwt = jwt.encode(
{"exp": exp, "nbf": now, "sub": email},
settings.SECRET_KEY,
algorithm="HS256",
algorithm=settings.ALGORITHM,
Copy link
Owner

Choose a reason for hiding this comment

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

This could be from security.ALGORITHM

)
return encoded_jwt


def verify_password_reset_token(token: str) -> str | None:
try:
decoded_token = jwt.decode(token, settings.SECRET_KEY, algorithms=["HS256"])
decoded_token = jwt.decode(
token, settings.SECRET_KEY, algorithms=[settings.ALGORITHM]
Copy link
Owner

Choose a reason for hiding this comment

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

This could be from security.ALGORITHM

@sameeramin
Copy link
Author

Thank you for the interest!

I don't think the algorithm should be configurable in the settings, it shouldn't really be variable. Let's better keep it in security.py.

But I agree that the backend utils should use the same variable from security.py. 🤓 Could you please update that?

Sure, I'll update that!

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

Successfully merging this pull request may close these issues.

None yet

3 participants