-
Notifications
You must be signed in to change notification settings - Fork 291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Phone provider refactoring #1713
Changes from 13 commits
f1e9b30
2c8f83b
bbd1430
ab928fa
e0cc0b1
81178a6
0ccd284
678a23d
f66242f
b53558e
3b99554
21d589e
56d6ccd
9ffaa24
7fd7600
8dd39f9
26ebc43
3b8479e
381aa54
4d79515
bb5b505
d3ad2ea
b6cbd7d
ae09bbe
bfa71ce
7c8df3d
bf3ec04
db33b14
59ec9a9
1be1305
6f017da
67929a3
128c549
ea0a414
f589b53
6147dfd
81c2490
2eca39e
a513a0e
163aeb3
a38f4a8
4f16a9b
7f0e6d4
b9a6402
2ea310b
24d9eb0
3298392
7acb39c
3987336
9b71018
e65a3c3
0585099
7bebc85
2e336f9
0a227d4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ class ActionSource: | |
( | ||
SLACK, | ||
WEB, | ||
TWILIO, | ||
PHONE, | ||
TELEGRAM, | ||
) = range(4) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,10 +37,16 @@ | |
from apps.base.messaging import get_messaging_backend_from_id | ||
from apps.base.utils import live_settings | ||
from apps.mobile_app.auth import MobileAppAuthTokenAuthentication | ||
from apps.phone_notifications.exceptions import ( | ||
FailedToMakeCall, | ||
FailedToStartVerification, | ||
NumberAlreadyVerified, | ||
NumberNotVerified, | ||
ProviderNotSupports, | ||
) | ||
from apps.phone_notifications.phone_backend import PhoneBackend | ||
from apps.telegram.client import TelegramClient | ||
from apps.telegram.models import TelegramVerificationCode | ||
from apps.twilioapp.phone_manager import PhoneManager | ||
from apps.twilioapp.twilio_client import twilio_client | ||
from apps.user_management.models import Team, User | ||
from common.api_helpers.exceptions import Conflict | ||
from common.api_helpers.mixins import FilterSerializerMixin, PublicPrimaryKeyMixin | ||
|
@@ -291,22 +297,44 @@ def timezone_options(self, request): | |
throttle_classes=[GetPhoneVerificationCodeThrottlerPerUser, GetPhoneVerificationCodeThrottlerPerOrg], | ||
) | ||
def get_verification_code(self, request, pk): | ||
|
||
logger.info("get_verification_code: validating reCAPTCHA code") | ||
# valid = recaptcha.check_recaptcha_internal_api(request, "mobile_verification_code") | ||
valid = check_recaptcha_internal_api(request, "mobile_verification_code") | ||
if not valid: | ||
logger.warning(f"get_verification_code: invalid reCAPTCHA validation") | ||
return Response("failed reCAPTCHA check", status=status.HTTP_400_BAD_REQUEST) | ||
logger.info('get_verification_code: pass reCAPTCHA validation"') | ||
|
||
user = self.get_object() | ||
phone_manager = PhoneManager(user) | ||
code_sent = phone_manager.send_verification_code() | ||
phone_backend = PhoneBackend() | ||
try: | ||
phone_backend.send_verification_sms(user) | ||
except NumberAlreadyVerified: | ||
return Response("Phone number already verified", status=status.HTTP_400_BAD_REQUEST) | ||
except FailedToStartVerification: | ||
return Response("Something went wrong while sending code", status=status.HTTP_500_INTERNAL_SERVER_ERROR) | ||
return Response(status=status.HTTP_200_OK) | ||
|
||
if not code_sent: | ||
logger.warning(f"Mobile app verification code was not successfully sent") | ||
return Response(status=status.HTTP_400_BAD_REQUEST) | ||
@action( | ||
detail=True, | ||
methods=["get"], | ||
throttle_classes=[GetPhoneVerificationCodeThrottlerPerUser, GetPhoneVerificationCodeThrottlerPerOrg], | ||
) | ||
def get_verification_call(self, request, pk): | ||
logger.info("get_verification_code_via_call: validating reCAPTCHA code") | ||
valid = check_recaptcha_internal_api(request, "mobile_verification_code") | ||
if not valid: | ||
logger.warning(f"get_verification_code_via_call: invalid reCAPTCHA validation") | ||
return Response("failed reCAPTCHA check", status=status.HTTP_400_BAD_REQUEST) | ||
logger.info('get_verification_code_via_call: pass reCAPTCHA validation"') | ||
|
||
user = self.get_object() | ||
phone_backend = PhoneBackend() | ||
try: | ||
phone_backend.make_verification_call(user) | ||
except NumberAlreadyVerified: | ||
return Response("Phone number already verified", status=status.HTTP_400_BAD_REQUEST) | ||
except FailedToStartVerification: | ||
return Response("Something went wrong while calling", status=status.HTTP_500_INTERNAL_SERVER_ERROR) | ||
Comment on lines
+351
to
+371
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. new endpoint? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, it is introduced keeping in mind that some providers can't sent sms, like asterisk. Twilio is also capable of verifying via call, but I didn't supported it to not overcomplicate refactoring. |
||
return Response(status=status.HTTP_200_OK) | ||
|
||
@action( | ||
|
@@ -320,29 +348,31 @@ def verify_number(self, request, pk): | |
if not code: | ||
return Response("Invalid verification code", status=status.HTTP_400_BAD_REQUEST) | ||
prev_state = target_user.insight_logs_serialized | ||
phone_manager = PhoneManager(target_user) | ||
verified, error = phone_manager.verify_phone_number(code) | ||
|
||
if not verified: | ||
return Response(error, status=status.HTTP_400_BAD_REQUEST) | ||
new_state = target_user.insight_logs_serialized | ||
write_resource_insight_log( | ||
instance=target_user, | ||
author=self.request.user, | ||
event=EntityEvent.UPDATED, | ||
prev_state=prev_state, | ||
new_state=new_state, | ||
) | ||
return Response(status=status.HTTP_200_OK) | ||
|
||
phone_backend = PhoneBackend() | ||
verified = phone_backend.verify_phone_number(target_user, code) | ||
if verified: | ||
new_state = target_user.insight_logs_serialized | ||
write_resource_insight_log( | ||
instance=target_user, | ||
author=self.request.user, | ||
event=EntityEvent.UPDATED, | ||
prev_state=prev_state, | ||
new_state=new_state, | ||
) | ||
return Response(status=status.HTTP_200_OK) | ||
else: | ||
return Response("Verification code is not correct", status=status.HTTP_400_BAD_REQUEST) | ||
|
||
@action(detail=True, methods=["put"]) | ||
def forget_number(self, request, pk): | ||
target_user = self.get_object() | ||
prev_state = target_user.insight_logs_serialized | ||
phone_manager = PhoneManager(target_user) | ||
forget = phone_manager.forget_phone_number() | ||
|
||
if forget: | ||
phone_backend = PhoneBackend() | ||
removed = phone_backend.forget_number(target_user) | ||
|
||
if removed: | ||
new_state = target_user.insight_logs_serialized | ||
write_resource_insight_log( | ||
instance=target_user, | ||
|
@@ -356,18 +386,17 @@ def forget_number(self, request, pk): | |
@action(detail=True, methods=["post"], throttle_classes=[TestCallThrottler]) | ||
def make_test_call(self, request, pk): | ||
user = self.get_object() | ||
phone_number = user.verified_phone_number | ||
|
||
if phone_number is None: | ||
return Response(status=status.HTTP_400_BAD_REQUEST) | ||
|
||
try: | ||
twilio_client.make_test_call(to=phone_number) | ||
except Exception as e: | ||
logger.error(f"Unable to make a test call due to {e}") | ||
phone_backend = PhoneBackend() | ||
phone_backend.make_test_call(user) | ||
except NumberNotVerified: | ||
return Response("Phone number is not verified", status=status.HTTP_400_BAD_REQUEST) | ||
except FailedToMakeCall: | ||
return Response( | ||
data="Something went wrong while making a test call", status=status.HTTP_500_INTERNAL_SERVER_ERROR | ||
"Something went wrong while making a test call", status=status.HTTP_500_INTERNAL_SERVER_ERROR | ||
) | ||
except ProviderNotSupports: | ||
return Response("Phone provider not supports phone calls", status=status.HTTP_500_INTERNAL_SERVER_ERROR) | ||
|
||
return Response(status=status.HTTP_200_OK) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
class FailedToMakeCall(Exception): | ||
pass | ||
|
||
|
||
class FailedToSendSMS(Exception): | ||
pass | ||
|
||
|
||
class NumberNotVerified(Exception): | ||
pass | ||
|
||
|
||
class NumberAlreadyVerified(Exception): | ||
pass | ||
|
||
|
||
class FailedToStartVerification(Exception): | ||
pass | ||
|
||
|
||
class FailedToFinishVerification(Exception): | ||
pass | ||
|
||
|
||
class ProviderNotSupports(Exception): | ||
pass | ||
|
||
|
||
class CallsLimitExceeded(Exception): | ||
pass | ||
|
||
|
||
class SMSLimitExceeded(Exception): | ||
pass |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
# Generated by Django 3.2.18 on 2023-04-08 07:40 | ||
|
||
from django.db import migrations, models | ||
import django.db.models.deletion | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
initial = True | ||
|
||
dependencies = [ | ||
('base', '0003_delete_organizationlogrecord'), | ||
('alerts', '0010_channelfilter_filtering_term_type'), | ||
('user_management', '0010_team_is_sharing_resources_to_all'), | ||
('twilioapp', '0003_auto_20230408_0711') | ||
] | ||
|
||
state_operations = [ | ||
migrations.CreateModel( | ||
name='OnCallPhoneCall', | ||
fields=[ | ||
('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), | ||
('exceeded_limit', models.BooleanField(default=None, null=True)), | ||
('status', models.PositiveSmallIntegerField(blank=True, choices=[(10, 'queued'), (20, 'ringing'), (30, 'in-progress'), (40, 'completed'), (50, 'busy'), (60, 'failed'), (70, 'no-answer'), (80, 'canceled')], null=True)), | ||
('sid', models.CharField(blank=True, max_length=50)), | ||
('created_at', models.DateTimeField(auto_now_add=True)), | ||
('grafana_cloud_notification', models.BooleanField(default=False)), | ||
('notification_policy', models.ForeignKey(default=None, null=True, on_delete=django.db.models.deletion.SET_NULL, to='base.usernotificationpolicy')), | ||
('receiver', models.ForeignKey(default=None, null=True, on_delete=django.db.models.deletion.CASCADE, to='user_management.user')), | ||
('represents_alert', models.ForeignKey(default=None, null=True, on_delete=django.db.models.deletion.SET_NULL, to='alerts.alert')), | ||
('represents_alert_group', models.ForeignKey(default=None, null=True, on_delete=django.db.models.deletion.SET_NULL, to='alerts.alertgroup')), | ||
], | ||
), | ||
migrations.CreateModel( | ||
name='OnCallSMS', | ||
fields=[ | ||
('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), | ||
('exceeded_limit', models.BooleanField(default=None, null=True)), | ||
('status', models.PositiveSmallIntegerField(blank=True, | ||
choices=[(10, 'accepted'), (20, 'queued'), (30, 'sending'), | ||
(40, 'sent'), (50, 'failed'), (60, 'delivered'), | ||
(70, 'undelivered'), (80, 'receiving'), | ||
(90, 'received'), (100, 'read')], null=True)), | ||
('grafana_cloud_notification', models.BooleanField(default=False)), | ||
('sid', models.CharField(blank=True, max_length=50)), | ||
('created_at', models.DateTimeField(auto_now_add=True)), | ||
('notification_policy', | ||
models.ForeignKey(default=None, null=True, on_delete=django.db.models.deletion.SET_NULL, | ||
to='base.usernotificationpolicy')), | ||
('receiver', models.ForeignKey(default=None, null=True, on_delete=django.db.models.deletion.CASCADE, | ||
to='user_management.user')), | ||
('represents_alert', | ||
models.ForeignKey(default=None, null=True, on_delete=django.db.models.deletion.SET_NULL, | ||
to='alerts.alert')), | ||
('represents_alert_group', | ||
models.ForeignKey(default=None, null=True, on_delete=django.db.models.deletion.SET_NULL, | ||
to='alerts.alertgroup')), | ||
], | ||
), | ||
] | ||
|
||
operations = [ | ||
migrations.SeparateDatabaseAndState(state_operations=state_operations) | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
from .phone_call import OnCallPhoneCall | ||
from .sms import OnCallSMS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that expected that we don't escape
< > &
anymore?https://github.com/grafana/oncall/pull/1713/files#diff-007b962c05ebca7bac34170b3d6fa519225c2bc6d652371dfc27ae018f942b73L193-L195
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this escape was Twilio Related, so it's moved to TwilioPhoneProvider https://github.com/grafana/oncall/pull/1713/files#diff-764f1941b04b97b1517990791c24bc6446e623f0326b95c44060dd4ec42afd75R26