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

Secure API for listing users and groups #338

Closed
wants to merge 26 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
e496e6a
WIP: secured apis for getting and creating users
ethanstrominger Jun 26, 2024
3765562
Complete tests and implentation for secure api
ethanstrominger Jun 28, 2024
69a53e5
Add comments
ethanstrominger Jun 30, 2024
056f5f4
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 30, 2024
7317ea7
Adjust for pre-commit
ethanstrominger Jun 30, 2024
c484de3
Adjust for pre-commit
ethanstrominger Jun 30, 2024
2db02f8
Run through black
ethanstrominger Jun 30, 2024
2e235d2
Merge branch 'secured-apis' of https://github.com/hackforla/peopledep…
ethanstrominger Jun 30, 2024
16138a0
Format secret_permissions.py
ethanstrominger Jun 30, 2024
aecde3a
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jun 30, 2024
08b7e26
Change self.assertEqual to assert
ethanstrominger Jul 1, 2024
a0c3370
Merge branch 'secured-apis' of https://github.com/hackforla/peopledep…
ethanstrominger Jul 1, 2024
b049409
Refactored to simplify
ethanstrominger Sep 5, 2024
28ff18b
Merge branch 'main' of https://github.com/hackforla/peopledepot into …
ethanstrominger Sep 11, 2024
4bb5e3c
Changed to use djangorestframework-api-key
ethanstrominger Sep 12, 2024
be8ecc8
Force port to be 5433
ethanstrominger Sep 12, 2024
8b30649
Fix spelling
ethanstrominger Sep 12, 2024
837b0f9
Spelling
ethanstrominger Sep 12, 2024
6e28f9b
Add to settings.py
ethanstrominger Sep 12, 2024
052f3c3
Add hadolint ignore=DL3008
ethanstrominger Sep 12, 2024
8089052
Revert .env.docker-example
ethanstrominger Sep 12, 2024
60c27b3
Removed unneeded files
ethanstrominger Sep 12, 2024
5e39d61
Revert changes to Dockerfile
ethanstrominger Sep 13, 2024
be33e78
Merge branch 'main' into secured-apis
ethanstrominger Sep 13, 2024
b43f003
Merge branch 'main' of https://github.com/hackforla/peopledepot into …
ethanstrominger Sep 20, 2024
842c33f
Remove unneeded code, rename secret to api_key, and update doc
ethanstrominger Sep 20, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions app/.env.docker-example
Original file line number Diff line number Diff line change
Expand Up @@ -36,5 +36,3 @@ DATABASE=postgres
COGNITO_DOMAIN=peopledepot
COGNITO_AWS_REGION=us-west-2
COGNITO_USER_POOL=us-west-2_Fn4rkZpuB

PEOPLE_DEPOT_API_SECRET=people-depot-api-secret
61 changes: 61 additions & 0 deletions app/core/api/api_key_views.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
# from rest_framework import serializers as rest_serializers
from drf_spectacular.utils import extend_schema
from drf_spectacular.utils import extend_schema_view
from rest_framework import viewsets
from rest_framework.permissions import IsAuthenticatedOrReadOnly
from rest_framework_api_key.permissions import HasAPIKey

from core.api.serializers import PracticeAreaSerializer
from core.api.serializers import UserSerializer
from core.models import PracticeArea
from core.models import User


@extend_schema_view(
list=extend_schema(
description="""
Lists all users and the associated groups for the user

Requires
- API keys have been added using the API key screen in the People depot App.
Check with People Depot admin.
- Requires X-Api-Key be set when submitting request
For curl, it would look like this:
```http
curl -L -X GET "localhost:8001/api/v1/apikey/getusers/"
-H "X-Api-Key: *************"
-H "Content-Type: application/json"
```


For python, it would look like this:
```bash
import requests

API_KEY = os.environ.get("API_KEY")
BASE_URL = "http:8000"
HEADERS = {
'X-Api-Key': API_KEY,
'Content-Type': 'application/json'
}
response = requests.get(f"{BASE_URL}/api/v1/apikey/getusers/", headers=HEADERS)
```
"""
)
)
class ApiKeyUserViewSet(viewsets.ReadOnlyModelViewSet):
# HasAPIKey checks if value of X-API-KEY is in API keys table
permission_classes = [IsAuthenticatedOrReadOnly, HasAPIKey]
queryset = PracticeArea.objects.all()
serializer_class = PracticeAreaSerializer # HasAPIKey checks against keys stored in
Comment on lines +49 to +50
Copy link
Member

