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.

Sign up for GitHub

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

Jump to bottom

Task/add specific failed comment #1994

Merged
merged 7 commits into from
Nov 6, 2023
Merged

Task/add specific failed comment #1994

merged 7 commits into from
Nov 6, 2023

Conversation

jzbahrai
Copy link
Collaborator

@jzbahrai jzbahrai commented Oct 4, 2023 •
edited
Loading

Summary | Résumé

Update the error statement so if a user has a failed login more than 10 times, we can set up a cloudwatch alert for it.

Log while testing:

"{'result': 'error', 'message': {'password': ['Incorrect password for user_id ddd8b872-3012-46b4-85d7-24d7f8e8775a']}}" [in /workspace/app/errors.py:74]
2023-11-01T20:55:28 api api INFO None "{'result': 'error', 'message': {'password': ['Incorrect password for user_id ddd8b872-3012-46b4-85d7-24d7f8e8775a']}}" [in /workspace/app/errors.py:74]
2023-11-01T20:55:33 api api INFO None "{'result': 'error', 'message': {'password': ['Incorrect password for user_id ddd8b872-3012-46b4-85d7-24d7f8e8775a']}}" [in /workspace/app/errors.py:74]
2023-11-01T20:55:39 api api INFO None "{'result': 'error', 'message': {'password': ['Incorrect password for user_id ddd8b872-3012-46b4-85d7-24d7f8e8775a']}}" [in /workspace/app/errors.py:74]
2023-11-01T20:55:44 api api INFO None "{'result': 'error', 'message': {'password': ['Failed login: Incorrect password for user_id ddd8b872-3012-46b4-85d7-24d7f8e8775a failed_login 10 times

@jzbahrai jzbahrai force-pushed the task/add-specific-failed-comment branch from 2703caf to 7743cfa Compare October 4, 2023 19:01
github-advanced-security[bot]
github-advanced-security bot found potential problems Oct 4, 2023
View reviewed changes
tests/app/user/test_rest.py Fixed Show fixed Hide fixed
@jzbahrai jzbahrai force-pushed the task/add-specific-failed-comment branch from 7743cfa to 7944c05 Compare October 31, 2023 16:36
@jzbahrai jzbahrai marked this pull request as ready for review October 31, 2023 20:19
andrewleith
andrewleith reviewed Nov 1, 2023
View reviewed changes
Copy link
Member

@andrewleith andrewleith left a comment

Choose a reason for hiding this comment

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

Just a question about a possible db fetch that isn't needed.

app/user/rest.py Outdated
@@ -230,7 +230,14 @@ def verify_user_password(user_id):
return jsonify({}), 204
else:
increment_failed_login_count(user_to_verify)
message = "Incorrect password"
# We want to get the user again to check the failed login count
user_to_verify = get_user_by_id(user_id=user_id)
Copy link
Member

Choose a reason for hiding this comment

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

Why bother going back to the DB for the user again?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cause we increment_failed_login_count in 232

Copy link
Member

Choose a reason for hiding this comment

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

Right but I think the object is updated in place so I don't think we need to bother with the extra db call!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

let me look at it again, thanks for pointing it out

andrewleith
andrewleith approved these changes Nov 2, 2023
View reviewed changes
Copy link
Member

@andrewleith andrewleith left a comment

Choose a reason for hiding this comment

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

LGTM!

@jzbahrai jzbahrai mentioned this pull request Nov 2, 2023
Merged
@jzbahrai jzbahrai merged commit 917bf40 into main Nov 6, 2023
4 checks passed
@jzbahrai jzbahrai deleted the task/add-specific-failed-comment branch November 6, 2023 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewleith andrewleith andrewleith approved these changes

@whabanks whabanks Awaiting requested review from whabanks

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jzbahrai @andrewleith