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

Raise PermissionDenied for inactive user accounts #826

Merged
merged 1 commit into from
Oct 1, 2018

Conversation

Jkrzy
Copy link
Contributor

@Jkrzy Jkrzy commented Sep 27, 2018

Description

For #825, deny access by and provide a help message to users with inactive accounts.

Additional information

Prevents inactive users from completing authentication w/ tock backend.

@codecov-io
Copy link

codecov-io commented Sep 27, 2018

Codecov Report

Merging #826 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #826      +/-   ##
=========================================
+ Coverage   90.87%   90.9%   +0.03%     
=========================================
  Files          39      39              
  Lines        1742    1748       +6     
=========================================
+ Hits         1583    1589       +6     
  Misses        159     159
Impacted Files Coverage Δ
tock/remote_user_auth.py 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44629d6...4fd483c. Read the comment docs.

@@ -5,11 +5,13 @@
from django.core.exceptions import ValidationError
from django.conf import settings
from django.contrib.auth.models import User
from django.core.exceptions import PermissionDenied
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's combine this with the ValidationError imported on L5 (and while we're at it, we can better match up our imports with best conventions)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, missed that one thanks!

Deny access by and provide a help message to
users with inactive accounts.

def user_can_authenticate(self, user):
if not user.is_active:
raise PermissionDenied(ACCOUNT_INACTIVE_MSG)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an interesting way of handling this. In my head, I think I saw either redirecting to a simple templateView or maybe leveraging the messages framework, but I think I like piggybacking on the 403.

@tbaxter-18f tbaxter-18f merged commit e4a3f65 into master Oct 1, 2018
@Jkrzy Jkrzy deleted the 825-inactive-users branch October 10, 2018 15:58
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.

3 participants