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

Block API endpoints access to users when their password has been invalidated by a superuser #4573

Merged
merged 29 commits into from
Sep 7, 2023
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
cc5ed4c
Validate password before any other permission checks
noliveleger Aug 7, 2023
a3a64b5
Update permission classes to check password first
noliveleger Aug 7, 2023
94d2f23
Add unit tests for API endpoints access
noliveleger Aug 8, 2023
78791db
Restore useful import
noliveleger Aug 8, 2023
379dcd5
Merge branch 'feature/password-complexity' into 4473-block-api-with-i…
noliveleger Aug 8, 2023
5a6fa3a
Merge branch 'feature/password-complexity' into 4473-block-api-with-i…
noliveleger Aug 9, 2023
b932a18
Merge branch 'feature/password-complexity' into 4473-block-api-with-i…
noliveleger Aug 10, 2023
0a4fa4a
Merge branch 'feature/password-complexity' into 4473-block-api-with-i…
noliveleger Aug 10, 2023
22c8fe9
Allow access to app messages when password is unsafe
noliveleger Aug 16, 2023
511ca44
Merge branch 'feature/password-complexity' into 4473-block-api-with-i…
noliveleger Aug 16, 2023
b3c2f6c
Merge branch 'feature/password-complexity' into 4473-block-api-with-i…
noliveleger Aug 16, 2023
48625bd
Merge branch 'feature/password-complexity' into 4473-block-api-with-i…
noliveleger Aug 18, 2023
6012e2a
Merge branch 'feature/password-complexity' into 4473-block-api-with-i…
noliveleger Aug 21, 2023
0f089c9
Merge branch 'feature/password-complexity' into 4473-block-api-with-i…
noliveleger Aug 21, 2023
3115b81
Fix typo in help text
noliveleger Aug 21, 2023
bac0309
Remove unused import
jnm Aug 30, 2023
92c7007
Revise comments
jnm Aug 30, 2023
babd0ee
Merge remote-tracking branch 'origin/feature/password-complexity' int…
jnm Aug 30, 2023
db36eb0
Relax number of queries assertion
jnm Aug 30, 2023
f3b4598
Change some password validation messages
jnm Aug 30, 2023
960e2a6
Make strict password requirements opt-in
jnm Aug 30, 2023
8f2635a
Update validation text in unit tests
jnm Aug 30, 2023
e359a2d
Merge branch 'feature/password-complexity' into 4473-block-api-with-i…
noliveleger Aug 30, 2023
3a8564f
Make fields for extra details optional in Admin interface
noliveleger Aug 30, 2023
9a01cef
Fix current user unit tests with new wordings
noliveleger Aug 30, 2023
84fa06e
Block current user endpoint access to anonymous user
noliveleger Aug 30, 2023
db205f9
Use FuzzyInt with assertNumQueries
noliveleger Aug 30, 2023
9668267
Merge pull request #4606 from kobotoolbox/fuzzy-int-assert-num-queries
JacquelineMorrissette Sep 1, 2023
b2c8b43
Merge branch 'feature/password-complexity' into 4473-block-api-with-i…
noliveleger Sep 5, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion kobo/apps/accounts/mfa/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@
from django.db.models import QuerySet
from django.urls import reverse
from rest_framework.generics import ListAPIView
from rest_framework.permissions import IsAuthenticated
from trench.utils import get_mfa_model

from kpi.permissions import IsAuthenticated
from .forms import MfaLoginForm, MfaTokenForm
from .serializers import UserMfaMethodSerializer

