✨🔧 452 - [Feature branch] - Add ega permissions integration #457

anncatton · 2024-09-17T13:09:56Z

Note: This will be the base branch for work associated with the new EGA permissions flow.

Includes work from:

Adds initial setup for EGA API client.

EGA Job

updates config.ts to include values for EGA APIs
adds axios clients for EGA auth server and API
creates client for interacting with EGA API, including fetching an access token from auth server
adds initial logic for refreshing the access token, using Axios interceptors. Will need to expand this to address multiple concurrent requests!
adds constants for API paths

Documentation

adds a new section for environment variables in the README. For now, just added the vars related to EGA. Will add remaining values in a cleanup ticket, as some of them are likely to be removed (i.e. Vault values)
updates .env.example file

Docker Compose

adds specific version for Vault image, there is no latest image available on dockerhub
changes mailhog image to a forked version to allow building Multi Arch Docker images: https://hub.docker.com/r/jcalonso/mailhog
modifies Makefile scripts to match updated Docker executable

Dependencies

adds axios and types support for jsonwebtoken

Cleanup

unrelated to ticket, moves some utils functions from the router file (applications.ts) to their own file

joneubank

Comments are about code strictness and readability/maintainability. There's nothing very functional happening here to request changes for.

joneubank · 2024-09-24T15:35:01Z

.env.example

-DACO_UI_BASE_URL=https://dac.dev.argo.cancercollaboratory.org
+DACO_UI_BASE_URL=http://localhost:3000


Thank you for removing specific system references.

joneubank · 2024-09-24T15:35:30Z

README.md

+## Environment Variables
+
+| Name                | Description                                                                   | Type     | Required | Default |
+| ------------------- | ----------------------------------------------------------------------------- | -------- | -------- | ------- |
+| EGA_CLIENT_ID       | Client ID for EGA API                                                         | `string` | true     |         |
+| EGA_AUTH_HOST       | Root URL for EGA authentication server                                        | `string` | true     |         |
+| EGA_AUTH_REALM_NAME | Realm name for EGA authentication server                                      | `string` | true     |         |
+| EGA_API_URL         | Root URL for EGA API                                                          | `string` | true     |         |
+| EGA_USERNAME        | Username for account used to gain access token from EGA authentication server | `string` | true     |         |
+| EGA_PASSWORD        | Password for account used to gain access token from EGA authentication server | `string` | true     |         |
+


joneubank · 2024-09-24T15:39:54Z

src/jobs/ega/egaClient.ts

+    },
+  );
+
+  const token = response.data;


data has type any. Are we planning to ensure that the response data matches the type IdpToken?

i'm adding Zod in the next PR, i will add a safeParse call for this and the refreshToken return

joneubank · 2024-09-24T15:40:27Z

src/jobs/ega/egaClient.ts

+    },
+  );
+
+  return response.data;


Repeat comment, data has type any.

Will fix as in above comment

joneubank · 2024-09-24T15:42:00Z

src/jobs/ega/egaClient.ts

+  const getDacs = async () => {
+    const response = await apiAxiosClient.get('/dacs');
+    return response.data;
+  };


getDacs: () => Promise<any> and no comments. Can you add tsdocs to this so I know what this is supposed to be returning?

This will be removed in the next PR!

joneubank · 2024-09-24T15:42:31Z

src/routes/utils.ts

