From 8644c7c3fe1c9cbb7c3ce2df85c6b7772d0ca6c2 Mon Sep 17 00:00:00 2001 From: tangoyankee Date: Thu, 5 Dec 2024 11:18:04 -0500 Subject: [PATCH] Guard against unauthorized project access Authorization guard for projects Remove pass-through for planning email addresses Feature flag use of dcpcreeper email closes #nycplanning/ae-private#53 --- .github/workflows/production.yml | 1 + .github/workflows/qa.yml | 4 +- .github/workflows/staging.yml | 1 + server/src/auth.middleware.ts | 8 +- server/src/authorize.guard.ts | 215 ++++++++++++++++++ server/src/config/config.service.ts | 1 + server/src/packages/package-access.guard.ts | 84 ------- server/src/packages/packages.controller.ts | 21 +- .../project-applicant.controller.ts | 10 +- server/src/projects/projects.controller.ts | 16 +- server/src/projects/projects.service.ts | 2 +- server/src/relationships.decorator.ts | 5 + 12 files changed, 258 insertions(+), 110 deletions(-) create mode 100644 server/src/authorize.guard.ts delete mode 100644 server/src/packages/package-access.guard.ts create mode 100644 server/src/relationships.decorator.ts diff --git a/.github/workflows/production.yml b/.github/workflows/production.yml index b447cafb..a8c81591 100644 --- a/.github/workflows/production.yml +++ b/.github/workflows/production.yml @@ -48,6 +48,7 @@ jobs: appdir: server env: HD_FEATURE_FLAG_SELF_SERVICE: ${{ vars.FEATURE_FLAG_SELF_SERVICE }} + HD_FEATURE_FLAG_CREEPER: ${{ vars.FEATURE_FLAG_CREEPER }} HD_ADO_PRINCIPAL: ${{ secrets.ADO_PRINCIPAL }} HD_AUTHORITY_HOST_URL: ${{ secrets.AUTHORITY_HOST_URL }} HD_CITYPAY_AGENCYID: ${{ secrets.CITYPAY_AGENCYID }} diff --git a/.github/workflows/qa.yml b/.github/workflows/qa.yml index fed2c3d3..f4d814e6 100644 --- a/.github/workflows/qa.yml +++ b/.github/workflows/qa.yml @@ -48,6 +48,7 @@ jobs: appdir: server env: HD_FEATURE_FLAG_SELF_SERVICE: ${{ vars.FEATURE_FLAG_SELF_SERVICE }} + HD_FEATURE_FLAG_CREEPER: ${{ vars.FEATURE_FLAG_CREEPER }} HD_ADO_PRINCIPAL: ${{ secrets.ADO_PRINCIPAL }} HD_AUTHORITY_HOST_URL: ${{ secrets.AUTHORITY_HOST_URL }} HD_CITYPAY_AGENCYID: ${{ secrets.CITYPAY_AGENCYID }} @@ -104,7 +105,7 @@ jobs: uses: actions/setup-node@v4 with: node-version: 12.x - - name: Install application dependencies + - name: Install application dependencies working-directory: client run: yarn install --immutable --immutable-cache --check-cache - name: Build client @@ -121,4 +122,3 @@ jobs: --site ${{secrets.NETLIFY_SITE_ID}} \ --auth ${{secrets.NETLIFY_AUTH_TOKEN}} \ --message "${{ github.event.head_commit.message }}" - diff --git a/.github/workflows/staging.yml b/.github/workflows/staging.yml index 181bed94..081bb626 100644 --- a/.github/workflows/staging.yml +++ b/.github/workflows/staging.yml @@ -52,6 +52,7 @@ jobs: appdir: server env: HD_FEATURE_FLAG_SELF_SERVICE: ${{ vars.FEATURE_FLAG_SELF_SERVICE }} + HD_FEATURE_FLAG_CREEPER: ${{ vars.FEATURE_FLAG_CREEPER }} HD_ADO_PRINCIPAL: ${{ secrets.ADO_PRINCIPAL }} HD_AUTHORITY_HOST_URL: ${{ secrets.AUTHORITY_HOST_URL }} HD_CLIENT_ID: ${{ secrets.CLIENT_ID }} diff --git a/server/src/auth.middleware.ts b/server/src/auth.middleware.ts index 9d047e96..480c9e7f 100644 --- a/server/src/auth.middleware.ts +++ b/server/src/auth.middleware.ts @@ -38,11 +38,7 @@ export class AuthMiddleware implements NestMiddleware { const email = refererParams.searchParams.get('email'); // the query param, email, sent from the client. this is who the "Creeper" wants to be. const { mail: userEmail } = validatedToken; // the "creepers" actual email, for verification. - if ( - email && - (userEmail === 'dcpcreeper@gmail.com' || - userEmail.includes('@planning.nyc.gov')) - ) { + if (email && userEmail === 'dcpcreeper@gmail.com') { // simply include the "creeper" param in the session validatedToken = { ...validatedToken, @@ -82,8 +78,6 @@ export class AuthMiddleware implements NestMiddleware { const validatedSpoofedToken = await this.authService.validateCurrentToken(spoofedZapToken); - validatedSpoofedToken.isCreeper = true; - return validatedSpoofedToken; } } diff --git a/server/src/authorize.guard.ts b/server/src/authorize.guard.ts new file mode 100644 index 00000000..96a96d22 --- /dev/null +++ b/server/src/authorize.guard.ts @@ -0,0 +1,215 @@ +import { + CanActivate, + ExecutionContext, + HttpException, + HttpStatus, + Injectable, +} from '@nestjs/common'; +import { Reflector } from '@nestjs/core'; +import { ConfigService } from 'src/config/config.service'; +import { CrmService } from 'src/crm/crm.service'; +import { Relationship } from 'src/relationships.decorator'; + +const APPLICANT_ACTIVE_STATUS_CODE = 1; +const PROJECT_ACTIVE_STATE_CODE = 0; +const PROJECT_VISIBILITY_APPLICANT_ONLY = 717170002; +const PROJECT_VISIBILITY_GENERAL_PUBLIC = 717170003; + +@Injectable() +export class AuthorizeGuard implements CanActivate { + constructor( + private readonly crmService: CrmService, + private readonly config: ConfigService, + private readonly reflector: Reflector, + ) {} + + async canActivate(context: ExecutionContext): Promise { + const { + params: { projectId, packageId, projectApplicantId }, + session: { contactId, mail, creeperTargetEmail }, + body, + } = context.switchToHttp().getRequest() as { + params: { + projectId: string | undefined; + packageId: string | undefined; + projectApplicantId: string | undefined; + }; + session: { + contactId: string; + mail: string; + creeperTargetEmail: string | null | undefined; + }; + body: { + data?: { + relationships?: { + project?: { + data: { + id: string; + }; + }; + }; + }; + }; + }; + + const projectIdBody = body?.data?.relationships?.project?.data?.id; + const relationships: Array = + this.reflector.get('relationships', context.getHandler()) ?? []; + + return ( + this.withHelper(relationships, creeperTargetEmail, mail) || + this.withSelf(relationships, creeperTargetEmail) || + (await this.withApplicantTeam({ + contactId, + packageId, + projectId, + projectIdBody, + projectApplicantId, + relationships, + })) + ); + } + + withHelper( + relationships: Array, + creeperTargetEmail: string | null | undefined, + mail: string, + ) { + if (!this.config.featureFlag.creeper) return false; + if (!relationships.includes('helper')) return false; + // There is no account being helped by creeper + if (creeperTargetEmail === null || creeperTargetEmail === undefined) + return false; + if (mail === 'dcpcreeper@gmail.com') return true; + return false; + } + + withSelf( + relationships: Array, + creeperTargetEmail: string | null | undefined, + ) { + if (!relationships.includes('self')) return false; + // Access is not being attempted by the creeper account + if (creeperTargetEmail === null || creeperTargetEmail === undefined) + return true; + return false; + } + + withApplicantTeam({ + contactId, + packageId, + projectId, + projectIdBody, + projectApplicantId, + relationships, + }: { + contactId: string; + packageId: string | undefined; + projectId: string | undefined; + projectIdBody: string | undefined; + projectApplicantId: string | undefined; + relationships: Array; + }) { + if (!relationships.includes('applicant-team')) return false; + if (projectId !== undefined) { + return this.checkByProjectId(contactId, projectId); + } else if (projectIdBody !== undefined) { + return this.checkByProjectId(contactId, projectIdBody); + } else if (packageId !== undefined) { + return this.checkByPackageId(contactId, packageId); + } else if (projectApplicantId !== undefined) { + return this.checkByProjectApplicantId(contactId, projectApplicantId); + } else { + return false; + } + } + + async checkByProjectApplicantId( + contactId: string, + projectApplicantId: string, + ) { + try { + const { records } = await this.crmService.get( + 'dcp_projects', + ` + $filter= + dcp_dcp_project_dcp_projectapplicant_Project/ + any(o: + o/dcp_projectapplicantid eq '${projectApplicantId}' + ) + and dcp_dcp_project_dcp_projectapplicant_Project/ + any(o: + o/_dcp_applicant_customer_value eq '${contactId}' + and o/statuscode eq ${APPLICANT_ACTIVE_STATUS_CODE} + ) + `, + ); + return records.length > 0; + } catch (e) { + console.error('failed to find project applicant', e); + throw new HttpException( + 'Could not find project applicant', + HttpStatus.INTERNAL_SERVER_ERROR, + ); + } + } + + async checkByProjectId(contactId: string, projectId: string) { + try { + const { records } = await this.crmService.get( + 'dcp_projects', + ` + $filter= + dcp_dcp_project_dcp_projectapplicant_Project/ + any(o: + o/_dcp_applicant_customer_value eq '${contactId}' + and o/statuscode eq ${APPLICANT_ACTIVE_STATUS_CODE} + ) + and dcp_projectid eq '${projectId}' + `, + ); + + return records.length > 0; + } catch (e) { + console.error('failed to find project for user', e); + throw new HttpException( + 'Could not find project', + HttpStatus.INTERNAL_SERVER_ERROR, + ); + } + } + + async checkByPackageId(contactId: string, packageId: string) { + try { + const { records } = await this.crmService.get( + 'dcp_projects', + ` + $filter= + dcp_dcp_project_dcp_package_project/ + any(o: + o/dcp_packageid eq '${packageId}' + ) + and + dcp_dcp_project_dcp_projectapplicant_Project/ + any(o: + o/_dcp_applicant_customer_value eq '${contactId}' + and o/statuscode eq ${APPLICANT_ACTIVE_STATUS_CODE} + ) + and ( + dcp_visibility eq ${PROJECT_VISIBILITY_APPLICANT_ONLY} + or dcp_visibility eq ${PROJECT_VISIBILITY_GENERAL_PUBLIC} + ) + and statecode eq ${PROJECT_ACTIVE_STATE_CODE} + `, + ); + + return records.length > 0; + } catch (e) { + console.error('cannot verify package or contact', e); + throw new HttpException( + 'Cannot find package', + HttpStatus.INTERNAL_SERVER_ERROR, + ); + } + } +} diff --git a/server/src/config/config.service.ts b/server/src/config/config.service.ts index a94a8f97..5e7326af 100644 --- a/server/src/config/config.service.ts +++ b/server/src/config/config.service.ts @@ -37,6 +37,7 @@ export class ConfigService { get featureFlag() { return { selfService: this.envConfig['FEATURE_FLAG_SELF_SERVICE'] === 'ON', + creeper: this.envConfig['FEATURE_FLAG_CREEPER'] === 'ON', }; } } diff --git a/server/src/packages/package-access.guard.ts b/server/src/packages/package-access.guard.ts deleted file mode 100644 index ea2c20a6..00000000 --- a/server/src/packages/package-access.guard.ts +++ /dev/null @@ -1,84 +0,0 @@ -import { - CanActivate, - ExecutionContext, - HttpException, - HttpStatus, - Injectable, -} from '@nestjs/common'; -import { CrmService } from '../crm/crm.service'; - -const APPLICANT_ACTIVE_STATUS_CODE = 1; -const PROJECT_ACTIVE_STATE_CODE = 0; -const PROJECT_VISIBILITY_APPLICANT_ONLY = 717170002; -const PROJECT_VISIBILITY_GENERAL_PUBLIC = 717170003; - -// This guard makes sure that only -// authenticated users who are part of the requested -// package's project applicant team can access the package -@Injectable() -export class PackageAccessGuard implements CanActivate { - constructor(private readonly crmService: CrmService) {} - - async canActivate(context: ExecutionContext): Promise { - const { - params: { id: packageId }, - session: { contactId, isCreeper = false, ...theRest }, - } = context.switchToHttp().getRequest(); - - // permit internal staff to view - if ( - theRest.mail === 'dcpcreeper@gmail.com' || - theRest.mail?.includes('@planning.nyc.gov') - ) { - return true; - } - - // because creeper mode allows authorization on all resources, skip this layer - if (isCreeper) { - return true; - } - - if (contactId) { - return this.isOnApplicantTeam(packageId, contactId); - } - - throw new HttpException( - { - code: 'NO_PACKAGE_ACCESS', - title: 'No access to package', - detail: 'Access to the requested package is forbidden.', - }, - HttpStatus.FORBIDDEN, - ); - } - - async isOnApplicantTeam(packageId, contactId) { - const { records } = await this.crmService.get( - 'dcp_projects', - ` - $filter= - dcp_dcp_project_dcp_package_project/ - any(o: - o/dcp_packageid eq '${packageId}' - ) - and - dcp_dcp_project_dcp_projectapplicant_Project/ - any(o: - o/_dcp_applicant_customer_value eq '${contactId}' - and o/statuscode eq ${APPLICANT_ACTIVE_STATUS_CODE} - ) - and ( - dcp_visibility eq ${PROJECT_VISIBILITY_APPLICANT_ONLY} - or dcp_visibility eq ${PROJECT_VISIBILITY_GENERAL_PUBLIC} - ) - and statecode eq ${PROJECT_ACTIVE_STATE_CODE} - `, - ); - - if (records.length > 0) { - return true; - } - - return false; - } -} diff --git a/server/src/packages/packages.controller.ts b/server/src/packages/packages.controller.ts index c05d1087..cc91e00b 100644 --- a/server/src/packages/packages.controller.ts +++ b/server/src/packages/packages.controller.ts @@ -13,7 +13,6 @@ import { import { PackagesService } from './packages.service'; import { JsonApiSerializeInterceptor } from '../json-api-serialize.interceptor'; import { JsonApiDeserializePipe } from '../json-api-deserialize.pipe'; -import { PackageAccessGuard } from './package-access.guard'; import { pick } from 'underscore'; import { LANDUSE_FORM_ATTRS } from './landuse-form/landuse-form.attrs'; import { RWCDS_FORM_ATTRS } from './rwcds-form/rwcds-form.attrs'; @@ -35,6 +34,9 @@ import { ZONING_MAP_CHANGE_ATTRS } from './landuse-form/zoning-map-changes/zonin import { CEQR_INVOICE_QUESTIONNAIRE_ATTRS } from './ceqr-invoice-questionnaires/ceqr-invoice-questionnaires.attrs'; import { CitypayService } from '../citypay/citypay.service'; import { ARTIFACTS_ATTRS } from '../artifacts/artifacts.attrs'; +import { AuthenticateGuard } from 'src/authenticate.guard'; +import { AuthorizeGuard } from 'src/authorize.guard'; +import { Relationships } from 'src/relationships.decorator'; @UseInterceptors( new JsonApiSerializeInterceptor('packages', { @@ -299,7 +301,7 @@ import { ARTIFACTS_ATTRS } from '../artifacts/artifacts.attrs'; }), ) @UsePipes(JsonApiDeserializePipe) -@UseGuards(PackageAccessGuard) +@UseGuards(AuthenticateGuard, AuthorizeGuard) @Controller('packages') export class PackagesController { constructor( @@ -307,8 +309,9 @@ export class PackagesController { private readonly cityPay: CitypayService, ) {} - @Get('/:id') - getPackage(@Param('id') id) { + @Get('/:packageId') + @Relationships('helper', 'applicant-team') + getPackage(@Param('packageId') id: string) { try { return this.packagesService.getPackage(id); } catch (e) { @@ -327,8 +330,9 @@ export class PackagesController { } } - @Patch('/:id') - async patchPackage(@Body() body, @Param('id') id) { + @Patch('/:packageId') + @Relationships('applicant-team') + async patchPackage(@Body() body, @Param('packageId') id: string) { try { const allowedAttrs = pick(body, PACKAGE_ATTRS); @@ -354,8 +358,9 @@ export class PackagesController { } } - @Get('/pay/:id') - async generatePaymentLink(@Param('id') id) { + @Get('/pay/:packageId') + @Relationships('helper', 'applicant-team') + async generatePaymentLink(@Param('packageId') id: string) { return { city_pay_url: await this.cityPay.generateCityPayLink(id), }; diff --git a/server/src/projects/project-applicants/project-applicant.controller.ts b/server/src/projects/project-applicants/project-applicant.controller.ts index 7f4bf418..f0d219c8 100644 --- a/server/src/projects/project-applicants/project-applicant.controller.ts +++ b/server/src/projects/project-applicants/project-applicant.controller.ts @@ -18,6 +18,8 @@ import { JsonApiDeserializePipe } from '../../json-api-deserialize.pipe'; import { PROJECTAPPLICANT_ATTRS } from './project-applicants.attrs'; import { CONTACT_ATTRS } from '../../contact/contacts.attrs'; import { ContactService } from '../../contact/contact.service'; +import { Relationships } from 'src/relationships.decorator'; +import { AuthorizeGuard } from 'src/authorize.guard'; const ACTIVE_STATUSCODE = 1; const INACTIVE_STATUSCODE = 2; @@ -36,7 +38,7 @@ const INACTIVE_STATECODE = 1; }, }), ) -@UseGuards(AuthenticateGuard) +@UseGuards(AuthenticateGuard, AuthorizeGuard) @UsePipes(JsonApiDeserializePipe) @Controller('project-applicants') export class ProjectApplicantController { @@ -46,6 +48,7 @@ export class ProjectApplicantController { ) {} @Post('/') + @Relationships('applicant-team') async create(@Body() body) { const allowedAttrs = pick(body, PROJECTAPPLICANT_ATTRS); // NOTE: dcp_name field in projectApplicant entity is automatically filled with... @@ -130,8 +133,9 @@ export class ProjectApplicantController { }; } - @Delete('/:id') - async deactivate(@Param('id') id) { + @Delete('/:projectApplicantId') + @Relationships('applicant-team') + async deactivate(@Param('projectApplicantId') id: string) { try { await this.crmService.update('dcp_projectapplicants', id, { statuscode: INACTIVE_STATUSCODE, diff --git a/server/src/projects/projects.controller.ts b/server/src/projects/projects.controller.ts index 40e2d0ae..27a8c6bf 100644 --- a/server/src/projects/projects.controller.ts +++ b/server/src/projects/projects.controller.ts @@ -26,6 +26,8 @@ import { PROJECTAPPLICANT_ATTRS } from './project-applicants/project-applicants. import { TEAMMEMBER_ATTRS } from './team-members/team-members.attrs'; import { CONTACT_ATTRS } from '../contact/contacts.attrs'; import { INVOICE_ATTRS } from '../invoices/invoices.attrs'; +import { Relationships } from 'src/relationships.decorator'; +import { AuthorizeGuard } from 'src/authorize.guard'; @UseInterceptors( new JsonApiSerializeInterceptor('projects', { @@ -77,7 +79,7 @@ import { INVOICE_ATTRS } from '../invoices/invoices.attrs'; }, }), ) -@UseGuards(AuthenticateGuard) +@UseGuards(AuthenticateGuard, AuthorizeGuard) @UsePipes(JsonApiDeserializePipe) @Controller('projects') export class ProjectsController { @@ -93,6 +95,7 @@ export class ProjectsController { } @Get('/') + @Relationships('helper', 'self') async listOfCurrentUserProjects(@Session() session) { const { creeperTargetEmail } = session; let { contactId } = session; @@ -130,6 +133,7 @@ export class ProjectsController { } @Post('/') + @Relationships('self') async createProject(@Body() body) { const allowedAttrs = pick(body, PROJECT_ATTRS) as { dcp_projectname: string; @@ -152,8 +156,9 @@ export class ProjectsController { return await this.projectsService.create(allowedAttrs); } - @Get('/:id') - async projectById(@Param('id') id) { + @Get('/:projectId') + @Relationships('helper', 'applicant-team') + async projectById(@Param('projectId') id: string) { try { return await this.projectsService.getProject(id); } catch (e) { @@ -172,8 +177,9 @@ export class ProjectsController { } } - @Patch('/:id') - async update(@Body() body, @Param('id') id) { + @Patch('/:projectId') + @Relationships('applicant-team') + async update(@Body() body, @Param('projectId') id: string) { const allowedAttrs = pick(body, PROJECT_ATTRS); await this.crmService.update('dcp_projects', id, allowedAttrs); diff --git a/server/src/projects/projects.service.ts b/server/src/projects/projects.service.ts index d1364fdd..1608d349 100644 --- a/server/src/projects/projects.service.ts +++ b/server/src/projects/projects.service.ts @@ -126,7 +126,7 @@ export class ProjectsService { dcp_projectid, }; } catch (e) { - console.debug('error creating project', e); + console.error('error creating project', e); throw new HttpException( 'Unable to create project', HttpStatus.INTERNAL_SERVER_ERROR, diff --git a/server/src/relationships.decorator.ts b/server/src/relationships.decorator.ts new file mode 100644 index 00000000..91dfaf22 --- /dev/null +++ b/server/src/relationships.decorator.ts @@ -0,0 +1,5 @@ +import { SetMetadata } from '@nestjs/common'; + +export type Relationship = 'helper' | 'self' | 'applicant-team'; +export const Relationships = (...relationships: Array) => + SetMetadata('relationships', relationships);