Expand Down
1 change: 0 additions & 1 deletion kobo/apps/accounts/tests/test_email.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
from allauth.account.models import EmailAddress
from django.core import mail
from django.urls import reverse
from model_bakery import baker
Expand Down
2 changes: 1 addition & 1 deletion kobo/apps/accounts/tests/test_social_account.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def test_list(self):
account1 = baker.make('socialaccount.SocialAccount', user=self.user)
account2 = baker.make('socialaccount.SocialAccount')
# Auth, Count, Queryset
with self.assertNumQueries(3):
with self.assertNumQueries(4):
noliveleger marked this conversation as resolved.
Show resolved Hide resolved
res = self.client.get(self.url_list)
self.assertContains(res, account1.uid)
self.assertNotContains(res, account2.uid)
Expand Down
2 changes: 1 addition & 1 deletion kobo/apps/accounts/views.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
from allauth.account.models import EmailAddress
from allauth.socialaccount.models import SocialAccount
from rest_framework import mixins, status, viewsets
from rest_framework.permissions import IsAuthenticated
from rest_framework.response import Response

from kpi.permissions import IsAuthenticated
from .mixins import MultipleFieldLookupMixin
from .serializers import EmailAddressSerializer, SocialAccountSerializer

Expand Down
5 changes: 4 additions & 1 deletion kobo/apps/audit_log/permissions.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
from rest_framework.permissions import IsAdminUser

from kpi.mixins.validation_password_permission import ValidationPasswordPermissionMixin

class SuperUserPermission(IsAdminUser):

class SuperUserPermission(ValidationPasswordPermissionMixin, IsAdminUser):

def has_permission(self, request, view):
self.validate_password(request)
return bool(request.user and request.user.is_superuser)
3 changes: 3 additions & 0 deletions kobo/apps/help/permissions.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
# coding: utf-8
from rest_framework import exceptions, permissions

from kpi.mixins.validation_password_permission import ValidationPasswordPermissionMixin
jnm marked this conversation as resolved.
Show resolved Hide resolved


class InAppMessagePermissions(permissions.BasePermission):
def has_permission(self, request, view):

if not request.user.is_authenticated:
# Deny access to anonymous users
return False
Expand Down
2 changes: 1 addition & 1 deletion kobo/apps/languages/views/base.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
# coding: utf-8
from rest_framework import viewsets
from rest_framework.permissions import IsAuthenticated

from kpi.filters import SearchFilter
from kpi.permissions import IsAuthenticated


class BaseViewSet(viewsets.ReadOnlyModelViewSet):
Expand Down
10 changes: 9 additions & 1 deletion kobo/apps/organizations/permissions.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,20 @@
from rest_framework import permissions

from kpi.mixins.validation_password_permission import ValidationPasswordPermissionMixin

class IsOrgAdminOrReadOnly(permissions.BasePermission):

class IsOrgAdminOrReadOnly(
ValidationPasswordPermissionMixin, permissions.BasePermission
):
"""
Object-level permission to only allow admin members of an object to edit it.
Assumes the model instance has an `is_admin` attribute.
"""

def has_permission(self, request, view):
self.validate_password(request)
return super().has_permission(request=request, view=view)

def has_object_permission(self, request, view, obj):
# Read permissions are allowed to any request,
# so we'll always allow GET, HEAD or OPTIONS requests.
Expand Down
4 changes: 2 additions & 2 deletions kobo/apps/organizations/tests/test_organizations_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ def test_list(self):
self._insert_data()
organization2 = baker.make(Organization, id='org_abcd123')
organization2.add_user(user=self.user, is_admin=True)
with self.assertNumQueries(2):
with self.assertNumQueries(3):
noliveleger marked this conversation as resolved.
Show resolved Hide resolved
res = self.client.get(self.url_list)
self.assertContains(res, organization2.name)

Expand All @@ -61,7 +61,7 @@ def test_api_returns_org_data(self):
def test_update(self):
self._insert_data()
data = {'name': 'edit'}
with self.assertNumQueries(4):
with self.assertNumQueries(5):
noliveleger marked this conversation as resolved.
Show resolved Hide resolved
res = self.client.patch(self.url_detail, data)
self.assertContains(res, data['name'])

Expand Down
3 changes: 1 addition & 2 deletions kobo/apps/organizations/views.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
from django.contrib.auth.models import User
from django.db.models import QuerySet
from rest_framework import viewsets
from rest_framework.permissions import IsAuthenticated

