Merge pull request #120 from codeforjapan/yash/admin-acl-p2

ACL - PlantUML diagrams & Admin Delete

docs/acl.pu

@@ -0,0 +1,66 @@
+@startuml
+
+title ACL
+
+== ACL on LIST endpoints ==
+autonumber 1
+
+actor AdminUser
+AdminUser  -> "NestJS Guard": GET /admins/organizations
+note over "NestJS Guard" 
+Expects JWT Payload:
+- isAdminUser 
+- userAdminRole 
+- userAccessKey
+end note
+
+"NestJS Guard" -> "Service":  Pass userAccessKey to organization service
+note left "Service" 
+Example value: 
+- userAccessKey: SUPER_ADMIN_KEY
+end note
+
+"Service" -> "Firestore": Fetch all accessible documents \n in organizations collection
+note over "Firestore" 
+.where(
+  accessControlList, 
+  array-contains, 
+  userAccessKey
+)
+end note
+
+"Firestore" -> "Service": Return array of organizations
+
+"Service" -> "AdminUser": Organizations LIST based on ACL
+
+
+== ACL on GET by ID endpoints ==
+autonumber 1
+
+AdminUser  -> "NestJS Guard": GET /admins/organizations/ABC
+note over "NestJS Guard" 
+Expects JWT Payload:
+- isAdminUser 
+- userAdminRole 
+- userAccessKey
+end note
+
+"NestJS Guard" -> "Service":  Pass userAccessKey to organization service
+note left "Service" 
+Example value: 
+- userAccessKey: SUPER_ADMIN_KEY
+end note
+
+"Service" -> "Firestore": Fetch single document \n based on ID ABC
+
+"Firestore" -> "Service": Return ABC organization
+note over "Service" 
+canUserAccessResource(
+  userAccessKey, 
+  organization
+)
+end note
+
+"Service" -> "AdminUser": Organizations GET by ID based on ACL
+
+@enduml

src/admins/admins.controller.ts

@@ -9,6 +9,8 @@ import {
   ValidationPipe,
   Request,
   HttpCode,
+  Param,
+  Delete,
 } from '@nestjs/common'
 import {
   ApiOperation,
@@ -59,6 +61,27 @@ export class AdminsController {
   ): Promise<NoResponseBody> {
     const requestAdminUser: RequestAdminUser = req.user
     await this.adminsService.createOneAdminUser(requestAdminUser, createAdminRequest)
+
+    return {}
+  }
+
+  @ApiOperation({ summary: 'Get admin by id' })
+  @ApiOkResponse({ type: Admin })
+  @Get('/users/:userId')
+  async getAdminById(@Request() req, @Param('userId') userId: string): Promise<Admin> {
+    const requestAdminUser: RequestAdminUser = req.user
+    const admin = await this.adminsService.getOneAdminById(requestAdminUser, userId)
+
+    return admin
+  }
+
+  @ApiOperation({ summary: 'Get admin by id' })
+  @ApiOkResponse({ type: Admin })
+  @Delete('/users/:userId')
+  async deleteAdminById(@Request() req, @Param('userId') userId: string): Promise<NoResponseBody> {
+    const requestAdminUser: RequestAdminUser = req.user
+    await this.adminsService.deleteOneAdminById(requestAdminUser, userId)
+
     return {}
   }
 }

src/admins/admins.service.spec.ts

@@ -3,23 +3,33 @@ import { AdminsService } from './admins.service'
 import { AdminsRepository } from './admins.repository'
 import { OrganizationsService } from '../organizations/organizations.service'
 import { PrefecturesService } from '../prefectures/prefectures.service'
