Add reef admin relationships #128

avalmas-programize · 2020-09-04T13:34:15Z

Add reef-to-admin table (custom intermediate table for manyToMany relationship between admin and reef)
Change reef \ admin relationship from manyToOne (users can have many reefs but a reef has only one admin) to manyToMany (reefs now can have more than one admin)
Refactor reef endpoints to follow the manyToMany relationship between user and reef
Add IsReefAdminGuard to restrict access to reef_managers to only their reefs
Refactor surveys endpoints:
- Change survey's global prefix to reefs/:reef_id/surveys so that we always track the reef_id, which is needed for IsReefAdminGuard
- Change GET /surveys/reefs/:id to /reefs/:reef_id/surveys
- Make necessary changes in the DTOs and survey service

Some front-end refactor is needed due to the prefix change

Related issue: #111

packages/api/src/users/users.service.ts

packages/api/src/reefs/reef-to-admin.entity.ts

ericboucher · 2020-09-04T14:17:24Z

@avalmas-programize can you list the necessary frontend changes? Or even have a stab at them? They shouldn't be too complicated at the moment.

packages/api/migration/1599209835916-AddReefToAdminTable.ts

ericboucher · 2020-09-04T14:22:18Z

packages/api/src/reefs/reefs.controller.ts

@@ -44,17 +43,6 @@ export class ReefsController {
    return this.reefsService.findDailyData(id);
  }

-  @Get(':id/surveys/:poi')


is this not needed anymore?

As I see it, this endpoint was using a mock response which is already implemented in the survey details endpoint (GET /reefs/:reef_id/surveys/:id). The only difference was that I fetch all POIs of a survey and this was designed to fetch only one POI of a survey. I don't see any use for it anymore.
Also it was conflicting with the new endpoints (surveys endpoints with the updated prefix)

avalmas-programize · 2020-09-04T14:50:33Z

Front-end changes:
All surveys/ endpoints URLs should changes:

POST /surveys/ => POST /reefs/:reef_id/surveys/
POST /surveys/:survey_id/media/ => POST /reefs/:reef_id/surveys/:survey_id/media/
POST /surveys/upload/ => POST /reefs/:reef_id/surveys/upload/
GET /surveys/reefs/:reef_id => GET /reefs/:reef_id/surveys/
GET /surveys/:survey_id/ => GET /reefs/:reef_id/surveys/:survey_id/
GET /surveys/:survey_id/media/ => GET /reefs/:reef_id/surveys/:survey_id/media/
PUT /surveys/:survey_id/ => PUT /reefs/:reef_id/surveys/:survey_id/
PUT /surveys/media/:media_id/ => PUT /reefs/:reef_id/surveys/:survey_id/
DELETE /surveys/:survey_id/ => PUT /reefs/:reef_id/surveys/:survey_id/
DELETE /surveys/media/:media_id/ => PUT /reefs/:reef_id/surveys/:survey_id/

bencpeters

Some suggested clean-ups throughout, but the big thing is re-doing the schema & entities a bit...

bencpeters · 2020-09-04T21:51:06Z

packages/api/src/auth/is-reef-admin.guard.ts

+      return !!isReefAdmin;
+    }
+
+    return true;


Do we really want to return true in this case? The only way to reach this point is if reefId is null/undefined, which (probably) means the request was malformed. It seems to me like a safer course of action in that case is to return false...

Hmm I thought it from a different angle, but I see your point. I will return false by default.

bencpeters · 2020-09-04T21:51:50Z

packages/api/src/reefs/reef-to-admin.entity.ts

+  ManyToOne,
+  Unique,
+} from 'typeorm';
+// eslint-disable-next-line import/no-cycle


Don't disable the linter here, just import the type:

import type { Reef } from './reefs.entity';

I removed all the lint disables from all the entities.
However the relations decorator expects a entity returned to use as a type for the relation.
To bypass this I had to change the way we declare a relationship from:

@ManyToOne(() => Reef, (reef) => reef.reefToAdmin, { cascade: true }) reef: Reef;

to

@ManyToOne<Reef>('Reef', (reef) => reef.reefToAdmin, { cascade: true }) reef: Reef;

But this does not solve all of the issues. After compiling the code an error appears in the dist files:

/home/antonios/Documents/Programize/aqualink-app-backend/packages/api/dist/src/reefs/reef-to-admin.entity.js __metadata("design:type", User) ReferenceError: User is not defined

