feat: remove SAML provider admin views from openedx-platform#38104
feat: remove SAML provider admin views from openedx-platform#38104
Conversation
b6bed01 to
9d68b41
Compare
There was a problem hiding this comment.
Pull request overview
This PR removes enterprise-admin-only SAML provider management REST endpoints from openedx-platform and documents the architectural decision to host those endpoints in the edx-enterprise plugin, preserving the existing API contract under the same URL prefix.
Changes:
- Added an ADR documenting the move of
provider_config/provider_dataSAML admin viewsets toedx-enterprise. - Unregistered (and removed) the SAML provider admin viewsets, URLs, and their API tests from
common.djangoapps.third_party_auth. - Cleaned up
third_party_auth.utilsby removing now-unused SAML admin helper functions and adding focused unit tests forcreate_or_update_bulk_saml_provider_data.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| docs/decisions/0025-saml-admin-views-in-enterprise-plugin.rst | Documents the decision and consequences of moving the SAML admin endpoints to the enterprise plugin. |
| common/djangoapps/third_party_auth/utils.py | Removes SAML-admin-only helpers and associated unused imports; keeps core SAML metadata parsing + provider data upsert logic. |
| common/djangoapps/third_party_auth/urls.py | Stops registering the removed admin endpoints; adds a note about routes now provided by edx-enterprise. |
| common/djangoapps/third_party_auth/tests/test_utils.py | Removes tests for deleted helpers; adds tests for create_or_update_bulk_saml_provider_data. |
| common/djangoapps/third_party_auth/samlproviderdata/views.py | Deleted: removes enterprise-admin-only Provider Data viewset from platform. |
| common/djangoapps/third_party_auth/samlproviderdata/urls.py | Deleted: removes router registration for Provider Data endpoints. |
| common/djangoapps/third_party_auth/samlproviderdata/tests/test_samlproviderdata.py | Deleted: removes platform-level API tests for Provider Data endpoints. |
| common/djangoapps/third_party_auth/samlproviderconfig/views.py | Deleted: removes enterprise-admin-only Provider Config viewset from platform. |
| common/djangoapps/third_party_auth/samlproviderconfig/urls.py | Deleted: removes router registration for Provider Config endpoints. |
| common/djangoapps/third_party_auth/samlproviderconfig/tests/test_samlproviderconfig.py | Deleted: removes platform-level API tests for Provider Config endpoints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e7bf0b1 to
73299f8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
73299f8 to
1860f66
Compare
Other steps taken too: - Add ADR and URL routes comment for migrated SAML admin views to help ensure people don't inadvertantly register conflicting URL routes. - Remove validate_uuid4_string, convert_saml_slug_provider_id, and fetch_metadata_xml from third_party_auth/utils.py (migrated to edx-enterprise) - Remove test_convert_saml_slug_provider_id test and import - Add TestCreateOrUpdateBulkSAMLProviderData test class with 4 tests covering create, update, bulk_update, and multiple keys paths - Remove unused imports: logging, requests, uuid.UUID ENT-11567 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1860f66 to
1d23dff
Compare
|
Created edx#231 to deploy to the 2U fork. |
This change to remove URLs is safe because bundled in this same commit is an upgrade of edx-enterprise to a version which re-adds the URLs.
ENT-11567
Related:
Integration testing in local devstack:
Steps to seed data for integration testing in devstack
Claude-generated script to create a Postman collection
Manually testing the postman collection against my devstack running the new code and with seeded test data was successful.