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

Adds middleware to enforce 2fa #284

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

Conversation

singh1114
Copy link

Description

As far as I know, The current 2FA enforcement involves admin intervention. With the following change, people can directly include middleware in the settings file and things will work out of the box.

Motivation and Context

We are using this software for in our django app. We wanted to enforce 2FA. But the existing solution doesn't allowed the free movement and stopped the user from moving forward if the user has not enabled 2FA.

We wanted it differently, we wanted the users to move forward but only to the urls where the user can either logout or enable 2FA.

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@singh1114
Copy link
Author

@Bouke Please check if the following change is required or not. If yes, Please share if you want any modifications as well.

@codecov
Copy link

codecov bot commented Nov 12, 2018

Codecov Report

Merging #284 into master will decrease coverage by 0.68%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #284      +/-   ##
==========================================
- Coverage   96.52%   95.84%   -0.69%     
==========================================
  Files          39       40       +1     
  Lines        1640     1660      +20     
  Branches      116      118       +2     
==========================================
+ Hits         1583     1591       +8     
- Misses         35       47      +12     
  Partials       22       22
Impacted Files Coverage Δ
two_factor/middleware/enforce2fa.py 0% <0%> (ø)
tests/test_utils.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 e3265a5...a2cbfd7. Read the comment docs.

Copy link
Collaborator

@moggers87 moggers87 left a comment

Choose a reason for hiding this comment

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

As well as my comments, there are also no tests.

"""Wrap around actual Admin calls."""
response = self.get_response(request)
if resolve(request.path).app_name == 'two_factor' or (
resolve(request.path).url_name == 'logout'):
Copy link
Collaborator

Choose a reason for hiding this comment

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

How do we know the logout URL is called logout?

Copy link
Author

Choose a reason for hiding this comment

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

@moggers87 Is there any way of knowing this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not that I know of.

self.get_response = get_response

def __call__(self, request):
"""Wrap around actual Admin calls."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

This code won't limit itself to just the admin site as it makes no such check. Either code needs to do what the comment says or the comment needs changing.

from django.urls import resolve, reverse


class Enforce2FAMiddleware:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This package still supports Python 2.7 for Django 1.11, so that should be a subclass of object.

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.

None yet

2 participants