Conversation
This commit fixes an apparently failing test of the delete user endpoint that prevents the last admin user from being deleted from a reporting organisation (test_delete_user_role_cannot_delete_last_admin_user).
This adds an integration test for the provider admin changes to the PostgreSQL FGA provider database.
This commit adds the provider admin database models, database migration, and associated code to support refactored provider admin functionality. The provider_admin role is now generated dynamically by finding the users of a tool (ToolUserDbModel) that has authorisation to access a reporting organisation (ToolAuthorisationDbModel). It also adds an integrity check whereby a user cannot be associated with a tool and also have an authorisation (admin, editor or contributor) for a reporting organisation.
This adds a unit test to ensure that provider admins permissions are correctly parsed and validated by the FGA validator. This commit does not add checks for other role types, but this could be done in the future.
This commit modifies the FGA validator to change the fine-grained authorisations granted to provider admins.
Update the mock configuration and add additional SuiteCRM mocked responses to test the revised provider admin functionality.
Adds tests that check provider admin works correctly on user endpoints.
This commit adds the necessary changes to implement provider admin in user endpoints. This essentially involves making sure that the new FGA integrity exception is correctly handled in the dependencies.
This commit adds/updates reporting org endpoint tests for new provider admin functionality.
This commit adds a new unit test that users have the correct permissions for changing the visibility of a dataset. This requires a change to the mock suitecrm object so that it can return a dataset by ID and to handle an update_record() call.
| reporting_org: UUID | ||
|
|
||
|
|
||
| class ToolUserDbModel(SQLModel, table=True): |
There was a problem hiding this comment.
This all looks good, my only thought so far is that ToolUserDbModel may (going forward) have the potential to be confusing, because the clients/customers of the 3rd party tools are also aptly described as "(3rd party) tool users". I wonder if ToolAdminUserDbModel might be more explanatory? Maybe that was discounted for another reason?
| class ToolUserDbModel(SQLModel, table=True): | ||
| id: UUID = Field(primary_key=True, default_factory=lambda: uuid4()) | ||
| tool: UUID = Field(foreign_key="tooldbmodel.id") | ||
| user: UUID |
| .where(ToolUserDbModel.user == user) | ||
| ).all() | ||
|
|
||
| if user_db_fgas and providers_reporting_orgs: |
There was a problem hiding this comment.
This will block people from being both a provider admin for a tool, and also associated with a govern organisation in the normal way - is that correct? But some 3rd party tool providers publish datasets (whether or not they should or need to I don't know, but they do), so we might need to think that use-case through.
| ] | ||
|
|
||
| if len(associations) > 1: | ||
| if collections.Counter([association.user for association in associations]).most_common(1)[0][1] > 1: |
There was a problem hiding this comment.
This would also pick up if the FGA database gets into an error state where a user has two entries in there for the same org. I'm not suggesting this code needs to be changed, just flagging.
| "delete-dataset", | ||
| ] | ||
| permissions[FineGrainedAuthorisationRole.PROVIDER_ADMIN] = [ | ||
| *permissions[FineGrainedAuthorisationRole.EDITOR], |
There was a problem hiding this comment.
Is this change in the requirements for provider admin permissions documented somewhere?
2 participants
This PR refactors and enabled provider admin functionality which closes #56:
Note: I've added tests and then the feature work to made the tests pass, but this creates a failing repo at the test commits. Before merging I'll squash the feature and test commits together.