from kpi.permissions import IsAuthenticated
from .models import Organization, create_organization
from .permissions import IsOrgAdminOrReadOnly
from .serializers import OrganizationSerializer
Expand Down
2 changes: 1 addition & 1 deletion kobo/apps/project_views/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
from django.db.models.query import QuerySet
from django.http import Http404
from rest_framework import viewsets
from rest_framework.permissions import IsAuthenticated
from rest_framework.decorators import action
from rest_framework.response import Response

Expand All @@ -17,6 +16,7 @@
)
from kpi.mixins.object_permission import ObjectPermissionViewSetMixin
from kpi.models import Asset, ProjectViewExportTask
from kpi.permissions import IsAuthenticated
from kpi.serializers.v2.asset import AssetMetadataListSerializer
from kpi.serializers.v2.user import UserListSerializer
from kpi.utils.object_permission import get_database_user
Expand Down
2 changes: 1 addition & 1 deletion kobo/apps/stripe/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
from djstripe.settings import djstripe_settings
from organizations.utils import create_organization
from rest_framework import mixins, status, viewsets
from rest_framework.permissions import IsAuthenticated
from rest_framework.response import Response
from rest_framework.views import APIView

Expand All @@ -27,6 +26,7 @@
ProductSerializer,
SubscriptionSerializer,
)
from kpi.permissions import IsAuthenticated


# Lists the one-time purchases made by the organization that the logged-in user owns
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -302,10 +302,10 @@ def test_google_transcript_post(self, m1, m2):
'submission': submission_id,
'q1': {GOOGLETS: {'status': 'requested', 'languageCode': ''}}
}
with self.assertNumQueries(52):
with self.assertNumQueries(53):
noliveleger marked this conversation as resolved.
Show resolved Hide resolved
res = self.client.post(url, data, format='json')
self.assertContains(res, 'complete')
with self.assertNumQueries(21):
with self.assertNumQueries(22):
noliveleger marked this conversation as resolved.
Show resolved Hide resolved
self.client.post(url, data, format='json')

@override_settings(CACHES={'default': {'BACKEND': 'django.core.cache.backends.locmem.LocMemCache'}})
Expand Down
21 changes: 16 additions & 5 deletions kpi/exceptions.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# coding: utf-8
from django.conf import settings
from django.utils.translation import gettext_lazy as t
from rest_framework import exceptions
from rest_framework import exceptions, status


class AbstractMethodError(NotImplementedError):
Expand Down Expand Up @@ -67,8 +68,18 @@ class ImportAssetException(Exception):
pass


class InvalidPasswordAPIException(exceptions.APIException):
status_code = status.HTTP_403_FORBIDDEN
default_detail = t(
'Your access is restricted. Please reclaim your access by '
'changing your password at '
'##koboform_url##/accounts/password/reset/.'
).replace('##koboform_url##', settings.KOBOFORM_URL)
default_code = 'invalid_password'


class InvalidSearchException(exceptions.APIException):
status_code = 400
status_code = status.HTTP_400_BAD_REQUEST
default_detail = t('Invalid search. Please try again')
default_code = 'invalid_search'

Expand Down Expand Up @@ -96,7 +107,7 @@ class KobocatBulkUpdateSubmissionsClientException(exceptions.ValidationError):


class KobocatBulkUpdateSubmissionsException(exceptions.APIException):
status_code = 500
status_code = status.HTTP_500_INTERNAL_SERVER_ERROR
default_detail = t(
'An error occurred trying to bulk update the submissions.')
default_code = 'bulk_update_submissions_error'
Expand All @@ -121,7 +132,7 @@ def invalid_form_id(self):


class KobocatDuplicateSubmissionException(exceptions.APIException):
status_code = 500
status_code = status.HTTP_500_INTERNAL_SERVER_ERROR
default_detail = t('An error occurred trying to duplicate the submission')
default_code = 'submission_duplication_error'

Expand All @@ -135,7 +146,7 @@ class NotSupportedFormatException(Exception):


class ObjectDeploymentDoesNotExist(exceptions.APIException):
status_code = 400
status_code = status.HTTP_400_BAD_REQUEST
default_detail = t('The specified object has not been deployed')
default_code = 'deployment_does_not_exist'

