feat(codeowners): Add strict naming to user/team mappings for GH/GL (#28269)

src/sentry/api/bases/external_actor.py

@@ -9,7 +9,9 @@ from rest_framework.request import Request
 from sentry import features
 from sentry.api.serializers.rest_framework.base import CamelSnakeModelSerializer
 from sentry.api.validators.external_actor import (
+    is_valid_provider,
     validate_external_id_option,
+    validate_external_name,
     validate_integration_id,
 )
 from sentry.api.validators.integrations import validate_provider
@@ -23,6 +25,11 @@ AVAILABLE_PROVIDERS = {
     ExternalProviders.CUSTOM,
 }
 
+STRICT_NAME_PROVIDERS = {
+    ExternalProviders.GITHUB,
+    ExternalProviders.GITLAB,
+}
+
 
 class ExternalActorSerializerBase(CamelSnakeModelSerializer):  # type: ignore
     external_id = serializers.CharField(required=False, allow_null=True)
@@ -40,6 +47,13 @@ class ExternalActorSerializerBase(CamelSnakeModelSerializer):  # type: ignore
     def validate_external_id(self, external_id: str) -> Optional[str]:
         return validate_external_id_option(external_id)
 
+    def validate_external_name(self, external_name: str) -> str:
+        provider = self.initial_data.get("provider")
+        # Ensure the provider is strict, otherwise do not validate
+        if is_valid_provider(provider, STRICT_NAME_PROVIDERS):
+            return validate_external_name(external_name)
+        return external_name
+
     def validate_provider(self, provider_name_option: str) -> int:
         provider = validate_provider(provider_name_option, available_providers=AVAILABLE_PROVIDERS)
         return int(provider.value)

src/sentry/api/validators/external_actor.py

@@ -1,17 +1,35 @@
-from typing import Optional
+import re
+from typing import Optional, Set
 
 from rest_framework import serializers
 
+from sentry.api.exceptions import ParameterValidationError
+from sentry.api.validators.integrations import validate_provider
 from sentry.models import Organization, OrganizationIntegration
+from sentry.types.integrations import ExternalProviders
 
 EXTERNAL_ID_LENGTH_MIN = 1
 EXTERNAL_ID_LENGTH_MAX = 64
 
+EXTERNAL_NAME_LENGTH_MIN = 1
+EXTERNAL_NAME_LENGTH_MAX = 255
+EXTERNAL_NAME_REGEX = re.compile(r"^@[\w/\.-]+$")
+
 
 def _out_of_range_string(param: str, minimum: int, maximum: int, actual: int) -> str:
     return f"{param} has invalid length: {actual}. Length must be between {minimum} and {maximum}"
 
 
+def is_valid_provider(
+    provider: str, available_providers: Optional[Set[ExternalProviders]] = None
+) -> bool:
+    try:
+        validate_provider(provider, available_providers)
+    except ParameterValidationError:
+        return False
+    return True
+
+
 def validate_external_id_option(external_id: Optional[str]) -> Optional[str]:
     if not external_id:
         return None
@@ -29,6 +47,25 @@ def validate_external_id_option(external_id: Optional[str]) -> Optional[str]:
     return external_id
 
 
+def validate_external_name(external_name: str) -> str:
+    if not (EXTERNAL_NAME_LENGTH_MIN <= len(external_name) <= EXTERNAL_NAME_LENGTH_MAX):
+        raise serializers.ValidationError(
+            _out_of_range_string(
+                "External Name",
+                EXTERNAL_NAME_LENGTH_MIN,
+                EXTERNAL_NAME_LENGTH_MAX,
+                len(external_name),
+            )
+        )
+
+    if EXTERNAL_NAME_REGEX.match(external_name) is None:
+        raise serializers.ValidationError(
+            "External Name must start with '@' and can't contain special characters or spaces."
+        )
+
+    return external_name
+
+
 def validate_integration_id(integration_id: str, organization: Organization) -> str:
 
     integration_query = OrganizationIntegration.objects.filter(

static/app/components/integrationExternalMappingForm.tsx

@@ -1,4 +1,5 @@
 import {Component} from 'react';
+import styled from '@emotion/styled';
 import capitalize from 'lodash/capitalize';
 import pick from 'lodash/pick';
 
@@ -107,24 +108,32 @@ export default class IntegrationExternalMappingForm extends Component<Props> {
     const apiMethod = !baseEndpoint ? undefined : mapping ? 'PUT' : 'POST';
 
     return (
-      <Form
-        onSubmitSuccess={onSubmitSuccess}
-        initialData={this.initialData}
-        apiEndpoint={endpoint}
-        apiMethod={apiMethod}
-        onCancel={onCancel}
-        onSubmit={onSubmit}
-      >
-        {this.formFields.map(field => (
-          <FieldFromConfig
-            key={field.name}
-            field={field}
-            inline={false}
-            stacked
-            flexibleControlStateSize
-          />
-        ))}
-      </Form>
+      <FormWrapper>
+        <Form
+          requireChanges
+          onSubmitSuccess={onSubmitSuccess}
+          initialData={this.initialData}
+          apiEndpoint={endpoint}
+          apiMethod={apiMethod}
+          onCancel={onCancel}
+          onSubmit={onSubmit}
+        >
+          {this.formFields.map(field => (
+            <FieldFromConfig
+              key={field.name}
+              field={field}
+              inline={false}
+              stacked
+              flexibleControlStateSize
+            />
+          ))}
+        </Form>
+      </FormWrapper>
     );
   }
 }
+
+// Prevents errors from appearing off the modal
+const FormWrapper = styled('div')`
+  position: relative;
+`;

tests/acceptance/test_organization_integration_configuration_tabs.py

@@ -78,7 +78,7 @@ class OrganizationIntegrationConfigurationTabs(AcceptanceTestCase):
 
             # Add Mapping Modal
             externalName = self.browser.find_element_by_name("externalName")
-            externalName.send_keys("user2")
+            externalName.send_keys("@user2")
             self.browser.click("#userId:first-child div")
             self.browser.click('[id="react-select-2-option-1"]')
             self.browser.snapshot("integrations - save new external user mapping")

tests/sentry/api/serializers/test_external_actor.py

@@ -1,7 +1,12 @@
+from sentry.api.bases.external_actor import (
+    STRICT_NAME_PROVIDERS,
+    ExternalTeamSerializer,
+    ExternalUserSerializer,
+)
 from sentry.api.serializers import serialize
 from sentry.models import ExternalActor, Integration
 from sentry.testutils import TestCase
-from sentry.types.integrations import ExternalProviders
+from sentry.types.integrations import ExternalProviders, get_provider_name
 
 
 class ExternalActorSerializerTest(TestCase):
@@ -17,6 +22,7 @@ class ExternalActorSerializerTest(TestCase):
                 "installation_type": "born_as_bot",
             },
         )
