Skip to content

Commit

Permalink
PRO-2632: Fix migration (#46)
Browse files Browse the repository at this point in the history
* The initial migration wasn't properly handling constraint and index renaming in a case-insensitive way. Testing in the protocol repo revealed that this could potentially cause failed a migration. Using ILIKE and wrapping the constraint name in quotation marks solved the issue.
* There was a mis-named index that was a duplicate of another existing index
* The PluginModule needs to pull in the DbModule in order to make the ExternalIdService accessible from the FingerprintPlugin
* Request to Identity Service to verify fingerprint should use `dids` as a filter instead of `did`
* Update protocol-common and use updated error codes to be consistent with the platform.

Signed-off-by: Jeff Kennedy <[email protected]>
  • Loading branch information
Jeff Kennedy authored Feb 1, 2021
1 parent ea6355d commit 052c8c1
Show file tree
Hide file tree
Showing 10 changed files with 29 additions and 42 deletions.
6 changes: 3 additions & 3 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
"jsonwebtoken": "^8.5.1",
"jwks-rsa": "^1.12.2",
"pg": "^8.5.1",
"protocol-common": "^0.1.28",
"protocol-common": "^0.1.29",
"reflect-metadata": "^0.1.13",
"rxjs": "^6.6.3",
"swagger-ui-express": "^4.1.6",
Expand Down
14 changes: 7 additions & 7 deletions src/db/migration/1610566573713-MigrateTables.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@ export class MigrateTables1610566573713 implements MigrationInterface {
i RECORD;
BEGIN
FOR i IN
(SELECT indexname FROM pg_indexes WHERE tablename = _tablename AND LOWER(indexname) LIKE 'idx%' AND LOWER(indexdef) LIKE CONCAT('%', LOWER(_colname), '%'))
(SELECT indexname FROM pg_indexes WHERE tablename = _tablename AND indexname ILIKE 'idx%' AND indexdef ILIKE CONCAT('%', _colname, '%'))
LOOP
RAISE INFO 'DROPPING INDEX: %', i.indexname;
EXECUTE 'DROP INDEX IF EXISTS ' || i.indexname;
EXECUTE 'DROP INDEX IF EXISTS "' || i.indexname || '"';
END LOOP;
RETURN 1;
END;
Expand Down Expand Up @@ -46,17 +46,17 @@ export class MigrateTables1610566573713 implements MigrationInterface {
BEGIN
IF _pk THEN
FOR i IN
(SELECT indexname FROM pg_indexes WHERE tablename = _tablename AND LOWER(indexname) LIKE 'pk%' AND LOWER(indexdef) LIKE CONCAT('%', LOWER(_colname), '%'))
(SELECT indexname FROM pg_indexes WHERE tablename = _tablename AND indexname ILIKE 'pk%' AND indexdef ILIKE CONCAT('%', _colname, '%'))
LOOP
RAISE INFO 'RENAMING CONSTRAINT % to %', i.indexname, _newname;
EXECUTE 'ALTER TABLE IF EXISTS ' || _tablename || ' RENAME CONSTRAINT ' || i.indexname || ' TO ' || _newname;
EXECUTE 'ALTER TABLE IF EXISTS ' || _tablename || ' RENAME CONSTRAINT "' || i.indexname || '" TO "' || _newname || '"';
END LOOP;
ELSE
FOR i IN
(SELECT indexname FROM pg_indexes WHERE tablename = _tablename AND LOWER(indexname) LIKE 'uq%' AND LOWER(indexdef) LIKE CONCAT('%', LOWER(_colname), '%'))
(SELECT indexname FROM pg_indexes WHERE tablename = _tablename AND indexname ILIKE 'uq%' AND indexdef ILIKE CONCAT('%', _colname, '%'))
LOOP
RAISE INFO 'RENAMING INDEX % to %', i.indexname, _newname;
EXECUTE 'ALTER TABLE IF EXISTS ' || _tablename || ' RENAME CONSTRAINT ' || i.indexname || ' TO ' || _newname;
EXECUTE 'ALTER TABLE IF EXISTS ' || _tablename || ' RENAME CONSTRAINT "' || i.indexname || '" TO "' || _newname || '"';
END LOOP;
END IF;
RETURN 1;
Expand All @@ -81,7 +81,7 @@ export class MigrateTables1610566573713 implements MigrationInterface {
`SELECT rename_constraint('wallet_credentials', 'id', 'wallet_credentials_pkey', true);`
);
await queryRunner.query(
`SELECT rename_constraint('wallet_credentials', 'did', 'sms_otp_did_key', false);`
`SELECT rename_constraint('wallet_credentials', 'did', 'wallet_credentials_did_key', false);`
);

// Don't need this function anymore, so delete it
Expand Down
10 changes: 5 additions & 5 deletions src/plugins/impl/fingerprint.plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@ export class FingerprintPlugin implements IPlugin {
} catch(e) {
// Handle specific error codes
switch (e.code) {
case 'FINGERPRINT_NO_MATCH':
case 'FINGERPRINT_MISSING_NOT_CAPTURED':
case 'FINGERPRINT_MISSING_AMPUTATION':
case 'FINGERPRINT_MISSING_UNABLE_TO_PRINT':
case ProtocolErrorCode.FINGERPRINT_NO_MATCH:
case ProtocolErrorCode.FINGERPRINT_MISSING_NOT_CAPTURED:
case ProtocolErrorCode.FINGERPRINT_MISSING_AMPUTATION:
case ProtocolErrorCode.FINGERPRINT_MISSING_UNABLE_TO_PRINT:
if (process.env.QUALITY_CHECK_ENABLED === 'true') {
e = await this.fingerprintQualityCheck(e, did);
}
Expand All @@ -54,7 +54,7 @@ export class FingerprintPlugin implements IPlugin {

// The identity service should throw this error on no match, but just to be safe double check it and throw here
if (response.data.status !== 'matched') {
throw new ProtocolException(ProtocolErrorCode.FINGERPRINT_NOMATCH, 'Fingerprint did not match stored records for citizen supplied through filters');
throw new ProtocolException(ProtocolErrorCode.FINGERPRINT_NO_MATCH, 'Fingerprint did not match stored records for citizen supplied through filters');
}

// TODO right now the data we get from identity service uses did we should change it to agent id and then we don't need this conversion
Expand Down
2 changes: 2 additions & 0 deletions src/plugins/plugin.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@ import { SmsModule } from '../sms/sms.module';
import { PluginFactory } from './plugin.factory';
import { RemoteModule } from '../remote/remote.module';
import { TokenModule } from '../token/token.module';
import { DbModule } from '../db/db.module';

/**
* Module for our different authentication plugins
*/
@Module({
imports: [
DbModule,
RemoteModule,
SmsModule,
TokenModule,
Expand Down
4 changes: 2 additions & 2 deletions src/remote/impl/identity.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export class IdentityService implements IIdentityService {
position,
image,
filters: {
did
dids: did
},
},
};
Expand All @@ -50,7 +50,7 @@ export class IdentityService implements IIdentityService {
position,
image: template,
filters: {
did
dids: did
},
imageType: 'TEMPLATE',
},
Expand Down
3 changes: 1 addition & 2 deletions src/remote/impl/sms.twillio.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { Logger } from 'protocol-common/logger';
import { ProtocolException } from 'protocol-common/protocol.exception';
import { ProtocolErrorCode } from 'protocol-common/protocol.errorcode';
import { Constants } from 'protocol-common/constants';
import { SmsErrorCode } from '../../sms/sms.errorcode';
import twilio from 'twilio';
import { ISmsService } from '../sms.service.interface';

Expand Down Expand Up @@ -33,7 +32,7 @@ export class SmsTwillioService implements ISmsService {
}
} catch (e) {
Logger.log('Error sending SMS', e);
throw new ProtocolException(SmsErrorCode.SMS_SEND_FAILED, 'SMS failed to send');
throw new ProtocolException(ProtocolErrorCode.SMS_SEND_FAILED, 'SMS failed to send');
}
}

Expand Down
12 changes: 0 additions & 12 deletions src/sms/sms.errorcode.ts

This file was deleted.

11 changes: 5 additions & 6 deletions src/sms/sms.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { ProtocolException } from 'protocol-common/protocol.exception';
import { ProtocolErrorCode } from 'protocol-common/protocol.errorcode';
import { SecurityUtility } from 'protocol-common/security.utility';
import { SmsOtp } from '../db/entity/sms.otp';
import { SmsErrorCode } from './sms.errorcode';
import { SmsParamsDto } from './dto/sms.params.dto';
import { RateLimitService } from '../ratelimit/ratelimit.service';
import { RateLimitBucket } from '../ratelimit/ratelimit.bucket';
Expand Down Expand Up @@ -57,7 +56,7 @@ export class SmsService {
const attempt = async (key: string) => {
await this.rateLimitService.addAttempt(bucket, key);
if (await this.rateLimitService.shouldLimit(bucket, key)) {
throw new ProtocolException(SmsErrorCode.TOO_MANY_ATTEMPTS, 'Too many OTP verification attempts. Please wait awhile and try again');
throw new ProtocolException(ProtocolErrorCode.TOO_MANY_ATTEMPTS, 'Too many OTP verification attempts. Please wait awhile and try again');
}
};

Expand All @@ -73,12 +72,12 @@ export class SmsService {
private async sendSmsOtp(did: string, phoneNumber: any) {
const smsOtpEntity = await this.findSmsOtpEntity(did);
if (!smsOtpEntity.phone_number_hash) {
throw new ProtocolException(SmsErrorCode.NO_PHONE_NUMBER, 'No phone number stored for citizen');
throw new ProtocolException(ProtocolErrorCode.NO_PHONE_NUMBER, 'No phone number stored for citizen');
}

const phoneNumberHash = SecurityUtility.hash32(phoneNumber + process.env.HASH_PEPPER);
if (smsOtpEntity.phone_number_hash !== phoneNumberHash) {
throw new ProtocolException(SmsErrorCode.PHONE_NUMBER_NO_MATCH, 'Phone number doesn\'t match for stored citizen');
throw new ProtocolException(ProtocolErrorCode.PHONE_NUMBER_NO_MATCH, 'Phone number doesn\'t match for stored citizen');
}

const otp = await this.generateOtp(smsOtpEntity);
Expand Down Expand Up @@ -120,10 +119,10 @@ export class SmsService {
private async verifyOtp(id: string, otp: number) {
const smsOtpEntity = await this.findSmsOtpEntity(id);
if (!smsOtpEntity.otp) {
throw new ProtocolException(SmsErrorCode.OTP_EXPIRED, 'The OTP has expired, please send again');
throw new ProtocolException(ProtocolErrorCode.OTP_EXPIRED, 'The OTP has expired, please send again');
}
if (smsOtpEntity.otp !== otp) {
throw new ProtocolException(SmsErrorCode.OTP_NO_MATCH, 'The OTP does not match');
throw new ProtocolException(ProtocolErrorCode.OTP_NO_MATCH, 'The OTP does not match');
}

smsOtpEntity.otp = null;
Expand Down
7 changes: 3 additions & 4 deletions test/e2e/escrow.sms.e2e-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import request from 'supertest';
import { Test } from '@nestjs/testing';
import { CACHE_MANAGER, INestApplication } from '@nestjs/common';
import { ProtocolErrorCode } from 'protocol-common/protocol.errorcode';
import { SmsErrorCode } from '../../src/sms/sms.errorcode';
import { RateLimitModule } from '../../src/ratelimit/ratelimit.module';
import { EscrowService } from '../../src/escrow/escrow.service';
import { getRepositoryToken } from '@nestjs/typeorm';
Expand Down Expand Up @@ -206,7 +205,7 @@ describe('EscrowController (e2e) using SMS plugin', () => {
.send(data)
.expect(400)
.then((res) => {
expect(res.body.code).toBe(SmsErrorCode.PHONE_NUMBER_NO_MATCH);
expect(res.body.code).toBe(ProtocolErrorCode.PHONE_NUMBER_NO_MATCH);
});
});

Expand All @@ -222,7 +221,7 @@ describe('EscrowController (e2e) using SMS plugin', () => {
.send(data)
.expect(400)
.then((res) => {
expect(res.body.code).toBe(SmsErrorCode.OTP_NO_MATCH);
expect(res.body.code).toBe(ProtocolErrorCode.OTP_NO_MATCH);
});
});

Expand All @@ -244,7 +243,7 @@ describe('EscrowController (e2e) using SMS plugin', () => {
.send(data)
.expect(400)
.then((res) => {
expect(res.body.code).toBe(SmsErrorCode.OTP_EXPIRED);
expect(res.body.code).toBe(ProtocolErrorCode.OTP_EXPIRED);
});
});

Expand Down

0 comments on commit 052c8c1

Please sign in to comment.