509: Add Study Read Permission to Approved User #526

leoraba · 2025-12-10T23:07:44Z

Description

When application is approved, it should assign applicant and collaborators the corresponding permission to the specified study.

Details

Uses authz to assign an user to an specific study. (more info here)
~~🐛 An issue reported in authz. Permissions are removed automatically after couple of minutes~~ RESOLVED
Fetching the list of studies is not implemented yet. This PR uses a hardcoded list of studies for dev/testing purpose.
When an application is approved, the applicant and collaborators email addresses are used to retrieve their PCGL user IDs, which are then used to grant permissions for each study. Only users whose permissions are successfully granted are notified by email.
This PR does not include a retry mechanism for granting user permissions.
Added new env variables to configure retry fetch calls, default values:
```
FETCH_RETRIES=3
FETCH_RETRY_DELAY_MS=500
FETCH_TIMEOUT_MS=10000
```

Ticket

#509

leoraba · 2025-12-11T17:49:34Z

apps/ui/src/pages/applications/sections/requestedStudy.tsx

+	{
+		studyName: 'TEST-CA',
+		studyID: 'TEST-CA',
+	},


fetching the list of studies available is not implemented yet.. hardcoding for dev/testing purpose

leoraba · 2025-12-11T17:51:33Z

packages/data-model/src/types.ts


 export type ApproveApplication = {
 	applicationId: number; // The ID of the application to be approved
+	approverAccessToken: string; // The access token of the user approving the application


using the approver's access_token to add permissions in authz service. This requires that the DAC_CHAIR to be part of the CO:admin` group to allow this type of requests

leoraba · 2025-12-11T17:54:04Z

apps/ui/src/components/pages/dashboard/ApplicationStatusBar.tsx

 									<CheckCircleFilled style={{ color: token.colorPrimary, fontSize: 30 }} />
 									<Flex vertical>
 										<Text strong>{translate('dashboard.hasAccess')}</Text>
-										<Text>{formatDate(expiresAt)}</Text>


removing Expires At date on the top status bar, as per ticket specification.

Should the check on line 63 confirm that the current date is before the expiresAt date?

added a condition as suggested.

leoraba · 2025-12-11T17:58:42Z

apps/api/.env.schema

+# -- Approved Application --
+APPROVED_PERMISSION_EXPIRES_IN_DAYS=365


new env variable, updated readme file for more details

joneubank

Good work. We need a bit more to deal with error cases. What is missing is that if the request to add permissions fails, then we have no way remedy. We may need a manual process to correct individual failures, or some other mechanism to ensure the AuthZ state has all data from DACO.

joneubank · 2025-12-11T20:42:54Z

apps/api/src/config/authConfig.ts

 }

 const authConfigSchema = z.object({
+	APPROVED_PERMISSION_EXPIRES_IN_DAYS: z.coerce.number().int().optional().default(365),


joneubank · 2025-12-11T20:47:39Z

apps/api/src/service/permissionService.ts

+	institutionalEmail,
+	approverAccessToken,
+	requestedStudies,
+}: AssignUserPermissionsAndNotifyParams) => {


Would be good to declare the return type.

added return type to this function.

joneubank · 2025-12-11T20:53:24Z

apps/api/src/controllers/applicationController.ts

+			const permissionAdded = await assignUserPermissions({
+				institutionalEmail: applicant_institutional_email,
+				approverAccessToken,
+				requestedStudies: requested_studies,
+			});
+
+			if (permissionAdded) {
+				emailService.sendEmailApproval({
+					id: application.id,
+					to: applicant_institutional_email,
+					name: applicant_first_name || 'N/A',
+				});
+			}


We should have some behaviour that handles the case where we fail to successfully assign user permissions.

We should at minimum have a retry mechanism for failed requests.

What I'm thinking about though is that we need a mechanism to notify an admin that the approval was completed but the permissions weren't granted, and then the admin needs a mechanism to correct the issue.

~~To handle user permissions, included following changes:~~

~~A validation was added early in the code, to make sure applicant and collaborator's email address are valid PCGL users. This avoid approving an application with invalid users.~~

~~Implemented a retry mechanism on calls used to add user permission to Authz service~~

~~In case of an error occurs during approving the application. the DAC approver and the Applicant will be notified via email, UI displays a notification toast with a friendly error message.~~

A mechanism to handle retries will be implemented in a separate PR.

joneubank · 2025-12-11T20:57:58Z

apps/api/src/service/permissionService.ts

+/**
+ * This function assigns user permissions to the requested studies
+ * First it looks for the associated PCGL user ID using the institutional email
+ * Then it assigns the study permissions using the approver's access token
+ *
+ * @param param0
+ * @returns
+ */
+export const assignUserPermissions = async ({
+	institutionalEmail,
+	approverAccessToken,
+	requestedStudies,
+}: AssignUserPermissionsAndNotifyParams) => {


This function could fail at multiple stages, and the output from this function should correctly communicate where it failed and any work that was done.

It could fail due to auth or network reasons not related to this specific request content

It could fail because the user was not found (should not happen but let's handle this anyways).

It could fail adding permission for some studies, but succeed for others.

If the request to add permissions fails for any studies, the response should include a list of studies failed and studies succeeded.

~~Retry Authz calls and Emails are sent when an error occur during approval.~~

Added a retry mechanism to the main function used for making requests to Authz service.

Once the retry limit is exceeded, errors are not handled in this PR, a separate PR will be required to implement proper error handling.

joneubank · 2025-12-11T20:58:07Z

apps/api/README.md

 | `VALKEY_USER`        | Valkey user name.                                                                                                                                                                                                                                                                                                                          | `string`  | Required                                  |            |
 |                      |                                                                                                                                                                                                                                                                                                                                            |           |                                           |            |
 | `DISABLE_AUTH`       | Set this to `true` to disable auth. Any other value and this will default to auth enabled.                                                                                                                                                                                                                                                 | `boolean` | Optional                                  | `false`    |
+| `APPROVED_PERMISSION_EXPIRES_IN_DAYS`       | Number of days the applicant will have **read** access to the study when application gets approved.                                                                                                                                                                                                                                                 | `number` | Optional                                  | `365` days    |


joneubank · 2025-12-11T20:59:52Z

apps/ui/src/components/pages/dashboard/ApplicationStatusBar.tsx

 									<CheckCircleFilled style={{ color: token.colorPrimary, fontSize: 30 }} />
 									<Flex vertical>
 										<Text strong>{translate('dashboard.hasAccess')}</Text>
-										<Text>{formatDate(expiresAt)}</Text>


Should the check on line 63 confirm that the current date is before the expiresAt date?

leoraba · 2025-12-16T21:39:29Z

apps/api/src/controllers/applicationController.ts

+		const verifyUserAccountsResult = await verifyApplicationUserAccounts(applicationId, approverAccessToken);
+
+		if (!verifyUserAccountsResult.success) {
+			return verifyUserAccountsResult;
+		}


applicant and collaborators must be already registered as pcgl users.

leoraba · 2025-12-16T21:43:15Z

apps/ui/src/api/mutations/useApproveApplication.ts

-				message: translate('modals.approveApplication.notifications.applicationApproveFailed'),
-			});
+		onError: async (data, { applicationId }) => {
+			switch (data.error) {


User friendly errors to be displayed in the UI when application has errors during approval.

leoraba · 2025-12-16T21:44:07Z

apps/ui/src/components/pages/application/ApplicationViewerHeader.tsx

Fix a react warning. Adding key prop to each component in a list.

joneubank

You added a retry parameter to several functions but never specified the number of times they should be retried. Do you want to add an environment variable to the app to configure these, and also set a default retry at 2?

joneubank · 2025-12-16T21:52:42Z

apps/api/src/external/pcglAuthZClient.ts

+
+		if (response.status === 404) {
+			const message = `No user found with email ${emailAddress}.`;
+			logger.info('[AUTHZ]:', `retry: ${retry}`, message);


Should the comment specify "Retries remaining:" instead of "retry:"? As written, the log seems to indicate how many times its already been attempted.

removed retry from this function, and updated the logs

joneubank · 2025-12-16T21:53:56Z

apps/api/src/external/pcglAuthZClient.ts

+export const lookupUserByEmail = async (
+	emailAddress: string,
+	accessToken: string,
+	retry = 0,


Functional and simple retry mechanism. In the future, we should look for a way to make this reusable without having to implement all the retry logging and count tracking and final return cases.

refactored the retry mechanism, now is handled in a single the function used to call authz, also included a generic fetchWithRetry.

leoraba · 2025-12-17T16:55:28Z

apps/api/src/config/serverConfig.ts

+	FETCH_RETRIES: z.coerce.number().optional().default(3),
+	FETCH_RETRY_DELAY_MS: z.coerce.number().optional().default(500),
+	FETCH_TIMEOUT_MS: z.coerce.number().optional().default(10000),
+


new env. variables used to configure retry in fetch funcions

leoraba · 2025-12-17T16:58:23Z

apps/api/src/external/pcglAuthZClient.ts

+	// Retry mechanism for authz requires to refresh service token before retying fetch call
+	const { FETCH_RETRIES, FETCH_RETRY_DELAY_MS } = serverConfig;
+	let attempt = 0;
+	while (true) {
+		try {
+			const response = await _fetchFromAuthZ();


centralized the retry mechanism + refresh Token when calling Authz service.

leoraba · 2025-12-17T17:00:17Z

apps/api/src/external/fetchWithRetry.ts

+ * @param init
+ * @returns
+ */
+export async function fetchWithRetry(url: string, init: RequestInit): Promise<Response> {


generic fetch wrapper with retry options

leoraba · 2025-12-19T19:56:58Z

PR updated based on feedback received.

Note: A separate PR is required to handle permissions to unregistered users.

leoraba added 5 commits December 10, 2025 17:48

assign user to study permission

6e0002c

assign user to study permission

f21592e

dashboard access component

ee9b008

fix issue view signature as dac-chair

a07cdac

remove expire date and more

a64e990

leoraba commented Dec 11, 2025

View reviewed changes

update readme - new env variable

ef9cdd3

leoraba commented Dec 11, 2025

View reviewed changes

fix dac chair not able to see app signatures

9b520bd

leoraba requested review from demariadaniel and joneubank December 11, 2025 19:11

temp studies

2f91fdb

joneubank reviewed Dec 11, 2025

View reviewed changes

leoraba added 4 commits December 15, 2025 10:00

retry authz call

0f1190a

has access status bar

08519bf

unique key prop on list children

b1537c9

handle error adding user permissions

1978a45

leoraba commented Dec 16, 2025

View reviewed changes

leoraba requested a review from joneubank December 16, 2025 21:45

joneubank requested changes Dec 16, 2025

View reviewed changes

leoraba added 2 commits December 17, 2025 10:57

fetch with retry

5a1ddd3

fix - fetch with retry

6a979ae

leoraba commented Dec 17, 2025

View reviewed changes

leoraba requested a review from joneubank December 17, 2025 17:02

leoraba marked this pull request as draft December 18, 2025 21:13

approve appication without registered users

664198f

leoraba force-pushed the feat/509-study-read-permission branch from 99e7e8a to 664198f Compare December 19, 2025 16:47

leoraba added 2 commits December 19, 2025 14:27

Merge branch 'main' into feat/509-study-read-permission

08bd406

approve application params

0199fdf

leoraba marked this pull request as ready for review December 19, 2025 19:57

Merge branch 'main' into feat/509-study-read-permission

39e41d0

		# -- Approved Application --
		APPROVED_PERMISSION_EXPIRES_IN_DAYS=365

509: Add Study Read Permission to Approved User #526

Are you sure you want to change the base?

509: Add Study Read Permission to Approved User #526

Conversation

leoraba commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Details

Ticket

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!

joneubank left a comment

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!

leoraba Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

leoraba Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

joneubank left a comment

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!

leoraba Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leoraba commented Dec 19, 2025

Uh oh!

Reviewers

Assignees

leoraba commented Dec 10, 2025 •

edited

Loading

leoraba Dec 16, 2025 •

edited

Loading

leoraba Dec 16, 2025 •

edited

Loading

leoraba Dec 17, 2025 •

edited

Loading