Secure API for listing users and groups

[ethanstrominger]

Fixes #331

What changes did you make?

A new api secret-api/getusers lists users and the groups the users are assigned to. The API is authorized through an API key rather than user credentials.

• secret_views.py: view for securely viewing all users - requires api key. Calls user_serializer with include_group = True and calls permission in secret_permissions.
• serializer.py: add serializer for Django.contrib.groups and add include_groups paramter to UserSerializer to optionally include user groups in response
• test_secret_api.py: tests secret based API
• settings.py: standard changes required by djangorestframework_api_key
• urls.py: standard changes required by djangorestframework_api_key
• requirements.txt - added djangorestframework_api_key

Why did you make the changes (we will use this info to test)?

See issue #331 and see test_secret_api.py fir tests,

[fyliu]

Looks like you're trying to implement your own simple security protocol of "does the requesting client have the global secret key?". That doesn't seem very secure.

Can you comment on what makes this implementation secure? And why not just use Cognito since we already have that?

[fyliu]

Thanks for adding the code to support API keys!

I found a few problems, but the thing that's blocking my ability to do the review is I need confirmation for what part of the User model it's okay to return in the endpoint. I remember Bonnie said PracticeArea and ProgramArea are public in this comment.

[fyliu]

Looks like a duplicate line got added here?

[fyliu]

Adding an extra blank line? Possibly a merging artifact?

[fyliu]

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.

[fyliu]

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

[fyliu]

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?

[fyliu]

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.

[fyliu]

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.