Provider admin endpoints and client id restrictions by chrisarridge · Pull Request #79 · IATI/register-your-data-api

chrisarridge · 2026-04-28T09:12:23Z

This PR contains two significant changes and some placeholder changes.

The FGA Provider and Validator are modified to store the client id of the calling application, and of the tools that are used to access RYD by provider admin. As part of these modifications RYD can now accommodate multiple provider_admin roles per user per reporting org (to reflect multiple tools being used for a given reporting org). The validator now checks the client id of the caller against the client id of the tool. Tests are updated to reflect this change. This should also enable easier changes in the future if we decide per-tool fine-grained permissions are required.
Endpoints are provided to get lists of tools known to RYD and list user roles to support tooling in knowing the range of permissions a user has.

We also add placeholder not-implemented handlers for provider admin management endpoints and a fine-grained permissions endpoint and tests that these return 501.

This commit enhances the FGA provider database test to account for adding the client id to the ToolDbModel and a tool restriction to the FineGrainedAuthorisationRoleAuthorisation. The main update in this test is to validate storing tools and getting multiple provider_admin roles per reporting organisation per user. It also tidies up the provider test to reduce the verbosity.

This commit adds two features to the FGA functionality as a prelude to restricting the clients that provider_admin can be used with. Firstly, it adds tool client IDs to the FGA ToolDBModel table. It adds a field to the FineGrainedAuthorisationRoleAssociation model so that we can track which tool (client) an association is valid for. For a regular reporting org role this is null, for a provider_admin it will be linked to a tool. To enable this functionality, a small refactoring was added to allow get_user_role_for_org to return more than one role per user and per org (e.g., provider admin via multiple tools).

…r_admin This commit changes the FGA validator to store the client ID of the caller and a list of tools as well as the fine grained authorisations. Internally the validator now parses provider_admin and checks if the tools being used for provider_admin match the client id and return provider_admin roles as appropriate.

When initialising the FineGrainedAuthorisationUserValidator the user client ID and list of user tools is added to the validator.

Add test of the new /tools endpoint to fetch a list of tools known to RYD.

Implements the GET /tools list endpoint.

Adds tests of the /user/{uid}/roles endpoint for returning a list of user roles, and a test for the (not yet implemented) /user/{uid}/roles/reporting-org-permissions/{oid} endpoint.

…dpoint This commit implements the endpoint to return a list of user roles. It also adds a 501 response for the (not yet implemented) permissions endpoint.

simon-20 · 2026-04-28T10:53:12Z


    user_related_to_org_in_crm = find_item_in_suitecrm_response(users_for_org_from_suitecrm, str(user_id))

    if user_related_to_org_in_crm is None:


If this endpoint were ever called by a reporting org admin targeting a provider admin for this organisation, the block starting at line 363 would create a relationship in the CRM between the provider admin user and the organisation.

Maybe we don't need to worry too much about this, as we'll never be setting up a way for admins to PUT to this endpoint with the details of a provider admin, but it's possible a 3rd party tool provider may make such an incorrect call.

simon-20 · 2026-04-28T10:55:52Z

    # 8. Update the user's role in the FGA database
    # This is safe as new_role has been validated by Pydantic
-    user_role_for_org.role = get_fga_role_from_str(new_role.role)
+    user_roles_for_org[0].role = get_fga_role_from_str(new_role.role)


In a similar manner to above, if this endpoint were ever called (incorrectly) targeting a provider admin user, the code on line 411 would crash, I think, when it tries to save the update of the role to the DB.

simon-20 · 2026-04-28T11:04:09Z

    )

-    user_role_for_org = context.fine_grained_auth_provider.get_user_role_for_org(user_id, org_id)
+    user_roles_for_org = context.fine_grained_auth_provider.get_user_roles_for_org(user_id, org_id)


Again thinking defensively, if this were called with a target user who was a provider admin, I think the checks below would all pass, and then the code would crash when trying to delete a provider admin from the normal user FGA table.

simon-20 · 2026-04-28T12:26:09Z

+    context: Context = request.app.state.context
+
+    crm: SuiteCRM = context.suitecrm_client_factory.get_client()
+    # import pdb


Remove these debugging lines?

simon-20 · 2026-04-28T12:33:02Z

