Skip to content

AAI-421: add admin endpoints for approve and revoke access to platforms and groups#81

Merged
amandazhuyilan merged 15 commits into
mainfrom
aai-421-capture-revoke-reasoning
Sep 29, 2025
Merged

AAI-421: add admin endpoints for approve and revoke access to platforms and groups#81
amandazhuyilan merged 15 commits into
mainfrom
aai-421-capture-revoke-reasoning

Conversation

@amandazhuyilan

@amandazhuyilan amandazhuyilan commented Sep 25, 2025

Copy link
Copy Markdown
Contributor

Description

AAI-421: add admin endpoints for approve and revocation (with reasons) for groups and platforms.

Changes

  • New approval admin endpoint added for groups
  • Revoke endpoint now accepts the body, propagates the reason, and guards missing services
  • Admin and model tests updated to cover the new reason behaviour

Checklist

  • I have commented my code, particularly in hard-to-understand areas
  • I have added unit / integration tests that prove my fix is effective or that my feature works
  • I have run all tests locally and they pass
  • I have updated the documentation (if applicable)

How to Test Manually (if necessary)

<Describe how reviewers can manually, if necessary, test the changes>

@amandazhuyilan amandazhuyilan force-pushed the aai-421-capture-revoke-reasoning branch 3 times, most recently from 4db2994 to fbdf319 Compare September 25, 2025 04:57
@amandazhuyilan amandazhuyilan changed the title AAI-421: add admin route for revoke reasons AAI-421: add admin route for revoke access to platforms and groups Sep 25, 2025
@amandazhuyilan amandazhuyilan force-pushed the aai-421-capture-revoke-reasoning branch from 877c22b to f628917 Compare September 28, 2025 08:55
@amandazhuyilan amandazhuyilan changed the title AAI-421: add admin route for revoke access to platforms and groups AAI-421: add admin route for approve and revoke access to platforms and groups Sep 28, 2025

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds administrative endpoints for approving and revoking user access to platforms and groups, with support for storing revocation reasons up to 1024 characters.

Key changes:

  • New admin endpoints for approve/revoke operations on platforms, groups, and generic services
  • Database schema updates to store revocation reasons in membership and history tables
  • Request validation model that enforces trimming and character limits for revocation reasons

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
routers/admin.py Adds new admin endpoints and helper functions for approve/revoke operations
db/models.py Updates membership models to include revocation_reason field and history tracking
db/types.py Adds revocation_reason field to membership data models
tests/test_admin.py Test coverage for new approve/revoke functionality
tests/db/test_models.py Tests for revocation reason storage in models and history
migrations/ Database migration files for adding revocation_reason columns
README.md Minor documentation improvements for migration generation

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread routers/admin.py Outdated
Comment thread routers/admin.py Outdated

@marius-mather marius-mather left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A few changes needed here:

  • Unless I've missed something, platform and group approvals are separate and done by different people. So we don't need combined endpoints that do both.
  • We need to confirm that the admin has permission to act on the specific platform/group by checking their roles in their access token against the admin roles defined in the DB
  • Not sure why we ended up with multiple migration files - is it possible to delete the new ones, and see if generating them again creates a single migration (might help to merge main into this branch first)

Comment thread db/models.py
Comment thread migrations/versions/2a0012a7fb99_enable_admin_revoke.py Outdated
Comment thread routers/admin.py Outdated
Comment thread routers/admin.py Outdated
Comment thread routers/admin.py Outdated
Comment thread routers/admin.py
Comment thread routers/admin.py
Comment thread routers/admin.py
Comment thread tests/db/test_models.py
Comment thread tests/test_admin.py
@amandazhuyilan

Copy link
Copy Markdown
Contributor Author

Ready for review again @marius-mather 👍 - a list of changes:

  • Added a check for the admin roles before approving or revoking members to the groups / platforms
  • Separated the approvals and revokes for platforms and groups, previously grouped them as services and resources
  • Merged empty migration files
  • Update tests accordingly

@amandazhuyilan amandazhuyilan changed the title AAI-421: add admin route for approve and revoke access to platforms and groups AAI-421: add admin endpoints for approve and revoke access to platforms and groups Sep 29, 2025

@marius-mather marius-mather left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Needs some slight tweaking in terms of how we look up groups and platforms - we should do this from the DB wherever possible, PLATFORM_MAPPING/GROUP_MAPPING are workarounds that should only be needed in specific cases.

I've fixed up the migrations so that the previous platform-related ones aren't deleted - they might have already been applied on the deployment so we don't want to remove them.

Otherwise looks good to go.

Comment thread routers/admin.py Outdated
Comment thread routers/admin.py Outdated
@amandazhuyilan amandazhuyilan force-pushed the aai-421-capture-revoke-reasoning branch from e3afb56 to 9d9218c Compare September 29, 2025 07:09
@amandazhuyilan

Copy link
Copy Markdown
Contributor Author

One last review please @marius-mather 🙏

@marius-mather

Copy link
Copy Markdown
Collaborator

@amandazhuyilan force-pushing undid the fix I did for migrations - please don't force push or it can break things like that. I will fix migrations again now, after that this is looking good to merge.

@marius-mather marius-mather left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looking good now, thanks!

@amandazhuyilan amandazhuyilan merged commit 4badec0 into main Sep 29, 2025
3 checks passed
@amandazhuyilan amandazhuyilan deleted the aai-421-capture-revoke-reasoning branch November 24, 2025 22:22
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