diff --git a/CHANGELOG.md b/CHANGELOG.md index ad8911c1..2ac91fbd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Deprecate `/oncall` Slack command, update direct paging functionality by @vadimkerr ([#2537](https://github.com/grafana/oncall/pull/2537)) - Change plugin version to drop the `v` prefix. ([#2540](https://github.com/grafana/oncall/pull/2540)) +### Fixed + +- Fixed rendering of slack connection errors ([#2526](https://github.com/grafana/oncall/pull/2526)) + ## v1.3.13 (2023-07-17) ### Changed diff --git a/engine/apps/api/tests/test_auth.py b/engine/apps/api/tests/test_auth.py new file mode 100644 index 00000000..a0eee1f3 --- /dev/null +++ b/engine/apps/api/tests/test_auth.py @@ -0,0 +1,70 @@ +from unittest.mock import patch + +import pytest +from django.contrib.auth import REDIRECT_FIELD_NAME +from django.http import HttpResponse +from django.urls import reverse +from rest_framework import status +from rest_framework.test import APIClient + +from apps.auth_token.constants import SLACK_AUTH_TOKEN_NAME + + +@pytest.mark.django_db +@pytest.mark.parametrize( + "backend_name,expected_url", + ( + ("slack-login", "/a/grafana-oncall-app/users/me"), + ("slack-install-free", "/a/grafana-oncall-app/chat-ops"), + ), +) +def test_complete_slack_auth_redirect_ok( + make_organization, + make_user_for_organization, + make_slack_token_for_user, + backend_name, + expected_url, +): + organization = make_organization() + admin = make_user_for_organization(organization) + _, slack_token = make_slack_token_for_user(admin) + + client = APIClient() + url = ( + reverse("api-internal:complete-slack-auth", kwargs={"backend": backend_name}) + + f"?{SLACK_AUTH_TOKEN_NAME}={slack_token}" + ) + + with patch("apps.api.views.auth.do_complete") as mock_do_complete: + mock_do_complete.return_value = None + response = client.get(url) + + assert response.status_code == status.HTTP_302_FOUND + assert response.url == expected_url + + +@pytest.mark.django_db +def test_complete_slack_auth_redirect_error( + make_organization, + make_user_for_organization, + make_slack_token_for_user, +): + organization = make_organization() + admin = make_user_for_organization(organization) + _, slack_token = make_slack_token_for_user(admin) + + client = APIClient() + url = ( + reverse("api-internal:complete-slack-auth", kwargs={"backend": "slack-login"}) + + f"?{SLACK_AUTH_TOKEN_NAME}={slack_token}" + ) + + def _custom_do_complete(backend, *args, **kwargs): + backend.strategy.session[REDIRECT_FIELD_NAME] = "some-url" + return HttpResponse(status=status.HTTP_400_BAD_REQUEST) + + with patch("apps.api.views.auth.do_complete", side_effect=_custom_do_complete): + response = client.get(url) + + assert response.status_code == status.HTTP_302_FOUND + assert response.url == "some-url" diff --git a/engine/apps/api/views/auth.py b/engine/apps/api/views/auth.py index 582918b6..755bba43 100644 --- a/engine/apps/api/views/auth.py +++ b/engine/apps/api/views/auth.py @@ -3,7 +3,7 @@ from urllib.parse import urljoin from django.conf import settings from django.contrib.auth import REDIRECT_FIELD_NAME -from django.http import HttpResponseRedirect +from django.http import HttpResponse, HttpResponseRedirect from django.views.decorators.cache import never_cache from django.views.decorators.csrf import csrf_exempt from rest_framework.decorators import api_view, authentication_classes @@ -49,7 +49,7 @@ def overridden_complete_slack_auth(request, backend, *args, **kwargs): # if this was a user login/linking account, redirect to profile redirect_to = "/a/grafana-oncall-app/users/me" - do_complete( + result = do_complete( request.backend, _do_login, user=request.user, @@ -59,6 +59,13 @@ def overridden_complete_slack_auth(request, backend, *args, **kwargs): **kwargs, ) - # We build the frontend url using org url since multiple stacks could be connected to one backend. - return_to = urljoin(request.user.organization.grafana_url, redirect_to) + # handle potential errors in the strategy pipeline + return_to = None + if isinstance(result, HttpResponse): + # check if there was a redirect set in the session + return_to = request.backend.strategy.session.get(REDIRECT_FIELD_NAME) + + if return_to is None: + # We build the frontend url using org url since multiple stacks could be connected to one backend. + return_to = urljoin(request.user.organization.grafana_url, redirect_to) return HttpResponseRedirect(return_to) diff --git a/engine/apps/social_auth/pipeline.py b/engine/apps/social_auth/pipeline.py index 96aa4bc0..b69d447f 100644 --- a/engine/apps/social_auth/pipeline.py +++ b/engine/apps/social_auth/pipeline.py @@ -3,17 +3,14 @@ from urllib.parse import urljoin from django.apps import apps from django.conf import settings +from django.contrib.auth import REDIRECT_FIELD_NAME from django.http import HttpResponse from rest_framework import status from social_core.exceptions import AuthForbidden from apps.slack.tasks import populate_slack_channels_for_team, populate_slack_usergroups_for_team from apps.social_auth.exceptions import InstallMultiRegionSlackException -from common.constants.slack_auth import ( - REDIRECT_AFTER_SLACK_INSTALL, - SLACK_AUTH_SLACK_USER_ALREADY_CONNECTED_ERROR, - SLACK_AUTH_WRONG_WORKSPACE_ERROR, -) +from common.constants.slack_auth import SLACK_AUTH_SLACK_USER_ALREADY_CONNECTED_ERROR, SLACK_AUTH_WRONG_WORKSPACE_ERROR from common.insight_log import ChatOpsEvent, ChatOpsTypePlug, write_chatops_insight_log from common.oncall_gateway import check_slack_installation_possible, create_slack_connector @@ -41,26 +38,24 @@ def connect_user_to_slack(response, backend, strategy, user, organization, *args slack_team_identity = organization.slack_team_identity slack_user_id = response["authed_user"]["id"] + redirect_to = "/a/grafana-oncall-app/users/me/" + base_url_to_redirect = urljoin(organization.grafana_url, redirect_to) + if slack_team_identity is None: # means that organization doesn't have slack integration, so user cannot connect their account to slack return HttpResponse(status=status.HTTP_400_BAD_REQUEST) + if slack_team_identity.slack_id != response["team"]["id"]: # means that user authed in another slack workspace that is not connected to their organization # change redirect url to show user error message and save it in session param - url = urljoin( - strategy.session[REDIRECT_AFTER_SLACK_INSTALL], - f"?page=users&slack_error={SLACK_AUTH_WRONG_WORKSPACE_ERROR}", - ) - strategy.session[REDIRECT_AFTER_SLACK_INSTALL] = url + url = base_url_to_redirect + f"?slack_error={SLACK_AUTH_WRONG_WORKSPACE_ERROR}" + strategy.session[REDIRECT_FIELD_NAME] = url return HttpResponse(status=status.HTTP_400_BAD_REQUEST) if organization.users.filter(slack_user_identity__slack_id=slack_user_id).exists(): # means that slack user has already been connected to another user in current organization - url = urljoin( - strategy.session[REDIRECT_AFTER_SLACK_INSTALL], - f"?page=users&slack_error={SLACK_AUTH_SLACK_USER_ALREADY_CONNECTED_ERROR}", - ) - strategy.session[REDIRECT_AFTER_SLACK_INSTALL] = url + url = base_url_to_redirect + f"?slack_error={SLACK_AUTH_SLACK_USER_ALREADY_CONNECTED_ERROR}" + strategy.session[REDIRECT_FIELD_NAME] = url return HttpResponse(status=status.HTTP_400_BAD_REQUEST) slack_user_identity, _ = SlackUserIdentity.objects.get_or_create( diff --git a/engine/conftest.py b/engine/conftest.py index 36704ebb..00f9d579 100644 --- a/engine/conftest.py +++ b/engine/conftest.py @@ -44,7 +44,7 @@ from apps.api.permissions import ( LegacyAccessControlRole, RBACPermission, ) -from apps.auth_token.models import ApiAuthToken, PluginAuthToken +from apps.auth_token.models import ApiAuthToken, PluginAuthToken, SlackAuthToken from apps.base.models.user_notification_policy_log_record import ( UserNotificationPolicyLogRecord, listen_for_usernotificationpolicylogrecord_model_save, @@ -210,6 +210,14 @@ def make_mobile_app_auth_token_for_user(): return _make_mobile_app_auth_token_for_user +@pytest.fixture +def make_slack_token_for_user(): + def _make_slack_token_for_user(user): + return SlackAuthToken.create_auth_token(organization=user.organization, user=user) + + return _make_slack_token_for_user + + @pytest.fixture def make_public_api_token(): def _make_public_api_token(user, organization, name="test_api_token"): diff --git a/grafana-plugin/src/containers/Alerts/Alerts.tsx b/grafana-plugin/src/containers/Alerts/Alerts.tsx index 21a23a47..e8b19d66 100644 --- a/grafana-plugin/src/containers/Alerts/Alerts.tsx +++ b/grafana-plugin/src/containers/Alerts/Alerts.tsx @@ -68,15 +68,14 @@ export default function Alerts() { if (!showSlackInstallAlert && !showBannerTeam() && !showMismatchWarning() && !showChannelWarnings()) { return null; } - return (
{showSlackInstallAlert && ( {getSlackMessage( showSlackInstallAlert,