To solve this we have to create an interface for each entity and reference this as the type of the class member
typeorm/typeorm#4190
Are we okay with this refactor?

Ah, I see the issue, we need to actually use the types as a return value, not just a type annotation.

New plan: Since circular dependencies are pretty much baked into how TypeORM does many-to-many relations, let's add a new ESlint rule override to disable that check in *.entity.ts files only.

bencpeters · 2020-09-04T22:03:21Z

packages/api/src/reefs/reef-to-admin.entity.ts

+
+@Entity()
+@Unique('unique-relationship', ['reefId', 'adminId'])
+export class ReefToAdmin {


You shouldn't need to define a TypeORM entity at all for a join table unless you're trying to store custom information on the join table (e.g. an attribute that describes a condition on the join). In this case you're just trying to set up a many-to-many join between two entities, so defining it in this way has numerous disadvantages:

This is a class based entity that TypeORM will actually try to instantiate if you load records in some cases, increasing the overhead.

You have a completely extraneous extra column (reefToAdminId) that conveys no extra information and requires a hacky unique constraint (the "right" way to set up a table like this would just use the reefId, adminId as the primary key.

This entity forces you to specify a naming convention (which in this case you didn't use the "standard" SQL naming convention for join tables across most web frameworks which would be reefs_admins) rather than relying on predictable & standard defaults.

See the docs on Many-To-Many in TypeORM.

This is what you want to do:

class Reef { // Other attributes @ManyToMany(() => User, (user) => user.administered_reefs) @JoinTable() admins: User[] } class User { // Other attributes @ManyToMany(() => Reef, (reef) => reef.admins) administered_reefs: Reef[] }

I wanted to add a custom join table in case we need it in the future.
I will revert my change

bencpeters · 2020-09-04T22:04:50Z

packages/api/src/auth/is-reef-admin.guard.ts

+    }
+
+    if (reefId) {
+      const isReefAdmin = await this.reefToAdminRepository.findOne({


We have an adminLevel that corresponds to a reef manager, do we want to enforce that that admin level is set as well?

The routes this guard is added are the already protected ones, i.e the ones that do not allow the Default user to access them, only ReefManager and SuperAdmin.
In the first lines of this guard I bypass the guard if the level of the user is SuperAdmin.
So in this we are left with the case that the user is a ReefManager.
Also if in the future this is added to a not protected route, i.e a default user is tested to pass this check they will fail because the are not a manager of the reef, which makes perfect sense.
We should not allow a default user access more than a reef manager.

That may be true in the current use-case - wherever we use this is already a protected route, but it would be fairly easy down the line for someone not to realize that. That's why I was suggesting adding an adminLevel check inside this guard so that you don't have to ensure that a completely independent guard is also set whenever you use it.

bencpeters · 2020-09-04T22:05:28Z

packages/api/src/reefs/reefs.entity.ts

 // eslint-disable-next-line import/no-cycle
 import { DailyData } from './daily-data.entity';
 import { VideoStream } from './video-streams.entity';
 // eslint-disable-next-line import/no-cycle
 import { Survey } from '../surveys/surveys.entity';
+// eslint-disable-next-line import/no-cycle
+import { ReefToAdmin } from './reef-to-admin.entity';


import type { ReefToAdmin } (the other no cycle imports should use this as well - get rid of all the linter disable comments)

Like I said this is more complicated than it seems.

bencpeters · 2020-09-04T22:11:17Z

packages/api/src/surveys/surveys.controller.ts

 export class SurveysController {
  constructor(private surveyService: SurveysService) {}

+  @UseGuards(IsReefAdminGuard)


Rather than requiring us to specify this guard on every single handler, it might be worth adding a @Public decorator that does setMetadata("isPublic", true) and then look up the metadata on the handler inside the isReefAdminGuard function and only lets the request through without the check if that metadata is set. Then you could specify the guard on a controller-wise level here.

Interesting approach.
However I would name the decorator something more specific like @ManagersOnly because you would assume a @Public decorator to also have an effect to the @Auth decorator.

Hmmm. IsReefAdminGuard needs the Auth decorator to run first and since the first one is now a Controller decorator it will always run first. So we need a different approach if we want to implement this.
We could edit the Auth decorator in order to dynamically add the IsReefAdminGuard to the list of decorators used or have the new decorator (the Public one you suggested) affect both guards' behavior and add those as global or controller decorators

have the new decorator (the Public one you suggested) affect both guards' behavior and add those as global or controller decorators

This is what I was imagining. If you move IsReefAdminGuard to the controller level, you definitely have to move the Auth guard as well.

bencpeters · 2020-09-04T22:11:35Z

packages/api/src/surveys/surveys.entity.ts

@@ -10,6 +10,7 @@ import {
 } from 'typeorm';
 // eslint-disable-next-line import/no-cycle
 import { Reef } from '../reefs/reefs.entity';
+// eslint-disable-next-line import/no-cycle


again, use import type { ... }

Like I said this is more complicated than it seems.

bencpeters · 2020-09-04T22:12:13Z

packages/api/src/users/users.entity.ts

 } from 'typeorm';
 import { Exclude } from 'class-transformer';
+// eslint-disable-next-line import/no-cycle
+import { ReefToAdmin } from '../reefs/reef-to-admin.entity';


another import type { }

Like I said this is more complicated than it seems.

bencpeters · 2020-09-04T22:14:23Z

packages/api/src/users/users.service.ts

+
+        // If relationship already exists, skip
+        if (relationshipExists) {
+          return Promise.resolve();


This is already an sync function, you don't need to return anything at all to get it to wrap in a Promise (or you could return true if that feels better)

It was not letting me to just write return; because map expects something returned so I thought I could return an empty Promise since that's what I want.
But I will change it to null since I agree it is a bit ugly.

bencpeters

Getting close!

A few small cleanups, then:

for the cyclic dependencies, let's just leave them in the entity code, but add an eslint rule override for *.entity.ts files for that specific rule so that we don't need line-by-line overrides.
For the @Public decorator - I think you'd need to have it affect the auth guard too, then it should work. I don't feel super strongly about it either way, but I generally think that it might be a cleaner way to define a controller where almost all the routes are protected but there are 1 or 2 public ones... if it's taking too long don't worry about it though.

bencpeters · 2020-09-08T16:39:00Z

packages/api/src/users/users.controller.ts

+  @Get('current/administrated-reefs')
+  getAdministratedReefs(@Req() req: AuthRequest): Promise<Reef[]> {
+    return this.usersService.getAdministratedReefs(req);


Let's call these "administered" (not "administrated") (ref)

Oops typo. Fixed

bencpeters · 2020-09-08T16:42:07Z

packages/api/src/users/users.service.ts

+        // Add new user as admin
+        return this.reefRepository.update(
+          {
+            id: reef.id,
+          },
+          {
+            admins: [...reef.admins, user],
+          },
+        );


Can we add this relation from the "other side" (e.g. using the userRepository) instead? I think this is the only dependency the UserService has on the reefRepository, so if we could add it from the "other" side of the relation we could drop that dependency from this service...

this.usersRepository.update({ id: user.id }, { administeredReefs: ... })

Maybe?

bencpeters · 2020-09-08T16:44:24Z

packages/api/src/users/users.service.ts

+      relations: ['reef', 'reef.admins'],
+    });
+
+    const reefEntities: Promise<UpdateResult | null>[] = reefAssociations.map(


Implicitly, a user who has user reef admin associations should also get the reef manager role, no?

Yeah I thought it would have been set from the application. Done.

avalmas-programize · 2020-09-10T10:29:49Z

I have added the Public decorator. Implementation:

Override method AuthGuard('custom').canActivate so that it will not apply the strategy if isPublic equals to true
Override Levels and ReefAdmin guards to return true if isPublic equals to true
Add OverrideLevelAccess decorator which overrides the level defined in the Auth decorator on the Controller in order to have different level of access per Controller
Refactor Auth decorator in order for the above change to work without re-applying extra decorators

Change controller's decorators structure Add public decorator and override behavior for both auth and is-reef-admin decorators Add override-level-access to override the default decorator access for some endpoints

ericboucher · 2020-09-11T03:49:14Z

When I tried it, I did not get promoted to "reef_manager" in the DB for some reason.

But I am actually wondering if this is still useful, if a user has a reef - user row, we can probably assume that they are an admin this way. And just keep default / super_admin

Thoughts @avalmas-programize @bencpeters

avalmas-programize · 2020-09-11T08:15:54Z

No I think keeping all three roles is much useful and easier to maintain.
Also there was a bug regarding the promotion of the user to reef_manager.
Saves after the update operation for the user promotion were reverting the level of the user to their prior one.
Added a new commit that resolves this.

ericboucher · 2020-09-11T08:57:35Z

packages/api/src/users/users.service.ts

+      id: user.id,
+      administeredReefs: newAdministeredReefs,
+    };
+    return this.usersRepository.save(newUser);


if we pass in an id, should we use .update instead of save?

Updating ManyToMany relations is not working properly with update, only with save.
typeorm/typeorm#2821

ericboucher · 2020-09-11T09:00:12Z

packages/api/src/users/users.service.ts

+      // eslint-disable-next-line fp/no-mutation
+      priorAccount.adminLevel = newUser.adminLevel;
+    }
+
    const data = {
      ...priorAccount,


should we reverse the order here so that priorAccount info is prioritized over the createUserDto?

Do we want to do that?
I mean the createUserDto has the information added from the user in the registration form.

outdated

ericboucher reviewed Sep 4, 2020

View reviewed changes

packages/api/src/users/users.service.ts Outdated Show resolved Hide resolved

ericboucher reviewed Sep 4, 2020

View reviewed changes

packages/api/src/users/users.service.ts Outdated Show resolved Hide resolved

ericboucher reviewed Sep 4, 2020

View reviewed changes

packages/api/src/reefs/reef-to-admin.entity.ts Outdated Show resolved Hide resolved

ericboucher reviewed Sep 4, 2020

View reviewed changes

packages/api/migration/1599209835916-AddReefToAdminTable.ts Outdated Show resolved Hide resolved

ericboucher reviewed Sep 4, 2020

View reviewed changes

ericboucher linked an issue Sep 4, 2020 that may be closed by this pull request

Detect new users as reef admins #111

Closed

bencpeters requested changes Sep 4, 2020

View reviewed changes

avalmas-programize force-pushed the reef-admins branch 2 times, most recently from 83d2797 to 9ccda33 Compare September 8, 2020 10:36

avalmas-programize added 5 commits September 8, 2020 19:05

Add ManyToMany reef-user relationship

dc62aa0

Refactor reef service

7048a89

Add IsReefManagerGuard

6d1b9bf

Migrate reef admin relatioships

4dc7b83

Add endpoint to fetch administrated reefs of user

a29e271

avalmas-programize force-pushed the reef-admins branch from 427730a to a29e271 Compare September 8, 2020 16:08

bencpeters previously requested changes Sep 8, 2020

View reviewed changes

ericboucher and others added 3 commits September 10, 2020 00:11

Merge branch 'master' into reef-admins

72de005

Override cycle dependencies for eneity files

7469749

Fix typo

7b74822

avalmas-programize force-pushed the reef-admins branch from f2fdf7f to 7b74822 Compare September 10, 2020 06:54

Make users the joinTable for reef ManyToMany relationship

28deadb

Add public decorator

c33281b

Change controller's decorators structure Add public decorator and override behavior for both auth and is-reef-admin decorators Add override-level-access to override the default decorator access for some endpoints

avalmas-programize force-pushed the reef-admins branch from b210757 to c33281b Compare September 10, 2020 10:43

ericboucher added 2 commits September 10, 2020 15:18

Merge branch 'master' into reef-admins

7500856

Merge branch 'master' into reef-admins

725cb58

ericboucher requested a review from bencpeters September 10, 2020 17:19

Override user's role

9f1eecc

Update users.controller.ts

37600df

ericboucher approved these changes Sep 11, 2020

View reviewed changes

ericboucher reviewed Sep 11, 2020

View reviewed changes

Fix reef_manager elevation

2b0cc33

ericboucher changed the title ~~Reef admins~~ Add reef admin relationships Sep 11, 2020

ericboucher merged commit cbea1c4 into master Sep 11, 2020

ericboucher deleted the reef-admins branch September 11, 2020 10:08

bencpeters mentioned this pull request Sep 11, 2020

Clean up user reef admin migration #192

Merged

Add reef admin relationships #128

Add reef admin relationships #128

Conversation

avalmas-programize commented Sep 4, 2020 • edited Loading

ericboucher commented Sep 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avalmas-programize commented Sep 4, 2020

bencpeters 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avalmas-programize Sep 8, 2020 • edited Loading

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

bencpeters 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

avalmas-programize commented Sep 10, 2020

ericboucher commented Sep 11, 2020

avalmas-programize commented Sep 11, 2020

Choose a reason for hiding this comment

avalmas-programize Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avalmas-programize commented Sep 4, 2020 •

edited

Loading

avalmas-programize Sep 8, 2020 •

edited

Loading

avalmas-programize Sep 11, 2020 •

edited

Loading