diff --git a/src/sentry/core/endpoints/scim/constants.py b/src/sentry/core/endpoints/scim/constants.py index e7ca0317b69b0c..02d984d196ea67 100644 --- a/src/sentry/core/endpoints/scim/constants.py +++ b/src/sentry/core/endpoints/scim/constants.py @@ -23,11 +23,13 @@ class SCIMErrorResponse(TypedDict, total=False): SCIM_404_USER_RES = { "schemas": [SCIM_API_ERROR], + "status": "404", "detail": "User not found.", } SCIM_404_GROUP_RES = { "schemas": [SCIM_API_ERROR], + "status": "404", "detail": "Group not found.", } @@ -37,26 +39,31 @@ class SCIMErrorResponse(TypedDict, total=False): SCIM_400_INVALID_FILTER = { "schemas": [SCIM_API_ERROR], + "status": "400", "scimType": "invalidFilter", } SCIM_400_INTEGRITY_ERROR = { "schemas": [SCIM_API_ERROR], + "status": "400", "detail": "Database Integrity Error.", } SCIM_400_TOO_MANY_PATCH_OPS_ERROR = { "schemas": [SCIM_API_ERROR], + "status": "400", "detail": "Too many patch ops sent, limit is 100.", } SCIM_400_UNSUPPORTED_ATTRIBUTE = { "schemas": [SCIM_API_ERROR], + "status": "400", "detail": "Invalid Replace attr. Only displayName and members supported.", } SCIM_400_INVALID_PAYLOAD = { "schemas": [SCIM_API_ERROR], + "status": "400", "detail": "Invalid SCIM payload.", } diff --git a/src/sentry/core/endpoints/scim/members.py b/src/sentry/core/endpoints/scim/members.py index cb74e0fc300745..364b53c5980830 100644 --- a/src/sentry/core/endpoints/scim/members.py +++ b/src/sentry/core/endpoints/scim/members.py @@ -618,8 +618,12 @@ def post( ("is already a member" in error) for error in serializer.errors["email"] ): # we include conflict logic in the serializer, check to see if that was - # our error and if so, return a 409 so the scim IDP knows how to handle - raise SCIMApiError(detail=SCIM_409_USER_EXISTS, status_code=409) + # our error and if so, return a 409 so the scim IDP knows how to handle. + # "uniqueness" tells a spec-compliant IdP this is a duplicate so it can + # fall back from POST to PATCH/PUT against the existing member. + raise SCIMApiError( + detail=SCIM_409_USER_EXISTS, status_code=409, scim_type="uniqueness" + ) if "role" in serializer.errors: # TODO: Change this to an error pointing to a doc showing the workaround if they # tried to provision an org admin diff --git a/src/sentry/core/endpoints/scim/teams.py b/src/sentry/core/endpoints/scim/teams.py index 6ec1bb974121da..593daf40f8812d 100644 --- a/src/sentry/core/endpoints/scim/teams.py +++ b/src/sentry/core/endpoints/scim/teams.py @@ -295,12 +295,10 @@ def post( sender=None, ) except (IntegrityError, MaxSnowflakeRetryError): - return Response( - { - "non_field_errors": [CONFLICTING_SLUG_ERROR], - "detail": CONFLICTING_SLUG_ERROR, - }, - status=409, + # A duplicate slug is a uniqueness collision; surface it as a + # spec-compliant SCIM error so IdPs can reconcile the existing team. + raise SCIMApiError( + detail=CONFLICTING_SLUG_ERROR, status_code=409, scim_type="uniqueness" ) self.create_audit_entry( diff --git a/src/sentry/core/endpoints/scim/utils.py b/src/sentry/core/endpoints/scim/utils.py index d1133c262ef653..6d37e452a0ac08 100644 --- a/src/sentry/core/endpoints/scim/utils.py +++ b/src/sentry/core/endpoints/scim/utils.py @@ -24,11 +24,18 @@ class SCIMApiError(APIException): - def __init__(self, detail, status_code=400): + def __init__(self, detail, status_code=400, scim_type=None): transaction = sentry_sdk.get_current_scope().transaction if transaction is not None: transaction.set_tag("http.status_code", status_code) - super().__init__({"schemas": [SCIM_API_ERROR], "detail": detail}) + error_body = { + "schemas": [SCIM_API_ERROR], + "status": str(status_code), + "detail": detail, + } + if scim_type is not None: + error_body["scimType"] = scim_type + super().__init__(error_body) self.status_code = status_code diff --git a/tests/sentry/core/endpoints/scim/test_scim_team_details.py b/tests/sentry/core/endpoints/scim/test_scim_team_details.py index f2e185a305fb8e..6e6c4997ac37be 100644 --- a/tests/sentry/core/endpoints/scim/test_scim_team_details.py +++ b/tests/sentry/core/endpoints/scim/test_scim_team_details.py @@ -25,6 +25,7 @@ def test_team_details_404(self) -> None: assert response.data == { "detail": "Group not found.", + "status": "404", "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], } @@ -420,6 +421,7 @@ def test_team_member_doesnt_exist_add_to_team(self) -> None: ) assert response.data == { "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "status": "404", "detail": "User not found.", } @@ -432,6 +434,7 @@ def test_team_details_patch_too_many_ops(self) -> None: assert response.status_code == 400, response.data assert response.data == { "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "status": "400", "detail": "Too many patch ops sent, limit is 100.", } @@ -448,6 +451,7 @@ def test_team_details_invalid_filter_patch_route(self) -> None: assert response.data == { "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "status": "400", "scimType": "invalidFilter", } @@ -464,6 +468,7 @@ def test_add_members_invalid_value_format(self) -> None: ) assert response.data == { "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "status": "400", "detail": "Invalid SCIM payload.", } @@ -480,6 +485,7 @@ def test_add_members_non_numeric_value(self) -> None: ) assert response.data == { "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "status": "400", "detail": "Invalid SCIM payload.", } @@ -496,6 +502,7 @@ def test_replace_members_invalid_value_format(self) -> None: ) assert response.data == { "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "status": "400", "detail": "Invalid SCIM payload.", } @@ -512,6 +519,7 @@ def test_replace_members_non_numeric_value(self) -> None: ) assert response.data == { "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "status": "400", "detail": "Invalid SCIM payload.", } diff --git a/tests/sentry/core/endpoints/scim/test_scim_team_index.py b/tests/sentry/core/endpoints/scim/test_scim_team_index.py index d889488f366c14..7efa2443188f8d 100644 --- a/tests/sentry/core/endpoints/scim/test_scim_team_index.py +++ b/tests/sentry/core/endpoints/scim/test_scim_team_index.py @@ -178,6 +178,7 @@ def test_scim_invalid_filter(self) -> None: assert response.status_code == 400, response.data assert response.data == { "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "status": "400", "scimType": "invalidFilter", } @@ -230,7 +231,12 @@ def test_scim_team_no_duplicate_names(self) -> None: response = self.get_error_response( self.organization.slug, **self.post_data, status_code=409 ) - assert response.data["detail"] == "A team with this slug already exists." + assert response.data == { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "scimType": "uniqueness", + "status": "409", + "detail": "A team with this slug already exists.", + } def test_scim_team_invalid_numeric_slug(self) -> None: invalid_post_data = {**self.post_data, "displayName": "1234"} diff --git a/tests/sentry/core/endpoints/scim/test_scim_user_details.py b/tests/sentry/core/endpoints/scim/test_scim_user_details.py index e731ff49f3f649..e64f0515e7380b 100644 --- a/tests/sentry/core/endpoints/scim/test_scim_user_details.py +++ b/tests/sentry/core/endpoints/scim/test_scim_user_details.py @@ -595,6 +595,7 @@ def test_patch_bad_schema(self) -> None: ) assert response.data == { "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "status": "400", "detail": "Invalid Patch Operation.", } @@ -604,6 +605,7 @@ def test_patch_bad_schema(self) -> None: ) assert response.data == { "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "status": "400", "detail": "Invalid Patch Operation.", } @@ -620,6 +622,7 @@ def test_member_detail_patch_too_many_ops(self) -> None: assert response.status_code == 400, response.data assert response.data == { "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "status": "400", "detail": '{"Operations":["Ensure this field has no more than 100 elements."]}', } diff --git a/tests/sentry/core/endpoints/scim/test_scim_user_index.py b/tests/sentry/core/endpoints/scim/test_scim_user_index.py index a85ad0e1e55c1a..4b9475d6a4d94c 100644 --- a/tests/sentry/core/endpoints/scim/test_scim_user_index.py +++ b/tests/sentry/core/endpoints/scim/test_scim_user_index.py @@ -180,11 +180,28 @@ def test_post_users_already_exists(self) -> None: assert response.status_code == 409, response.content assert response.data == { "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "scimType": "uniqueness", + "status": "409", "detail": "User already exists in the database.", } assert not member.flags["idp:provisioned"] assert not member.flags["idp:role-restricted"] + def test_scim_error_includes_status_without_scim_type(self) -> None: + # Non-uniqueness SCIM errors must still carry a string `status` per + # RFC 7644 ยง3.12, but must not claim a `scimType` they don't have. + data = post_data() + data["sentryOrgRole"] = "nonexistant" + response = self.get_error_response( + self.organization.slug, method="post", status_code=400, **data + ) + assert response.data == { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "status": "400", + "detail": "Invalid organization role.", + } + assert "scimType" not in response.data + def test_post_users_with_role_valid(self) -> None: data = post_data() data["sentryOrgRole"] = "manager" @@ -268,6 +285,7 @@ def test_post_users_with_role_invalid(self) -> None: ) assert resp.data == { "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "status": "400", "detail": "Invalid organization role.", } @@ -278,6 +296,7 @@ def test_post_users_with_role_invalid(self) -> None: ) assert resp.data == { "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "status": "400", "detail": "Invalid organization role.", }