+import { FirebaseService } from '../shared/firebase/firebase.service'
 
 describe('AdminsService', () => {
   let service: AdminsService
   const adminsRepository = { findAll: () => ['test'] }
   const organizationsService = {}
   const prefecturesService = {}
+  const firebaseService = {}
 
   beforeEach(async () => {
     const module: TestingModule = await Test.createTestingModule({
-      providers: [AdminsService, AdminsRepository, OrganizationsService, PrefecturesService],
+      providers: [
+        AdminsService,
+        AdminsRepository,
+        OrganizationsService,
+        PrefecturesService,
+        FirebaseService,
+      ],
     })
       .overrideProvider(AdminsRepository)
       .useValue(adminsRepository)
       .overrideProvider(OrganizationsService)
       .useValue(organizationsService)
       .overrideProvider(PrefecturesService)
       .useValue(prefecturesService)
+      .overrideProvider(FirebaseService)
+      .useValue(firebaseService)
       .compile()
 
     service = module.get<AdminsService>(AdminsService)

src/admins/admins.service.ts

@@ -3,6 +3,7 @@ import {
   BadRequestException,
   ConflictException,
   UnauthorizedException,
+  NotFoundException,
 } from '@nestjs/common'
 import { AdminsRepository } from './admins.repository'
 import { CreateAdminDto, CreateAdminRequestDto } from './dto/create-admin.dto'
@@ -18,17 +19,20 @@ import {
   canUserCreatePrefectureAdmin,
   getPrefectureAdminACLKey,
   getNationalAdminACLKey,
+  canUserAccessResource,
 } from '../shared/acl'
 import { RequestAdminUser } from '../shared/interfaces'
 import { OrganizationsService } from '../organizations/organizations.service'
 import { PrefecturesService } from '../prefectures/prefectures.service'
+import { FirebaseService } from '../shared/firebase/firebase.service'
 
 @Injectable()
 export class AdminsService {
   constructor(
     private adminsRepository: AdminsRepository,
     private organizationsService: OrganizationsService,
-    private prefecturesService: PrefecturesService
+    private prefecturesService: PrefecturesService,
+    private firebaseService: FirebaseService
   ) {}
 
   async createOneAdminUser(
@@ -138,17 +142,46 @@ export class AdminsService {
     return this.adminsRepository.createOne(createAdminDto)
   }
 
-  async findOneAdminById(adminId: string): Promise<Admin | undefined> {
+  async getOneAdminById(
+    requestAdminUser: RequestAdminUser,
+    organizationId: string
+  ): Promise<Admin> {
+    // Fetch resource and perform ACL check.
+    const admin = await this.adminsRepository.findOneById(organizationId)
+    if (!admin) {
+      throw new NotFoundException('Could not find admin with this id')
+    }
+    if (!canUserAccessResource(requestAdminUser.userAccessKey, admin)) {
+      throw new UnauthorizedException('User does not have access on this resource')
+    }
+
+    return admin
+  }
+
+  /**
+   * Fetches one admin by adminId.
+   * Internal functions do not perform any ACL checks and should be used carefully.
+   * @param adminId: string
+   */
+  async findOneAdminByIdInternal(adminId: string): Promise<Admin | undefined> {
     return this.adminsRepository.findOneById(adminId)
   }
 
   async findAllAdminUsers(): Promise<Admin[]> {
+    // TODO @yashmurty :
+    // Fetch resource and perform ACL check.
+
     return this.adminsRepository.findAll()
   }
 
-  async deleteOneAdminById(adminId: string): Promise<void> {
-    const admin = await this.adminsRepository.findOneById(adminId)
+  async deleteOneAdminById(requestAdminUser: RequestAdminUser, adminId: string): Promise<void> {
     // TODO @yashmurty :
-    // Fetch admin and check for ACL. If okay, proceed to delete.
+    // Fetch resource and perform ACL check. Check performed within the called function.
+    await this.getOneAdminById(requestAdminUser, adminId)
+
+    // Delete admin in Firestore admins collection.
+    await this.adminsRepository.deleteOneById(adminId)
+    // Delete admin in Firebase auth.
+    await this.firebaseService.DeleteFirebaseUser(adminId)
   }
 }

src/auth/auth.service.ts

@@ -32,7 +32,7 @@ export class AuthService {
   }
 
   async adminUserlogin(requestAdminUser: RequestAdminUser): Promise<void> {
-    const adminObj = await this.adminsService.findOneAdminById(requestAdminUser.uid)
+    const adminObj = await this.adminsService.findOneAdminByIdInternal(requestAdminUser.uid)
     if (!adminObj) {
       throw new ForbiddenException('User Id does not belong to an admin')
     }

src/shared/filters/all-exceptions.filter.ts

@@ -27,20 +27,23 @@ export class AllExceptionsFilter implements ExceptionFilter {
       return response.status(status).json(responseObject)
     }
 
-    // Log the stack for non-HttpException errors
-    if (exception instanceof Error) {
-      this.appLogger.error(exception.message, exception.stack, exception.name)
-    } else {
-      console.log(exception)
-    }
-
     const status = HttpStatus.INTERNAL_SERVER_ERROR
     const responseObject = {
       statusCode: status,
       timestamp: new Date().toISOString(),
       url: request.url,
     }
 
+    // Log the stack for non-HttpException errors
+    if (exception instanceof Error) {
+      this.appLogger.error(exception.message, exception.stack, exception.name)
+      responseObject['error'] = exception.name
+      responseObject['message'] = exception.message
+    } else {
+      console.log(exception)
+      responseObject['error'] = 'INTERNAL SERVER'
+    }
+
     this.appLogger.debug(JSON.stringify({ status, responseObject }))
     return response.status(status).json(responseObject)
   }

src/shared/firebase/firebase.service.ts

@@ -60,4 +60,12 @@ export class FirebaseService {
     // Overwrite firebase custom claims with newCustomClaims.
     await firebaseAdmin.auth().setCustomUserClaims(userId, newCustomClaims)
   }
+
+  /**
+   * Delete user in firebase auth.
+   * @param userId: string
+   */
+  async DeleteFirebaseUser(userId: string): Promise<void> {
+    await firebaseAdmin.auth().deleteUser(userId)
+  }
 }