Expand Down
21 changes: 21 additions & 0 deletions kpi/mixins/validation_password_permission.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
from ..exceptions import InvalidPasswordAPIException


class ValidationPasswordPermissionMixin:

def validate_password(self, request):

if request.user.is_anonymous:
return

try:
extra_details = request.user.extra_details
except request.user.extra_details.RelatedObjectDoesNotExist:
# if user has not extra details, admin has not been able to set
# `validated_password`. Let's consider it as True.
return

if extra_details.validated_password:
return

raise InvalidPasswordAPIException
22 changes: 20 additions & 2 deletions kpi/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from django.contrib.auth.models import User
from django.http import Http404
from rest_framework import exceptions, permissions
from rest_framework.permissions import IsAuthenticated as DRFIsAuthenticated

from kpi.constants import (
PERM_ADD_SUBMISSIONS,
Expand All @@ -14,6 +15,7 @@
PERM_VIEW_ASSET,
PERM_VIEW_SUBMISSIONS,
)
from kpi.mixins.validation_password_permission import ValidationPasswordPermissionMixin
from kpi.models.asset import Asset
from kpi.utils.object_permission import get_database_user
from kpi.utils.project_views import (
Expand Down Expand Up @@ -119,7 +121,9 @@ def has_object_permission(self, request, view, obj):
return True


class AssetPermission(permissions.DjangoObjectPermissions):
class AssetPermission(
ValidationPasswordPermissionMixin, permissions.DjangoObjectPermissions
):

# Setting this to False allows real permission checking on AnonymousUser.
# With the default of True, anonymous requests are categorically rejected.
Expand All @@ -130,6 +134,10 @@ class AssetPermission(permissions.DjangoObjectPermissions):
perms_map['OPTIONS'] = perms_map['GET']
perms_map['HEAD'] = perms_map['GET']

def has_permission(self, request, view):
self.validate_password(request)
return super().has_permission(request=request, view=view)

def has_object_permission(self, request, view, obj):

user = get_database_user(request.user)
Expand All @@ -151,7 +159,9 @@ def has_object_permission(self, request, view, obj):
return super().has_object_permission(request, view, obj)


class AssetNestedObjectPermission(BaseAssetNestedObjectPermission):
class AssetNestedObjectPermission(
ValidationPasswordPermissionMixin, BaseAssetNestedObjectPermission
):
"""
Permissions for nested objects of Asset.
Only owner and managers can have write access on these objects.
Expand All @@ -172,6 +182,7 @@ class AssetNestedObjectPermission(BaseAssetNestedObjectPermission):
perms_map['DELETE'] = perms_map['POST']

def has_permission(self, request, view):
self.validate_password(request)
if not request.user:
return False
elif request.user.is_superuser:
Expand Down Expand Up @@ -274,6 +285,13 @@ class AssetVersionReadOnlyPermission(AssetNestedObjectPermission):
}


class IsAuthenticated(ValidationPasswordPermissionMixin, DRFIsAuthenticated):

def has_permission(self, request, view):
self.validate_password(request)
return super().has_permission(request=request, view=view)


# FIXME: Name is no longer accurate.
class IsOwnerOrReadOnly(AssetPermission):
"""
Expand Down
2 changes: 1 addition & 1 deletion kpi/serializers/current_user.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ def get_validated_password(self, obj):
extra_details = obj.extra_details
except obj.extra_details.RelatedObjectDoesNotExist:
# if user has not extra details, admin has not been able to set
# `validated_password`. Let's consider it True.
# `validated_password`. Let's consider it as True.
jnm marked this conversation as resolved.
Show resolved Hide resolved
return True

return extra_details.validated_password
Expand Down
1 change: 1 addition & 0 deletions kpi/tests/api/v2/test_api_imports.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from kpi.urls.router_api_v2 import URL_NAMESPACE as ROUTER_URL_NAMESPACE
from kpi.utils.strings import to_str


class AssetImportTaskTest(BaseTestCase):
fixtures = ['test_data']

Expand Down