Skip to content

Commit

Permalink
Merge branch 'main' into ARC-2643-poco-policy-update
Browse files Browse the repository at this point in the history
  • Loading branch information
kamaksheeAtl authored Dec 6, 2023
2 parents a5e935b + d2eb1fb commit f94ef30
Show file tree
Hide file tree
Showing 50 changed files with 521 additions and 299 deletions.
5 changes: 1 addition & 4 deletions .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -125,10 +125,7 @@
"src/jira/**",
"src/models/**",
"src/sync/**",
"src/transforms/**",
"src/util/**",
"src/sqs/**",
"src/middleware/**"
"src/transforms/**"
],
"rules": {
// To be removed later
Expand Down
4 changes: 2 additions & 2 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ Create a new [GitHub App](https://github.com/settings/apps), setting the followi

- **GitHub App name**: Anything you want, but it must be unique across GitHub
- **Homepage URL**: `https://github.com/apps/GITHUB_APP_NAME` (The full URL to your GitHub App’s website)
- **Callback URL**: `https://DOMAIN/github/callback`
- **Callback URL**: `https://DOMAIN/rest/app/cloud/github-callback`
- **Setup URL**: `https://DOMAIN/github/setup`
- **Webhook URL**: `https://DOMAIN/github/webhooks`
- **Webhook Secret**: `development`
Expand Down Expand Up @@ -140,4 +140,4 @@ That being said, here are the steps needed to create a Pull Request for us to re
1. Commit and Push your changes - verify it passes all checks.
1. Submit your pull request with a detailed message about what's changed.
1. Wait for us to review and answer questions/make changes where requested.
1. Once merged, celebrate with your drink of choice because you've just helped thousands (if not millions) of people get a better experience in both Jira and GitHub! :beers:
1. Once merged, celebrate with your drink of choice because you've just helped thousands (if not millions) of people get a better experience in both Jira and GitHub! :beers:
1 change: 1 addition & 0 deletions github-for-jira.sd.yml
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,7 @@ environmentOverrides:
type: dedicated-rds
attributes:
parameters:
DBInstanceClass: db.t2.medium
# Adding read replica in staging as it's not enabled by default so we can read state
ReadReplica: true

Expand Down
6 changes: 3 additions & 3 deletions spa/src/services/oauth-manager/test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ describe("AuthManager", () => {
const mockRedirectUrlOnce = (state: string) => {
when(Api.auth.generateOAuthUrl)
.calledWith()
.mockResolvedValueOnce({ data: { redirectUrl: REDIRECT_URL, state: state } } as any);
}
.mockResolvedValueOnce({ data: { redirectUrl: REDIRECT_URL, state: state } } as never);
};

const mockExchangeTokenOnce= (code: string, state: string, accessToken: string) => {
when(Api.auth.exchangeToken)
.calledWith(code, state)
.mockResolvedValueOnce({ data: { accessToken } } as any);
.mockResolvedValueOnce({ data: { accessToken } } as never);
};

const onWinCloseAndBlock = { onWinClosed: () => {}, onPopupBlocked: () => {} };
Expand Down
5 changes: 2 additions & 3 deletions src/config/feature-flags.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,13 @@ export enum BooleanFlags {
ENABLE_5KU_BACKFILL_PAGE = "enable-5ku-experience-backfill-page",
USE_DYNAMODB_TO_PERSIST_AUDIT_LOG = "use-dynamodb-to-persist-audit-log",
USE_CUSTOM_ROOT_CA_BUNDLE = "use-custom-root-ca-bundle",
GENERATE_CORE_HEAP_DUMPS_ON_LOW_MEM = "generate-core-heap-dumps-on-low-mem"
GENERATE_CORE_HEAP_DUMPS_ON_LOW_MEM = "generate-core-heap-dumps-on-low-mem",
USE_RATELIMIT_ON_JIRA_CLIENT = "use-ratelimit-on-jira-client"
}

export enum StringFlags {
BLOCKED_INSTALLATIONS = "blocked-installations",
LOG_LEVEL = "log-level",
HEADERS_TO_ENCRYPT = "headers-to-encrypt",
SEND_ALL = "send-all"
}

Expand All @@ -40,7 +40,6 @@ export enum NumberFlags {
PREEMPTIVE_RATE_LIMIT_THRESHOLD = "preemptive-rate-limit-threshold",
NUMBER_OF_BUILD_PAGES_TO_FETCH_IN_PARALLEL = "number-of-build-to-fetch-in-parallel",
BACKFILL_PAGE_SIZE = "backfill-page-size",
BACKFILL_MAX_SUBTASKS = "backfill-max-subtasks",
INSTALLATION_TOKEN_CACHE_MAX_SIZE = "installation-token-cache-max-size"
}

Expand Down
1 change: 1 addition & 0 deletions src/config/metric-names.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export const sqsQueueMetrics = {
received: `${server}.sqs.queue.received`,
completed: `${server}.sqs.queue.success`,
failed: `${server}.sqs.queue.failed`,
exception: `${server}.sqs.queue.exception`,
sent: `${server}.sqs.queue.sent`,
deleted: `${server}.sqs.queue.deleted`,
duration: `${server}.sqs.queue.duration`
Expand Down
17 changes: 17 additions & 0 deletions src/jira/client/axios.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,4 +120,21 @@ describe("Jira axios instance", () => {
expect(error?.message).toEqual("Error executing Axios Request HTTP 404 - Bad REST path, or Jira instance not found, renamed or temporarily suspended.");
});

describe("when having a rate limited", () => {
it("should extract the retry after header if present", async () => {
const requestPayload = "TestRequestPayload";
jiraNock.post("/foo/bar", requestPayload)
.reply(429, "", {
"Retry-After": "100"
});

await expect(getAxiosInstance(jiraHost, "secret", getLogger("test")).post("/foo/bar", requestPayload))
.rejects.toMatchObject({
status: 429,
retryAfterInSeconds: 100
});

});
});

});
33 changes: 32 additions & 1 deletion src/jira/client/axios.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,30 @@ export class JiraClientError extends Error {
status?: number;
cause: AxiosError;

constructor(message: string, cause: AxiosError, status?: number) {
constructor(
message: string,
cause: AxiosError,
status: number | undefined
) {
super(message);
this.status = status;
this.cause = cause;
}
}

export class JiraClientRateLimitingError extends JiraClientError {
retryAfterInSeconds: number | undefined;
constructor(
message: string,
cause: AxiosError,
status: number | undefined,
retryAfterInSeconds: number | undefined
) {
super(message, cause, status);
this.retryAfterInSeconds = retryAfterInSeconds;
}
}

export const getJiraErrorMessages = (status: number) => {
switch (status) {
case 400:
Expand Down Expand Up @@ -86,9 +103,23 @@ const getErrorMiddleware = (logger: Logger) =>
logger.error({ err: error, res: error?.response }, errorMessage);
}

if (error.response?.status === 429) {
return Promise.reject(new JiraClientRateLimitingError(errorMessage, error, status, getRetryAfterInSec(error)));
}

return Promise.reject(new JiraClientError(errorMessage, error, status));
};

const getRetryAfterInSec = (error: AxiosError): number | undefined => {

const retryAfterInSecondsStr = error.response?.headers["retry-after"];
const retryAfterInSeconds = parseInt(retryAfterInSecondsStr || "unknown");

if (isNaN(retryAfterInSeconds)) return undefined;

return retryAfterInSeconds;
};

/**
* Middleware to enhance successful requests in Jira.
*
Expand Down
2 changes: 1 addition & 1 deletion src/middleware/frontend-log-middleware.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ describe("frontend-log-middleware", () => {

it("preserves old fields", async () => {
await LogMiddleware(request, response, next);
expect(request.log?.fields?.foo).toBe(123);
expect(request.log.fields?.foo).toBe(123);
});

describe("log level FF", () => {
Expand Down
8 changes: 5 additions & 3 deletions src/middleware/frontend-log-middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,14 @@ declare global {

export const LogMiddleware = async (req: Request<ParamsDictionary, unknown, { jiraHost?: string }>, res: Response, next: NextFunction): Promise<void> => {
req.log = getLogger("frontend-log-middleware", {
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unnecessary-condition
fields: req.log?.fields,
level: await stringFlag(StringFlags.LOG_LEVEL, defaultLogLevel, getUnvalidatedJiraHost(req)),
filterHttpRequests: true
});

req.addLogFields = (fields: Record<string, unknown>): void => {
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
if (req.log) {
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
req.log.fields = merge(req.log.fields, fields);
Expand All @@ -78,14 +79,15 @@ export const LogMiddleware = async (req: Request<ParamsDictionary, unknown, { ji
};

const getUnvalidatedJiraHost = (req: Request<ParamsDictionary, unknown, { jiraHost?: string }>): string | undefined =>
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
req.session?.jiraHost || extractUnsafeJiraHost(req);

/**
* Checks if the URL matches any of the URL patterns defined in `moduleUrls`
*/
const checkPathValidity = (url: string) => moduleUrls.some(moduleUrl => matchRouteWithPattern(moduleUrl, url));

const extractUnsafeJiraHost = (req: Request<ParamsDictionary, unknown, { jiraHost?: string }>): string | undefined => {
const extractUnsafeJiraHost = (req: Request<ParamsDictionary, unknown, { jiraHost?: string } | undefined>): string | undefined => {
if (checkPathValidity(req.path) && req.method == "GET") {
// Only save xdm_e query when on the GET post install url (iframe url)
return req.query.xdm_e as string;
Expand All @@ -95,7 +97,7 @@ const extractUnsafeJiraHost = (req: Request<ParamsDictionary, unknown, { jiraHos
return req.body?.jiraHost;
}

const cookies = req.cookies as { jiraHost?: string };
const cookies = req.cookies as { jiraHost?: string } | undefined;
if (cookies && cookies.jiraHost) {
return cookies.jiraHost;
}
Expand Down
31 changes: 17 additions & 14 deletions src/middleware/github-webhook-middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@ export const LOGGER_NAME = "github.webhooks";
// Returns an async function that reports errors errors to Sentry.
// This works similar to Sentry.withScope but works in an async context.
// A new Sentry hub is assigned to context.sentry and can be used later to add context to the error message.
const withSentry = function(callback: (context: WebhookContext<{ action?: string }>) => Promise<void>) {
const withSentry = function(callback: (context: WebhookContext<{ action?: string } | undefined>) => Promise<void>) {
return async (context: WebhookContext<{ action?: string }>) => {
context.sentry = new Sentry.Hub(Sentry.getCurrentHub().getClient());
context.sentry?.configureScope((scope) =>
context.sentry.configureScope((scope) =>
scope.addEventProcessor((event, hint) => AxiosErrorEventDecorator.decorate(event, hint))
);
context.sentry?.configureScope((scope) =>
context.sentry.configureScope((scope) =>
scope.addEventProcessor((event, hint) => SentryScopeProxy.processEvent(event, hint))
);

Expand All @@ -36,7 +36,7 @@ const withSentry = function(callback: (context: WebhookContext<{ action?: string
} catch (err: unknown) {
context.log.error({ err, context }, "Error while processing webhook");
emitWebhookFailedMetrics(extractWebhookEventNameFromContext(context), undefined);
context.sentry?.captureException(err);
context.sentry.captureException(err);
throw err;
}
};
Expand All @@ -60,15 +60,15 @@ const isStateChangeBranchCreateOrDeploymentAction = (action: string) =>
action
);

const extractWebhookEventNameFromContext = (context: WebhookContext<{ action?: string }>): string => {
const extractWebhookEventNameFromContext = (context: WebhookContext<{ action?: string } | undefined>): string => {
let webhookEvent = context.name;
if (context.payload?.action) {
webhookEvent = `${webhookEvent}.${context.payload.action}`;
}
return webhookEvent;
};

const moreWebhookSpecificTags = (webhookContext: WebhookContext<{ deployment_status?: { state?: string } }>): Record<string, string | undefined> => {
const moreWebhookSpecificTags = (webhookContext: WebhookContext<{ deployment_status?: { state?: string } } | undefined>): Record<string, string | undefined> => {
if (webhookContext.name === "deployment_status") {
return {
deploymentStatusState: webhookContext.payload?.deployment_status?.state
Expand All @@ -88,7 +88,7 @@ export const GithubWebhookMiddleware = (
sender?: { type?: string; id?: number; login?: string };
action?: string;
deployment_status?: { state?: string };
}>): Promise<void> => {
} | undefined>): Promise<void> => {
const webhookEvent = extractWebhookEventNameFromContext(context);

// Metrics for webhook payload size
Expand All @@ -102,7 +102,7 @@ export const GithubWebhookMiddleware = (
action: context.payload?.action,
id: context.id,
repo: context.payload?.repository ? {
owner: context.payload.repository?.owner?.login,
owner: context.payload.repository.owner?.login,
repo: context.payload.repository.name
} : undefined,
payload: context.payload,
Expand All @@ -116,7 +116,9 @@ export const GithubWebhookMiddleware = (
context.log.info("Halting further execution since no installation id found.");
return;
}
const gitHubInstallationId = Number(payload?.installation?.id);
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
const gitHubInstallationId = Number(payload.installation?.id);
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
const gitHubAppId = context.gitHubAppConfig?.gitHubAppId;

const subscriptions = await Subscription.getAllForInstallation(gitHubInstallationId, gitHubAppId);
Expand All @@ -140,7 +142,7 @@ export const GithubWebhookMiddleware = (

const gitHubProduct = getCloudOrServerFromGitHubAppId(gitHubAppId);

const action = String(payload?.action);
const action = String(payload.action);
statsd.increment(metricWebhooks.webhookEvent, {
name: "webhooks",
event: name,
Expand All @@ -152,22 +154,22 @@ export const GithubWebhookMiddleware = (
// Edit actions are not allowed because they trigger this Jira integration to write data in GitHub and can trigger events, causing an infinite loop.
// State change actions are allowed because they're one-time actions, therefore they won’t cause a loop.
if (
context.payload?.sender?.type === "Bot" &&
payload.sender?.type === "Bot" &&
!isStateChangeBranchCreateOrDeploymentAction(action) &&
!isStateChangeBranchCreateOrDeploymentAction(context.name)
) {
context.log.info(
{
noop: "bot",
botId: context.payload?.sender?.id,
botLogin: context.payload?.sender?.login
botId: payload.sender.id,
botLogin: payload.sender.login
},
"Halting further execution since the sender is a bot and action is not a state change nor a deployment"
);
return;
}

if (isFromIgnoredRepo(context.payload)) {
if (isFromIgnoredRepo(payload)) {
context.log.info(
{
installation_id: context.payload?.installation?.id,
Expand Down Expand Up @@ -232,6 +234,7 @@ export const GithubWebhookMiddleware = (
const jiraClient = await getJiraClient(
jiraHost,
gitHubInstallationId,
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
context.gitHubAppConfig?.gitHubAppId,
context.log
);
Expand Down
Loading

0 comments on commit f94ef30

Please sign in to comment.