Choose a reason for hiding this comment

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

These 2 lines don't seem to do anything and they're soon replaced by the next 2 code lines of User objects and UserSerializer. Maybe you meant to remove them?

queryset = User.objects.all()

# when instantiated, get_serializer_context will be called
serializer_class = UserSerializer
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure you should be returning all the User data here rather than a subset that doesn't contain personally identifiable information (PII)?
What I mean is UserSerializer is exposing all the data.

I don't know if this PR is dependent on the user permission changes to somehow limit the exposed data fields, but the code right now will return all user table data, which doesn't seem like a good idea.

Copy link
Member

Choose a reason for hiding this comment

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

This probably should have been settled in the planning stage of this feature rather than during the PR review.

To clarify further, the problem I see is this new API key authentication method allows the API client to view user data without any of the limitations that are applied to permission-tiered users.

These are the fields it returns for user:

        fields = (
            "uuid",
            "username",
            "created_at",
            "updated_at",
            "email",
            "first_name",
            "last_name",
            "gmail",
            "preferred_email",
            "current_job_title",
            "target_job_title",
            "current_skills",
            "target_skills",
            "linkedin_account",
            "github_handle",
            "slack_id",
            "phone",
            "texting_ok",
            "time_zone",
            "groups",
        )

I'm thinking that KnowledgeBase doesn't really need or care about a user's phone number to function.

So I think we need to consult the "permissions team" (@ExperimentsInHonesty @Neecolaa) to figure out/negotiate what data this should include based on what data KnowledgeBase requires to function.

This seems to mean adding more/duplicate work to the "permissions" team (@ExperimentsInHonesty and @Neecolaa) to define exactly what tables and fields this new method can allow an API client to access. Would it be better to say the API client has the same permissions as one of the already-defined user permissions tier such as unverifiedUser or stakeholder API? In that case, wouldn't it be simpler to just use a service account (special user account with the right permission level) instead?

For reference: here's the Field Permissions sheet being worked on currently.


# get_serializer_context called to set include_groups to True
# to include groups in the response
def get_serializer_context(self):
context = super().get_serializer_context()
context["include_groups"] = True
return context
23 changes: 22 additions & 1 deletion app/core/api/serializers.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from django.contrib.auth.models import Group
from rest_framework import serializers
from timezone_field.rest_framework import TimeZoneSerializerField

Expand Down Expand Up @@ -39,6 +40,12 @@ class Meta:
)


class GroupSerializer(serializers.ModelSerializer):
class Meta:
model = Group
fields = ("id", "name")


class UserPermissionSerializer(serializers.ModelSerializer):
"""Used to retrieve user permissions"""

Expand All @@ -61,9 +68,16 @@ class Meta:


class UserSerializer(serializers.ModelSerializer):
"""Used to retrieve user info"""
"""
Serializer for User model.

Parameters:
- include_groups (bool): Flag to include user groups in the serialized output.
"""

time_zone = TimeZoneSerializerField(use_pytz=False)
# see get_groups method
groups = serializers.SerializerMethodField()

class Meta:
model = User
Expand All @@ -87,6 +101,7 @@ class Meta:
"phone",
"texting_ok",
"time_zone",
"groups",
)
read_only_fields = (
"uuid",
Expand All @@ -96,6 +111,12 @@ class Meta:
"email",
)

def get_groups(self, obj):
include_groups = self.context.get("include_groups", False)
if include_groups:
return GroupSerializer(obj.groups.all(), many=True).data
return None


class ProjectSerializer(serializers.ModelSerializer):
"""Used to retrieve project info"""
Expand Down
3 changes: 3 additions & 0 deletions app/core/api/urls.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from django.urls import path
from rest_framework import routers

from .api_key_views import ApiKeyUserViewSet
from .views import AffiliateViewSet
from .views import AffiliationViewSet
from .views import CheckTypeViewSet
Expand Down Expand Up @@ -37,6 +38,8 @@
router.register(
r"stack-element-types", StackElementTypeViewSet, basename="stack-element-type"
)
router.register(r"apikey/getusers", ApiKeyUserViewSet, basename="apikey-getusers")
router.register(r"sdgs", SdgViewSet, basename="sdg")
Copy link
Member

