Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 54 additions & 1 deletion src/sentry/integrations/pagerduty/integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,11 @@
from django.http.request import HttpRequest
from django.http.response import HttpResponseBase
from django.utils.translation import gettext_lazy as _
from rest_framework import serializers
from rest_framework.fields import CharField

from sentry import options
from sentry.api.serializers.rest_framework.base import CamelSnakeSerializer
from sentry.integrations.base import (
FeatureDescription,
IntegrationData,
Expand All @@ -27,7 +30,8 @@
from sentry.integrations.pipeline import IntegrationPipeline
from sentry.integrations.types import IntegrationProviderSlug
from sentry.organizations.services.organization.model import RpcOrganization
from sentry.pipeline.views.base import PipelineView
from sentry.pipeline.types import PipelineStepResult
from sentry.pipeline.views.base import ApiPipelineSteps, PipelineView
from sentry.shared_integrations.exceptions import IntegrationError
from sentry.utils.http import absolute_uri

Expand Down Expand Up @@ -186,6 +190,52 @@ def services(self) -> list[PagerDutyServiceDict]:
return []


class PagerDutyInstallationData(TypedDict):
config: str


class PagerDutyInstallationApiSerializer(CamelSnakeSerializer[PagerDutyInstallationData]):
config = CharField(required=True)

def validate_config(self, value: str) -> str:
try:
orjson.loads(value)
except orjson.JSONDecodeError:
raise serializers.ValidationError("Invalid JSON configuration data.")
return value


class PagerDutyInstallationApiStep:
"""API-mode step for PagerDuty integration setup.

PagerDuty uses an app install redirect flow: the user is sent to PagerDuty's
install page, and the callback returns a JSON config param containing account
info and integration keys.
"""

step_name = "installation_redirect"

def _get_app_url(self) -> str:
app_id = options.get("pagerduty.app-id")
setup_url = absolute_uri("/extensions/pagerduty/setup/")
return f"https://app.pagerduty.com/install/integration?app_id={app_id}&redirect_url={setup_url}&version=2"

def get_step_data(self, pipeline: IntegrationPipeline, request: HttpRequest) -> dict[str, str]:
return {"installUrl": self._get_app_url()}

def get_serializer_cls(self) -> type:
return PagerDutyInstallationApiSerializer

def handle_post(
self,
validated_data: dict[str, str],
pipeline: IntegrationPipeline,
request: HttpRequest,
) -> PipelineStepResult:
pipeline.bind_state("config", validated_data["config"])
return PipelineStepResult.advance()


class PagerDutyIntegrationProvider(IntegrationProvider):
key = IntegrationProviderSlug.PAGERDUTY.value
name = "PagerDuty"
Expand All @@ -198,6 +248,9 @@ class PagerDutyIntegrationProvider(IntegrationProvider):
def get_pipeline_views(self) -> Sequence[PipelineView[IntegrationPipeline]]:
return [PagerDutyInstallationRedirect()]

def get_pipeline_api_steps(self) -> ApiPipelineSteps[IntegrationPipeline]:
return [PagerDutyInstallationApiStep()]

def post_install(
self,
integration: Integration,
Expand Down
110 changes: 110 additions & 0 deletions static/app/components/pipeline/pipelineIntegrationPagerDuty.spec.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
import {act, render, screen, userEvent} from 'sentry-test/reactTestingLibrary';

import {pagerDutyIntegrationPipeline} from './pipelineIntegrationPagerDuty';
import type {PipelineStepProps} from './types';

const PagerDutyInstallStep = pagerDutyIntegrationPipeline.steps[0].component;

function makeStepProps<D, A>(
overrides: Partial<PipelineStepProps<D, A>> & {stepData: D}
): PipelineStepProps<D, A> {
return {
advance: jest.fn(),
advanceError: null,
isAdvancing: false,
stepIndex: 0,
totalSteps: 1,
...overrides,
};
}

let mockPopup: Window;

function dispatchPipelineMessage({
data,
origin = document.location.origin,
source = mockPopup,
}: {
data: Record<string, string>;
origin?: string;
source?: Window | MessageEventSource | null;
}) {
act(() => {
const event = new MessageEvent('message', {data, origin});
Object.defineProperty(event, 'source', {value: source});
window.dispatchEvent(event);
});
}

beforeEach(() => {
mockPopup = {
closed: false,
close: jest.fn(),
focus: jest.fn(),
} as unknown as Window;
jest.spyOn(window, 'open').mockReturnValue(mockPopup);
});

afterEach(() => {
jest.restoreAllMocks();
});

describe('PagerDutyInstallStep', () => {
it('renders the install step for PagerDuty', () => {
render(
<PagerDutyInstallStep
{...makeStepProps({
stepData: {installUrl: 'https://app.pagerduty.com/install/integration'},
})}
/>
);

expect(
screen.getByRole('button', {name: 'Install PagerDuty App'})
).toBeInTheDocument();
});

it('calls advance with config on callback', async () => {
const advance = jest.fn();
render(
<PagerDutyInstallStep
{...makeStepProps({
stepData: {installUrl: 'https://app.pagerduty.com/install/integration'},
advance,
})}
/>
);

await userEvent.click(screen.getByRole('button', {name: 'Install PagerDuty App'}));

dispatchPipelineMessage({
data: {
_pipeline_source: 'sentry-pipeline',
config: '{"account":{"name":"Test","subdomain":"test"}}',
},
});

expect(advance).toHaveBeenCalledWith({
config: '{"account":{"name":"Test","subdomain":"test"}}',
});
});

it('shows loading state when isAdvancing is true', () => {
render(
<PagerDutyInstallStep
{...makeStepProps({
stepData: {installUrl: 'https://app.pagerduty.com/install/integration'},
isAdvancing: true,
})}
/>
);

expect(screen.getByRole('button', {name: 'Installing...'})).toBeDisabled();
});

it('disables install button when installUrl is not provided', () => {
render(<PagerDutyInstallStep {...makeStepProps({stepData: {}})} />);

expect(screen.getByRole('button', {name: 'Install PagerDuty App'})).toBeDisabled();
});
});
88 changes: 88 additions & 0 deletions static/app/components/pipeline/pipelineIntegrationPagerDuty.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
import {useCallback} from 'react';

import {Button} from '@sentry/scraps/button';
import {Stack} from '@sentry/scraps/layout';
import {Text} from '@sentry/scraps/text';

import {t} from 'sentry/locale';
import type {IntegrationWithConfig} from 'sentry/types/integrations';

import {useRedirectPopupStep} from './shared/useRedirectPopupStep';
import type {PipelineDefinition, PipelineStepProps} from './types';
import {pipelineComplete} from './types';

function PagerDutyInstallStep({
stepData,
advance,
isAdvancing,
}: PipelineStepProps<{installUrl?: string}, {config: string}>) {
const handleCallback = useCallback(
(data: Record<string, string>) => {
advance({config: data.config ?? ''});
},
[advance]
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Callback advances with empty string on missing config

Low Severity

The handleCallback unconditionally calls advance({config: data.config ?? ''}), sending an empty string to the backend if data.config is undefined. Other integrations like Bitbucket guard against missing data by checking before advancing. While the backend JSON validation catches this, the user gets a confusing validation error instead of the step simply waiting for valid callback data.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 6f1e1b5. Configure here.


const {openPopup, popupStatus} = useRedirectPopupStep({
redirectUrl: stepData.installUrl,
onCallback: handleCallback,
popup: {height: 900},
});

return (
<Stack gap="lg" align="start">
<Stack gap="sm">
<Text>
{t(
'Install the Sentry app on your PagerDuty account to complete the integration setup.'
)}
</Text>
{popupStatus === 'popup-open' && (
<Text variant="muted" size="sm">
{t('A popup should have opened to install the PagerDuty app.')}
</Text>
)}
{popupStatus === 'failed-to-open' && (
<Text variant="danger" size="sm">
{t(
'The installation popup was blocked by your browser. Please ensure popups are allowed and try again.'
)}
</Text>
)}
</Stack>
{isAdvancing ? (
<Button size="sm" disabled>
{t('Installing...')}
</Button>
) : popupStatus === 'popup-open' ? (
<Button size="sm" onClick={openPopup}>
{t('Reopen installation window')}
</Button>
) : (
<Button
size="sm"
priority="primary"
onClick={openPopup}
disabled={!stepData.installUrl}
>
{t('Install PagerDuty App')}
</Button>
)}
</Stack>
);
}

export const pagerDutyIntegrationPipeline = {
type: 'integration',
provider: 'pagerduty',
actionTitle: t('Installing PagerDuty Integration'),
getCompletionData: pipelineComplete<IntegrationWithConfig>,
completionView: null,
steps: [
{
stepId: 'installation_redirect',
shortDescription: t('Installing PagerDuty app'),
component: PagerDutyInstallStep,
},
],
} as const satisfies PipelineDefinition;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PagerDuty pipeline not registered in frontend registry

High Severity

pagerDutyIntegrationPipeline is exported from pipelineIntegrationPagerDuty.tsx but never imported or added to PIPELINE_REGISTRY in registry.tsx. Every other integration (AWS Lambda, Bitbucket, GitHub, GitLab, Slack) is registered there. Without this entry, getPipelineDefinition('integration', 'pagerduty') will throw at runtime, making the API-driven pipeline completely non-functional on the frontend.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 6f1e1b5. Configure here.

Loading
Loading