+
+
+@router.delete("/{org_id}/tools/{tool_id}")
+def revoke_tool_permission_for_reporting_org(


The route handler definition is missing the tool_id parameter.

simon-20 · 2026-04-28T12:35:48Z

+        ]
+        if len(regular_associations) > 1:
+            if (
+                collections.Counter([association.user for association in regular_associations]).most_common(1)[0][1]


A tiny optional suggestion, which is purely stylistic: I'd be tempted to do a from collections import Counter to avoid the collections prefix and so potentially avoid breaking this long comparison line (as Counter isn't ambiguous here). But understand some find the comparisons over line breaks quite readable.

Or, as an alternative, you could do:
max(collections.Counter([association.user for association in regular_associations]).values(), default=0) which would avoid the need for the if len(regular_associations) > 1: guard clause.

Good stuff. I wasn't happy about this myself and this is a great solution.

simon-20 · 2026-04-28T12:38:59Z


-        return associations
+        # Check that a provider admin is not in the list of regular associations.
+        if len(set([x.user for x in regular_associations]) & set([x.user for x in provider_admin_associations])) > 0:


Also an entirely stylistic note: sets evaluate to falsy when empty like lists do, so you could mirror the pattern you've used above (e.g., line 121) and just do:

if set([x.user for x in regular_associations]) & set([x.user for x in provider_admin_associations]):

Thanks - this is really good. I generally don't like relying on empty lists evaluating to False as I don't find it easy to parse, but the idea that the intersection of two disjoint sets evaluate to False is absolutely dandy :)

…oles

chrisarridge added 11 commits April 27, 2026 18:37

test: FGA validator rejects provider_admin when client id doesn't match

d4ff91e

feat: add client id and tools to FineGrainedAuthorisationUserValidator

a97a3ee

When initialising the FineGrainedAuthorisationUserValidator the user client ID and list of user tools is added to the validator.

test: fix dataset and reporting org routes to work with client id

ed48a42

test: add tools list endpoint test

716bd40

Add test of the new /tools endpoint to fetch a list of tools known to RYD.

feat: implement tools list endpoint

f65b097

Implements the GET /tools list endpoint.

test: user roles and reporting org permissions endpoints

bba75b4

Adds tests of the /user/{uid}/roles endpoint for returning a list of user roles, and a test for the (not yet implemented) /user/{uid}/roles/reporting-org-permissions/{oid} endpoint.

feat: implement user roles endpoint and return 501 for permissions en…

58035f5

…dpoint This commit implements the endpoint to return a list of user roles. It also adds a 501 response for the (not yet implemented) permissions endpoint.

test: provider admin management reporting org endpoints return 501

afa889b

chrisarridge requested a review from simon-20 April 28, 2026 09:12

simon-20 approved these changes Apr 28, 2026

View reviewed changes

chrisarridge added 5 commits April 28, 2026 18:42

feat: add 501 responses for reporting org tool managmeent endpoints

8baa302

fix: added guards to make sure tool admin users cannot be given org r…

aade789

…oles

refactor: small refactoring changes in fga provider DB

67859b4

bump: version bump

1ebdcd6

docs: updated CHANGELOG

94046ec

chrisarridge force-pushed the ca/provider-admin-endpoints branch from af67d73 to 94046ec Compare April 28, 2026 17:42

chrisarridge merged commit ff1a342 into develop Apr 28, 2026
5 checks passed

chrisarridge deleted the ca/provider-admin-endpoints branch April 28, 2026 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provider admin endpoints and client id restrictions#79

Provider admin endpoints and client id restrictions#79
chrisarridge merged 16 commits intodevelopfrom
ca/provider-admin-endpoints

chrisarridge commented Apr 28, 2026

Uh oh!

simon-20 Apr 28, 2026

Uh oh!

simon-20 Apr 28, 2026

Uh oh!

simon-20 Apr 28, 2026

Uh oh!

simon-20 Apr 28, 2026

Uh oh!

simon-20 Apr 28, 2026

Uh oh!

simon-20 Apr 28, 2026

Uh oh!

chrisarridge Apr 28, 2026

Uh oh!

simon-20 Apr 28, 2026

Uh oh!

chrisarridge Apr 28, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		user_related_to_org_in_crm = find_item_in_suitecrm_response(users_for_org_from_suitecrm, str(user_id))

		if user_related_to_org_in_crm is None:



		@router.delete("/{org_id}/tools/{tool_id}")
		def revoke_tool_permission_for_reporting_org(

Conversation

chrisarridge commented Apr 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants