feat: collaborators #485

prestonlimlianjie · 2022-08-11T06:14:12Z

Problem

This PR adds the collaborators feature as described in https://www.notion.so/opengov/Collaborators-65bfdc2267e044bfb19996ce9e7174f9

Solution

Features:

Key changes:

Add authorization middleware methods to allow routers to gate access to either site member or site admins [link]
New collaborator API endpoints [link]
Database migration to change role enum values to ADMIN and CONTRIBUTOR [link]
Tests for the abovementioned routes and services

Bug Fixes:

Fixed some typos/errors in the database models - 787a70e

Tests

Unit tests

Run npm run test

Smoke tests

Prepare database

Add the following records to the Whitelist table

1	.gov.sg	NULL	2022-07-29 02:49:31.464881+00	2022-07-29 02:49:31.464881+00
2	test.sg	2023-01-01 00:00:00+00	2022-08-02 05:20:57.738498+00	2022-08-02 05:20:57.738498+00

Add the following records to the Users table

1	[email protected]	test	91231234	2022-08-11 03:54:56.394+00	2022-04-04 07:25:41.013+00	2022-08-11 03:54:56.396+00	NULL
2	[email protected]	test	91231235	2022-04-30 07:41:09.661+00	2022-04-04 07:25:41.013+00	2022-07-30 07:41:09.662+00	NULL
3	[email protected]	testing	91231236	2022-04-29 07:41:09.661+00	2022-04-04 07:25:41.013+00	2022-07-30 07:41:09.662+00	NULL
4	[email protected]	testtest	91231237	2022-01-30 07:41:09.661+00	2022-04-04 07:25:41.013+00	2022-07-30 07:41:09.662+00	NULL

Add the following records to the Sites table

1	a-test-v4	null	2022-08-10 10:46:02.874209+00	2022-08-10 10:46:02.874209+00	INITIALIZED	READY	1	NULL

Add the following records to the SiteMembers table

1	1	ADMIN	2022-08-10 10:46:49.845261+00	2022-08-10 10:46:49.845261+00
2	1	ADMIN	2022-08-10 15:48:50.491+00	2022-08-10 15:48:50.491+00
3	1	ADMIN	2022-08-10 15:48:53.764+00	2022-08-10 15:48:53.764+00
4	1	CONTRIBUTOR	2022-08-11 06:06:44.013+00	2022-08-11 06:06:44.013+00

Run frontend

Check out the feat/collaborators branch on the CMS frontend
Run npm run dev
Log in via the email option
Go to [a-test-v4 dashboard](http://localhost:3000/sites/a-test-v4/dashboard)

Deploy Notes

Migrations

New database migration script to change role enum 20220811070630-change-role-enum.js

New dev dependencies:

@types/lodash

seaerchin · 2022-08-15T09:01:56Z

i think tihs PR needs rebasing! feel free to ping me on slack/re-add me as a reviewer so i don't miss this when done!

dcshzj

Some preliminary comments! Will continue looking through tomorrow.

src/fixtures/identity.ts

src/routes/v2/authenticated/index.js

src/services/identity/CollaboratorsService.ts

src/services/middlewareServices/AuthorizationMiddlewareService.ts

src/routes/v1/authenticatedSites/index.js

src/routes/v2/authenticatedSites/index.js

dcshzj · 2022-08-31T06:30:00Z

src/services/identity/CollaboratorsService.ts

+      return new BadRequestError(
+        "That doesn't look like a valid email. Try a gov.sg or other whitelisted email."
+      )


Should we throw the error directly here instead of letting it be handled at the router level?

My thought here was that we should move towards a neverthrow-like pattern (but I didn't want to introduce neverthrow into this specific PR just yet - we should do it in a separate PR to prevent scope creep).

In this pattern, we want to ensure that library methods don't throw errors themselves as far as possible. Instead, we want to get them to return errors of specific types so that their callers can figure out what to do with them.

In this PR, because we're not introducing neverthrow yet:

the business-logic/expected errors (e.g. site not found) will be returned as a specific error

the unexpected errors (e.g. database-related errors) will be thrown

In a separate PR, when we introduce neverthrow, errors in both categories will be returned as specific errors.

tagging @seaerchin since he is familiar with neverthrow from his work in FormSG - thoughts?

dcshzj · 2022-08-31T06:36:42Z

src/routes/v2/authenticated/index.js

+  authenticatedSubrouter.use(
+    "/sites/:siteName/collaborators",
+    collaboratorsRouter.getRouter()
+  )


Should this router be part of authenticatedSites instead?

The way I saw it was that authenticatedSites was responsible for site content, i.e. what the GitHub repo handles today, while authenticatedSites was responsible for site-related meta features (e.g. collaborators, review requests, notifications).

But this is definitely up for discussion. Tagging @alexanderleegs and @seaerchin for their thoughts since they are working on RR+Notifications at the moment

I think we haven't really made a decision on this - currently the authenticatedSubrouter and authenticatedSitesSubrouter are split as such because of the params they require. I think either way works, though I'm leaning towards reusing the authenticatedSites router to make use of the related subrouters which go together with it, to prevent having to set up siteName retrieval and other site specific operations again.

src/services/identity/CollaboratorsService.ts

src/routes/v2/authenticated/collaborators.ts

Co-authored-by: Hsu Zhong Jun <[email protected]>

prestonlimlianjie · 2022-10-03T18:21:57Z

Closing because it is now too painful to rebase. Redid the PR here #510

prestonlimlianjie requested review from alexanderleegs, seaerchin and kishore03109 August 11, 2022 06:14

prestonlimlianjie mentioned this pull request Aug 11, 2022

feat: collaborators isomerpages/isomercms-frontend#1007

Merged

alexanderleegs force-pushed the feat/email-login-flow branch from 8b13405 to a30c37d Compare August 12, 2022 03:40

alexanderleegs force-pushed the feat/email-login-flow branch from a30c37d to eec8788 Compare August 19, 2022 06:17

alexanderleegs and others added 16 commits August 24, 2022 13:49

Chore: migrate authmiddlewareservice to typescript

4592234

Feat: add site retrieval for email and admin users

705d2d8

build(devdeps): add lodash types

d94183a

fix(model): rectify db model definitions

00ec691

refactor: add message param to ForbiddenError

7528b75

fix(SitesService): fix type change from database model

0bdb767

feat: add CollaboratorsService

3262749

test: add tests for CollaboratorsService

f2a1ae2

feat: use CollaboratorService in authorization middleware

542d01f

test: add tests for authorization middleware

a8e385c

feat: add CollaboratorsRouter

e1b4889

test: add tests for CollaboratorsRouter

eea564b

feat(db-migration): change site_members role enum in the database

739f0bb

fix(models): undo erroneous changes

4f68c34

fix: model field and failing tests

4b700f0

fix: clean up painful rebase

bd377a0

prestonlimlianjie force-pushed the feat/collaborators branch from 9787348 to bd377a0 Compare August 24, 2022 06:50

prestonlimlianjie requested review from alexanderleegs, seaerchin and dcshzj and removed request for alexanderleegs, kishore03109 and seaerchin August 24, 2022 06:51

prestonlimlianjie requested a review from kishore03109 August 24, 2022 06:52

dcshzj requested changes Aug 30, 2022

View reviewed changes

dcshzj reviewed Aug 31, 2022

View reviewed changes

alexanderleegs reviewed Aug 31, 2022

View reviewed changes

src/routes/v2/authenticated/collaborators.ts Show resolved Hide resolved

prestonlimlianjie and others added 5 commits September 22, 2022 10:34

chore: rename comment for clarity

a41412d

Co-authored-by: Hsu Zhong Jun <[email protected]>

chore: remove unnecessary imports

ea4e726

style: rename var to be more boolean-like

92575fc

style: define let vars as const

b1b6a00

fix: use correct authZ method

cef758b

prestonlimlianjie requested a review from dcshzj September 22, 2022 04:17

refactor: authZ middleware methods

63bdf5d

prestonlimlianjie force-pushed the feat/collaborators branch from c0d1d45 to 63bdf5d Compare September 22, 2022 04:45

prestonlimlianjie added 2 commits September 23, 2022 12:52

chore: add types for collaborator mocks

22b28a7

docs: add comment for deletion check

bd1649e

prestonlimlianjie changed the base branch from feat/email-login-flow to feat/identity-phase-2 October 3, 2022 03:04

prestonlimlianjie changed the base branch from feat/identity-phase-2 to feat/email-login-flow October 3, 2022 16:10

prestonlimlianjie mentioned this pull request Oct 3, 2022

feat: collaborators #510

Merged

prestonlimlianjie closed this Oct 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: collaborators #485

feat: collaborators #485

prestonlimlianjie commented Aug 11, 2022

seaerchin commented Aug 15, 2022

dcshzj left a comment

dcshzj Aug 31, 2022

prestonlimlianjie Sep 22, 2022 •

edited

Loading

dcshzj Aug 31, 2022

prestonlimlianjie Sep 22, 2022

alexanderleegs Sep 26, 2022

prestonlimlianjie commented Oct 3, 2022

feat: collaborators #485

feat: collaborators #485

Conversation

prestonlimlianjie commented Aug 11, 2022

Problem

Solution

Tests

Unit tests

Smoke tests

Deploy Notes

seaerchin commented Aug 15, 2022

dcshzj left a comment

Choose a reason for hiding this comment

dcshzj Aug 31, 2022

Choose a reason for hiding this comment

prestonlimlianjie Sep 22, 2022 • edited Loading

Choose a reason for hiding this comment

dcshzj Aug 31, 2022

Choose a reason for hiding this comment

prestonlimlianjie Sep 22, 2022

Choose a reason for hiding this comment

alexanderleegs Sep 26, 2022

Choose a reason for hiding this comment

prestonlimlianjie commented Oct 3, 2022

prestonlimlianjie Sep 22, 2022 •

edited

Loading