9062526094

Committed 13 May 2024 12:04PM UTC coverage: 92.825% (-0.02%) from 92.844%

Build # 9062526094

Build Type

push

github

Committed by

web-flow

Commit Message

Authorise recovery module adding/deleting with JWT authentication (#1527)

Migrates the suboptiomal authentication/authorisation for adding/removing recovery module addresses to the new JWT/SIWE-based approach:

- Remove `OnlySafeOwner`, `TimestampGuard`, `DisableRecoveryAlertsGuard` and `EnableRecoveryAlertsGuard` and associated tests
- Replace above guard usage `AuthGuard`
- Checksum incoming require Safe/module addresses
- Pass `AuthPayload` to domain and assert chain, signer and ownership before adding/deleting module
- Add/update associated test coverage

Run Details

1978 of 2405 branches covered (82.25%)

Branch coverage included in aggregate %.

22 of 25 new or added lines in 3 files covered. (88.0%)

1 existing line in 1 file now uncovered.

6961 of 7225 relevant lines covered (96.35%)

323.47 hits per line

Source File
Press 'n' to go to next uncovered line, 'b' for previous

92.11

/src/routes/recovery/recovery.service.ts

import { Inject, Injectable, UnauthorizedException } from '@nestjs/common';
import { IAlertsRepository } from '@/domain/alerts/alerts.repository.interface';
import { AlertsRepository } from '@/domain/alerts/alerts.repository';
import { AddRecoveryModuleDto } from '@/routes/recovery/entities/add-recovery-module.dto.entity';
import { AlertsRegistration } from '@/domain/alerts/entities/alerts-registration.entity';
import { AuthPayload } from '@/domain/auth/entities/auth-payload.entity';
import { ISafeRepository } from '@/domain/safe/safe.repository.interface';

@Injectable()
export class RecoveryService {
  constructor(
    @Inject(IAlertsRepository)
    private readonly alertsRepository: AlertsRepository,
    @Inject(ISafeRepository)
    private readonly safeRepository: ISafeRepository,
  ) {}

  async addRecoveryModule(args: {
    chainId: string;
    safeAddress: `0x${string}`;
    addRecoveryModuleDto: AddRecoveryModuleDto;
    authPayload: AuthPayload;
  }): Promise<void> {
    if (
      !args.authPayload.isForChain(args.chainId) ||
      !args.authPayload.signer_address
    ) {
      throw new UnauthorizedException();
    }

    // Check after AuthPayload check to avoid unnecessary request
    const isOwner = await this.safeRepository
      .isOwner({
        safeAddress: args.safeAddress,
        chainId: args.chainId,
        address: args.authPayload.signer_address,
      })
      // Swallow error to avoid leaking information
      .catch(() => false);
    if (!isOwner) {
      throw new UnauthorizedException();
    }

    // After after owner check to avoid unnecessary request
    const isEnabled = await this.safeRepository
      .getSafesByModule({
        chainId: args.chainId,
        moduleAddress: args.addRecoveryModuleDto.moduleAddress,
      })
      .then(({ safes }) => {
        return safes.some((safe) => safe === args.safeAddress);
      })

      // Swallow error to avoid leaking information
      .catch(() => false);
    if (!isEnabled) {
      throw new UnauthorizedException();
    }

    const contract: AlertsRegistration = {
      chainId: args.chainId,
      address: args.addRecoveryModuleDto.moduleAddress,
      displayName: `${args.chainId}:${args.safeAddress}:${args.addRecoveryModuleDto.moduleAddress}`,
    };
    await this.alertsRepository.addContract(contract);
  }

  async deleteRecoveryModule(args: {
    chainId: string;
    moduleAddress: `0x${string}`;
    safeAddress: `0x${string}`;
    authPayload: AuthPayload;
  }): Promise<void> {
    if (
      !args.authPayload.isForChain(args.chainId) ||
      !args.authPayload.signer_address
    ) {
      throw new UnauthorizedException();
    }

    // Check after AuthPayload check to avoid unnecessary request
    const isOwner = await this.safeRepository
      .isOwner({
        safeAddress: args.safeAddress,
        chainId: args.chainId,
        address: args.authPayload.signer_address,
      })
      // Swallow error to avoid leaking information
      .catch(() => false);
    if (!isOwner) {
      throw new UnauthorizedException();
    }

    await this.alertsRepository.deleteContract({
      chainId: args.chainId,
      address: args.moduleAddress,
    });
  }
}

1	import { Inject, Injectable, UnauthorizedException } from '@nestjs/common';	96✔
2	import { IAlertsRepository } from '@/domain/alerts/alerts.repository.interface';	96✔
3	import { AlertsRepository } from '@/domain/alerts/alerts.repository';	96✔
4	import { AddRecoveryModuleDto } from '@/routes/recovery/entities/add-recovery-module.dto.entity';
5	import { AlertsRegistration } from '@/domain/alerts/entities/alerts-registration.entity';
6	import { AuthPayload } from '@/domain/auth/entities/auth-payload.entity';
7	import { ISafeRepository } from '@/domain/safe/safe.repository.interface';	96✔
8
9	@Injectable()
10	export class RecoveryService {	96✔
11	constructor(
12	@Inject(IAlertsRepository)
13	private readonly alertsRepository: AlertsRepository,	88✔
14	@Inject(ISafeRepository)
15	private readonly safeRepository: ISafeRepository,	88✔
16	) {}
17
18	async addRecoveryModule(args: {
19	chainId: string;
20	safeAddress: `0x${string}`;
21	addRecoveryModuleDto: AddRecoveryModuleDto;
22	authPayload: AuthPayload;
23	}): Promise<void> {
24	if (	8✔
25	!args.authPayload.isForChain(args.chainId) \|\|	7✔
26	!args.authPayload.signer_address
27	) {
28	throw new UnauthorizedException();	2✔
29	}
30
31	// Check after AuthPayload check to avoid unnecessary request
32	const isOwner = await this.safeRepository	6✔
33	.isOwner({
34	safeAddress: args.safeAddress,
35	chainId: args.chainId,
36	address: args.authPayload.signer_address,
37	})
38	// Swallow error to avoid leaking information
NEW 39	.catch(() => false);	×
40	if (!isOwner) {	6✔
41	throw new UnauthorizedException();	2✔
42	}
43
44	// After after owner check to avoid unnecessary request
45	const isEnabled = await this.safeRepository	4✔
46	.getSafesByModule({
47	chainId: args.chainId,
48	moduleAddress: args.addRecoveryModuleDto.moduleAddress,
49	})
50	.then(({ safes }) => {
51	return safes.some((safe) => safe === args.safeAddress);	4✔
52	})
53
54	// Swallow error to avoid leaking information
NEW 55	.catch(() => false);	×
56	if (!isEnabled) {	4✔
57	throw new UnauthorizedException();	2✔
58	}
59
60	const contract: AlertsRegistration = {	2✔
61	chainId: args.chainId,
62	address: args.addRecoveryModuleDto.moduleAddress,
63	displayName: `${args.chainId}:${args.safeAddress}:${args.addRecoveryModuleDto.moduleAddress}`,
64	};
65	await this.alertsRepository.addContract(contract);	2✔
66	}
67
68	async deleteRecoveryModule(args: {
69	chainId: string;
70	moduleAddress: `0x${string}`;
71	safeAddress: `0x${string}`;
72	authPayload: AuthPayload;
73	}): Promise<void> {
74	if (	10✔
75	!args.authPayload.isForChain(args.chainId) \|\|	9✔
76	!args.authPayload.signer_address
77	) {
78	throw new UnauthorizedException();	2✔
79	}
80
81	// Check after AuthPayload check to avoid unnecessary request
82	const isOwner = await this.safeRepository	8✔
83	.isOwner({
84	safeAddress: args.safeAddress,
85	chainId: args.chainId,
86	address: args.authPayload.signer_address,
87	})
88	// Swallow error to avoid leaking information
NEW 89	.catch(() => false);	×
90	if (!isOwner) {	8✔
91	throw new UnauthorizedException();	2✔
92	}
93
94	await this.alertsRepository.deleteContract({	6✔
95	chainId: args.chainId,
96	address: args.moduleAddress,
97	});
98	}
99	}

safe-global / safe-client-gateway / 9062526094

Source File Press 'n' to go to next uncovered line, 'b' for previous

Source File
Press 'n' to go to next uncovered line, 'b' for previous