Choose a reason for hiding this comment

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

Looks like a duplicate line got added here?

router.register(r"sdgs", SdgViewSet, basename="sdg")
router.register(
r"affiliations",
Expand Down
29 changes: 29 additions & 0 deletions app/core/tests/test_api_key.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import datetime

from django.urls import reverse
from rest_framework import status
from rest_framework.test import APITestCase
from rest_framework_api_key.models import APIKey

api_key_url = reverse("apikey-getusers-list")


class ApiKeyUserViewSetTests(APITestCase):
def test_succeeds(self):
_, api_key = APIKey.objects.create_key(name="test")
response = self.client.get(
api_key_url,
HTTP_X_API_KEY=api_key, # Uppercase X-API-KEY
Content_Type="application/json",
)
assert response.status_code == status.HTTP_200_OK

def test_expired_fails(self):
expired_date = datetime.datetime.now() - datetime.timedelta(days=2)
_, api_key = APIKey.objects.create_key(name="test", expiry_date=expired_date)
response = self.client.get(
api_key_url,
HTTP_X_API_KEY=api_key, # Uppercase X-API-KEY
Content_Type="application/json",
)
assert response.status_code == status.HTTP_401_UNAUTHORIZED
9 changes: 8 additions & 1 deletion app/peopledepot/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,16 @@
# Build paths inside the project like this: BASE_DIR / 'subdir'.
BASE_DIR = Path(__file__).resolve().parent.parent

# Used by djangorestframework-api-key
API_KEY_CUSTOM_HEADER = "HTTP_X_API_KEY"

# Quick-start development settings - unsuitable for production
# See https://docs.djangoproject.com/en/4.0/howto/deployment/checklist/

# SECURITY WARNING: keep the secret key used in production secret!
SECRET_KEY = os.environ.get("SECRET_KEY")


Copy link
Member

@fyliu fyliu Sep 23, 2024

Choose a reason for hiding this comment

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

Adding an extra blank line? Possibly a merging artifact?

DJANGO_SUPERUSER_USERNAME = os.environ.get("DJANGO_SUPERUSER_USERNAME")
DJANGO_SUPERUSER_EMAIL = os.environ.get("DJANGO_SUPERUSER_EMAIL")
DJANGO_SUPERUSER_PASSWORD = os.environ.get("DJANGO_SUPERUSER_PASSWORD")
Expand Down Expand Up @@ -70,6 +73,7 @@
# 3rd party
"django_extensions",
"rest_framework",
"rest_framework_api_key",
"drf_spectacular",
"phonenumber_field",
"timezone_field",
Expand Down Expand Up @@ -175,7 +179,10 @@
]

REST_FRAMEWORK = {
"DEFAULT_PERMISSION_CLASSES": ("core.api.permissions.DenyAny",),
"DEFAULT_PERMISSION_CLASSES": (
"core.api.permissions.DenyAny",
"rest_framework_api_key.permissions.HasAPIKey",
),
"DEFAULT_AUTHENTICATION_CLASSES": (
"rest_framework_jwt.authentication.JSONWebTokenAuthentication",
),
Expand Down
2 changes: 2 additions & 0 deletions app/requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ djangorestframework==3.14.0
# via
# drf-jwt
# drf-spectacular
djangorestframework-api-key>=3.0
Copy link
Member

Choose a reason for hiding this comment

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

We're using pip-compile (actually the uv version of it) to generate requirements.txt from requirements.in . You can add the dependency to requirements.in and run this command to generate requirements.txt.

docker-compose exec web uv pip compile requirements.in -o requirements.txt --no-header 

There's documentation on how to use it for upgrading dependencies. The command above is just missing the --upgrade flag.

drf-jwt==1.19.2
drf-spectacular==0.27.1
flake8==7.0.0
Expand Down Expand Up @@ -59,6 +60,7 @@ platformdirs==4.2.0
# via black
pluggy==1.4.0
# via pytest
pre-commit==3.7.1
Copy link
Member

Choose a reason for hiding this comment

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

Why do you want to install pre-commit inside the docker container?

psycopg2-binary==2.9.9
pycodestyle==2.11.1
# via flake8
Expand Down