+
+import { BadRequest } from '../utils/errors';
+
+export function validateId(id: string) {


TSDocs please, no idea what this is validating or when to use.

joneubank · 2024-09-24T15:42:43Z

src/routes/utils.ts

+  return id;
+}
+
+export function validateType(type: string) {


TSDocs please, no idea what this is validating or when to use.

joneubank · 2024-09-24T15:44:00Z

src/utils/constants.ts

+export const EGA_API = {
+  DACS: 'dacs',
+  PERMISSIONS: 'permissions',
+  USERS: 'users',
+  MEMBERS: 'members',
+  REQUESTS: 'requests',
+  DATASETS: 'datasets',
+};


Whats the use of this?

these are pathnames for the ega api requests, they'll come into play in the next PR(s). I'll add a comment tho

joneubank · 2024-09-27T20:43:21Z

Before you merge this, you mentioned "Next PR" a couple times, should we be collecting this work into a larger feature branch?

anncatton · 2024-09-30T12:30:11Z

Before you merge this, you mentioned "Next PR" a couple times, should we be collecting this work into a larger feature branch?

My plan was to use this branch as the feature branch - the "next PR" is the work for #453 (#458), which uses this as its base branch.

453 ega api funcs

#461) * add pThrottle source code dir, add throttling to all egaClient functions

… one req

✨ 454 - implement ega job flow

…ag enabling

…fig, readme

🥅 455 - Error handling and retries + job reporting

✨ 454 - Add refresh token logic to Axios client

anncatton added 6 commits September 17, 2024 09:01

Update docker-compose, .env.example files. add axios dep

49b0512

Update config, secrets values. Create egaClient with token fetch

4227b2b

initial refresh token logic

0e06d57

add ega endpoint constants

cfa6d35

add section for env vars in readme

ce6d8a7

remove logs

912e16a

anncatton marked this pull request as ready for review September 23, 2024 12:41

remove public key reference

83be068

anncatton requested a review from joneubank September 23, 2024 13:02

anncatton added the ega-integration label Sep 24, 2024

anncatton changed the title ~~✨🔧 452 - Add ega client~~ ✨🔧 452 - Add ega permissions integration Sep 24, 2024

anncatton added 2 commits September 24, 2024 10:37

Add zod, update ts. Add expiry to approved app list data

aa42318

Add api response types, permissions and users get funcs

870ba74

joneubank requested changes Sep 24, 2024

View reviewed changes

anncatton added 2 commits September 25, 2024 08:53

Add safe parsing for api list results. Add dacId to config

6162104

add tsdocs

3570e4a

anncatton requested a review from joneubank September 25, 2024 13:46

anncatton added 4 commits September 25, 2024 13:57

Add safeParse checks to ega client calls.

76a740e

Add failure returns to ega client calls, restructure types

f62d34b

remove empty file

14d35f4

move failure types to separate file, add appId to approved users data

12bb890

anncatton mentioned this pull request Sep 26, 2024

453 ega api funcs #458

Merged

modify list response types

e995c79

joneubank approved these changes Sep 27, 2024

View reviewed changes

anncatton added the feature-branch label Sep 30, 2024

anncatton changed the title ~~✨🔧 452 - Add ega permissions integration~~ ✨🔧 452 - [Feature branch] - Add ega permissions integration Sep 30, 2024

anncatton added 2 commits September 30, 2024 10:21

Create ega permissions main job function, add happy path flow

965f0bb

move safeParseArray to own file, update imports

e866b2b

anncatton added 21 commits September 30, 2024 10:29

Merge pull request #458 from icgc-argo/453-ega-api-funcs

ab2e9bc

453 ega api funcs

Merge branch '452-ega-auth-client' into 454-implement-job-flow

cacf7cb

WIP - fix for pagination when checking permissions

ef48443

Expand tsdocs

04879b4

add comment for api bug, reorg ega services

951bebc

move custom errors to types dir

f4be3e8

➕ Add pThrottle dependency as source code, rate limit ega client funcs (

816df19

#461) * add pThrottle source code dir, add throttling to all egaClient functions

rename types + util functions for clarity

9476767

remove unneeded fields from EgaPermissionRequest response schema

81d23b6

Add refresh token logic for 401 errors. Fetch all user permissions in…

5bbda86

… one req

Merge pull request #460 from icgc-argo/454-implement-job-flow

db89e47

✨ 454 - implement ega job flow

Add ega public key fetch to secrets

e92dab7

Split clients into separate files. Fix error handling order

b25cca9

Adds ega job report data. Adds job to main batch job, with feature fl…

9cb3949

…ag enabling

add newline, cleanup

041962a

add axios-retry to egaClient

d7e93d1

improve ega report structure, add example report to tsdoc

4d3a9d1

remove unnecessary comments

7880d28

reject promise when max retry limit exceeded, add retry limits to con…

5a1d79b

…fig, readme

Merge pull request #463 from icgc-argo/455-implement-error-retry

afbb2c0

🥅 455 - Error handling and retries + job reporting

Merge pull request #462 from icgc-argo/add-refresh-token-flow

a227b1e

✨ 454 - Add refresh token logic to Axios client

anncatton merged commit f23c7dc into develop Nov 27, 2024
2 checks passed

anncatton deleted the 452-ega-auth-client branch November 27, 2024 19:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨🔧 452 - [Feature branch] - Add ega permissions integration #457

✨🔧 452 - [Feature branch] - Add ega permissions integration #457

anncatton commented Sep 17, 2024 •

edited

Loading

joneubank left a comment

joneubank Sep 24, 2024

joneubank Sep 24, 2024

joneubank Sep 24, 2024

anncatton Sep 25, 2024

joneubank Sep 24, 2024

anncatton Sep 25, 2024

joneubank Sep 24, 2024

anncatton Sep 25, 2024

joneubank Sep 24, 2024

joneubank Sep 24, 2024

joneubank Sep 24, 2024

anncatton Sep 24, 2024

joneubank commented Sep 27, 2024

anncatton commented Sep 30, 2024 •

edited

Loading

		DACO_UI_BASE_URL=https://dac.dev.argo.cancercollaboratory.org
		DACO_UI_BASE_URL=http://localhost:3000


		import { BadRequest } from '../utils/errors';

		export function validateId(id: string) {

✨🔧 452 - [Feature branch] - Add ega permissions integration #457

✨🔧 452 - [Feature branch] - Add ega permissions integration #457

Conversation

anncatton commented Sep 17, 2024 • edited Loading

EGA Job

Documentation

Docker Compose

Dependencies

Cleanup

joneubank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joneubank commented Sep 27, 2024

anncatton commented Sep 30, 2024 • edited Loading

anncatton commented Sep 17, 2024 •

edited

Loading

anncatton commented Sep 30, 2024 •

edited

Loading