+        self.org_integration = self.integration.add_organization(self.organization, self.user)
 
     def test_user(self):
         external_actor, _ = ExternalActor.objects.get_or_create(
@@ -55,3 +61,85 @@ class ExternalActorSerializerTest(TestCase):
         assert result["externalName"] == "Marcos"
         assert result["externalId"] == "Gaeta"
         assert result["teamId"] == str(team.id)
+
+    def test_strict_external_user_name(self):
+        # Ensure user names must start with @
+        external_actor_user_data = {
+            "provider": get_provider_name(ExternalProviders.GITHUB.value),
+            "externalName": "raz",
+            "integrationId": self.integration.id,
+            "userId": self.user.id,
+        }
+        serializer = ExternalUserSerializer(
+            data=external_actor_user_data,
+            context={"organization": self.organization},
+        )
+        assert serializer.is_valid() is False
+        assert "externalName" in serializer.errors
+
+        # Ensure longer user names are limited in length
+        external_actor_user_data["externalName"] = "@" + ("razputin_aquato" * 20)
+        serializer = ExternalUserSerializer(
+            data=external_actor_user_data,
+            context={"organization": self.organization},
+        )
+        assert serializer.is_valid() is False
+        assert "externalName" in serializer.errors
+
+        # Ensure proper user names are valid
+        external_actor_user_data["externalName"] = "@raz"
+        serializer = ExternalUserSerializer(
+            data=external_actor_user_data,
+            context={"organization": self.organization},
+        )
+        assert serializer.is_valid() is True
+
+    def test_strict_external_team_name(self):
+        team = self.create_team(organization=self.organization, members=[self.user])
+
+        # Ensure team names must start with @
+        external_actor_team_data = {
+            "provider": get_provider_name(ExternalProviders.GITHUB.value),
+            "externalName": "the-psychic-six",
+            "integrationId": self.integration.id,
+            "team_id": team.id,
+        }
+        serializer = ExternalTeamSerializer(
+            data=external_actor_team_data,
+            context={"organization": self.organization},
+        )
+        assert serializer.is_valid() is False
+        assert "externalName" in serializer.errors
+
+        # Ensure longer team names are limited in length
+        external_actor_team_data["externalName"] = "@" + ("the-psychic-six" * 20)
+        serializer = ExternalTeamSerializer(
+            data=external_actor_team_data,
+            context={"organization": self.organization},
+        )
+        assert serializer.is_valid() is False
+        assert "externalName" in serializer.errors
+
+        # Ensure proper team names are valid
+        external_actor_team_data["externalName"] = "@the-psychic-six"
+        serializer = ExternalTeamSerializer(
+            data=external_actor_team_data,
+            context={"organization": self.organization},
+        )
+        assert serializer.is_valid() is True
+
+    def test_avoid_strict_external_name(self):
+        # Strict rules should only run for strict providers
+        provider = get_provider_name(ExternalProviders.SLACK.value)
+        assert provider not in STRICT_NAME_PROVIDERS
+        external_actor_user_data = {
+            "provider": get_provider_name(ExternalProviders.SLACK.value),
+            "externalName": "ford-cruller",
+            "integrationId": self.integration.id,
+            "userId": self.user.id,
+        }
+        serializer = ExternalUserSerializer(
+            data=external_actor_user_data,
+            context={"organization": self.organization},
+        )
+        assert serializer.is_valid() is True