WIP: Feature/sms verification #78
Conversation
| TWILIO_ACCOUNT_SID=AC9b92504e0ef87d42765c1b338084abfb | ||
| TWILIO_API_KEY=SK803c26edc5e58ac78eccf7bc54789771 | ||
| TWILIO_API_SECRET=ToKtCcu5ZnTXAod8XUO85iW86wphqrbG | ||
| SENDING_PHONE_NUMBER=+19386665491 |
There was a problem hiding this comment.
I think another prefix is nice, the number belongs to twilio
| SENDING_PHONE_NUMBER=+19386665491 | |
| TWILIO_SENDING_PHONE_NUMBER=+19386665491 |
| TWILIO_API_SECRET=ToKtCcu5ZnTXAod8XUO85iW86wphqrbG | ||
| SENDING_PHONE_NUMBER=+19386665491 | ||
| APP_HASH=randomhash | ||
| CLIENT_SECRET=verysecret |
There was a problem hiding this comment.
This one is not really needed?
The name is a bit generic for a env.
| CLIENT_SECRET=verysecret |
| TWILIO_API_KEY=SK803c26edc5e58ac78eccf7bc54789771 | ||
| TWILIO_API_SECRET=ToKtCcu5ZnTXAod8XUO85iW86wphqrbG | ||
| SENDING_PHONE_NUMBER=+19386665491 | ||
| APP_HASH=randomhash |
There was a problem hiding this comment.
What is the APP_HASH needed for?
If it's a debugging identifier, one could use the API_ROOT_URL
| import { ApiProperty } from '@nestjs/swagger'; | ||
|
|
||
| export class VerificationDto { | ||
| @ApiProperty({ example: '+49123456789' }) |
There was a problem hiding this comment.
This is actually not a valid number :-D Tobi used the same one for testing and was wondering ;-)
| @ApiProperty({ example: '+49123456789' }) | |
| @ApiProperty({ example: '+491704567893' }) |
| @@ -1,11 +1,6 @@ | |||
| import { INestApplication } from '@nestjs/common'; | |||
| import { DocumentBuilder, SwaggerModule } from '@nestjs/swagger'; | |||
| import { | |||
There was a problem hiding this comment.
Can you run npm run format on it? I Think swagger wants single lines here.
|
|
||
| constructor(configService: ConfigurationService) { | ||
| let message = null; | ||
| if (configService.get('TWILIO_API_KEY') == null || |
There was a problem hiding this comment.
We can leave all the validation for the moment, but should definitely build an env variable validation:
| if (configService.get('TWILIO_API_KEY') == null || | |
| // TODO: build env variable validation | |
| if (configService.get('TWILIO_API_KEY') == null || |
| console.log(message); | ||
| Logger.error('Twilio will not work!', message); |
There was a problem hiding this comment.
| console.log(message); | |
| Logger.error('Twilio will not work!', message); | |
| this.logger.error('Twilio will not work!', message); |
| if (otp == null) { | ||
| const message = 'No cached otp value found for phone: ' + phone; | ||
| this.logger.log(message); | ||
| throw new BadRequestException(message); |
There was a problem hiding this comment.
Would a NotFoundException fit better? (request valid, but resource not there)
| if (smsMessage.indexOf(otp) <= -1) { | ||
| const message = 'Mismatch between otp value found and otp value expected'; | ||
| this.logger.log(message); | ||
| throw new BadRequestException(message); |
There was a problem hiding this comment.
If we want to keep BadRequest for input validation solely, maybe this could be a 422 (Unprocessable Entity).
| if (otp == null) { | ||
| const message = 'No cached otp value found for phone: ' + phone; | ||
| this.logger.log(message); | ||
| throw new BadRequestException(message); |
There was a problem hiding this comment.
see above, maybe a NotFoundException
Added (basic) SMS verification feature.
Missing: