Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARC-1355 add jwt cookie #1234

Merged
merged 15 commits into from
Jun 6, 2022
Merged

ARC-1355 add jwt cookie #1234

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
a731465
ARC-1355: passing JWT as a cookie value to github/configure page
bgvozdev Jun 1, 2022
8f1e91c
ARC-1355: only take jiraHost from a subset of requests
bgvozdev Jun 1, 2022
c46150f
Add unit test to jiraHostMiddleware.ts
gxueatlassian Jun 1, 2022
c24859f
add more test
gxueatlassian Jun 1, 2022
2d7d745
wip
gxueatlassian Jun 1, 2022
6d7b46b
ARC-1355: more tests
bgvozdev Jun 1, 2022
6f986dc
minor yarn lint fix
gxueatlassian Jun 2, 2022
e10ef25
ARC-1355: a few more tests
bgvozdev Jun 2, 2022
9a8d091
ARC-1355: tests for cookie part in jwt
bgvozdev Jun 2, 2022
f6f6b41
Merge branch 'main' into ARC-1355-add-jwt-cookie
bgvozdev Jun 2, 2022
fa7a3ae
Add feature flags to backend and update tests
gxueatlassian Jun 2, 2022
bccbaed
ARC-1355: Add feature flag to the extract jwt from cookie code
gxueatlassian Jun 2, 2022
2848d9d
Merge branch 'main' into ARC-1355-add-jwt-cookie
gxueatlassian Jun 3, 2022
93d9b98
Merge branch 'main' into ARC-1355-add-jwt-cookie
gxueatlassian Jun 3, 2022
a3d5c0a
Merge branch 'main' into ARC-1355-add-jwt-cookie
bgvozdev Jun 6, 2022
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
3 changes: 2 additions & 1 deletion src/config/feature-flags.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ export enum BooleanFlags {
SEND_RELATED_COMMITS_WITH_DEPLOYMENT_ENTITIES = "send-related-commits-with-deployment-entities",
USE_NEW_GITHUB_CLIENT_FOR_PR_TITLE = "use-new-github-client-for-pr-title",
RETRY_ALL_ERRORS = "retry-all-errors",
GHE_SERVER = "ghe_server"
GHE_SERVER = "ghe_server",
SECURE_JIRAHOST_IN_COOKIES = "secure-jirahost-in-cookies"
}

export enum StringFlags {
Expand Down
46 changes: 44 additions & 2 deletions src/jira/util/jwt.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,12 @@ import { when } from "jest-when";
import { Request, Response } from "express";
import Mock = jest.Mock;
import { getLogger } from "config/logger";
import { booleanFlag as mockBooleanFlag } from "../../config/feature-flags";

jest.mock("./query-atlassian-connect-public-key");

jest.mock("../../config/feature-flags");

describe("jwt", () => {
let testQueryParams: Record<string, string>;
let res: Response;
Expand Down Expand Up @@ -65,10 +68,13 @@ describe("jwt", () => {
// we know exactly how express is going to behave
res = {
locals: {},
status: jest.fn().mockReturnValue(res),
json: jest.fn().mockReturnValue(res)
status: jest.fn(),
json: jest.fn()
} as any;

(res.status as Mock).mockReturnValue(res);
(res.json as Mock).mockReturnValue(res);

// TODO: need to remove all references to 'kabakumov' in this file, just use jiraHost instead
testQueryParams = {
xdm_e: "https://kabakumov.atlassian.net",
Expand All @@ -88,6 +94,8 @@ describe("jwt", () => {
},
log: getLogger("jwt.test")
} as any;

(mockBooleanFlag as any).mockReturnValue(Promise.resolve(true));
});

describe("#verifySymmetricJwtTokenMiddleware", () => {
Expand Down Expand Up @@ -219,13 +227,47 @@ describe("jwt", () => {
};
};

const buildRequestWithTokenInCookie = (): any => {
const jwtValue = encodeSymmetric({
qsh: "context-qsh",
iss: "jira"
}, testSecret);

return {
...baseRequest,
query: testQueryParams,
method: "POST",
headers: {},
cookies: {
jwt: jwtValue
}
};
};

it("Passes if token is in header", async () => {
const req = buildRequestWithTokenInHeader();
verifySymmetricJwtTokenMiddleware(testSecret, TokenType.context, req, res, next);
expect(res.status).toHaveBeenCalledTimes(0);
expect(next).toBeCalledTimes(1);
});

it("Passes if token is in cookies", async () => {
const req = buildRequestWithTokenInCookie();
verifySymmetricJwtTokenMiddleware(testSecret, TokenType.context, req, res, next);
expect(res.status).toHaveBeenCalledTimes(0);
expect(next).toBeCalledTimes(1);
});

it("Token in headers has priority over token in cookies", async () => {
const req = buildRequestWithTokenInHeader();
req.cookies = {
jwt: "JWT boom"
};
verifySymmetricJwtTokenMiddleware(testSecret, TokenType.context, req, res, next);
expect(res.status).not.toBeCalled();
expect(next).toBeCalledTimes(1);
});

it("Fails if there is no token", async () => {
const req = buildRequestWithNoToken();
verifySymmetricJwtTokenMiddleware(testSecret, TokenType.context, req, res, next);
Expand Down
20 changes: 16 additions & 4 deletions src/jira/util/jwt.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { NextFunction, Request, Response } from "express";
import { envVars } from "config/env";
import { queryAtlassianConnectPublicKey } from "./query-atlassian-connect-public-key";
import { includes, isEmpty } from "lodash";
import { booleanFlag, BooleanFlags } from "../../config/feature-flags";

const JWT_PARAM = "jwt";
const AUTH_HEADER = "authorization"; // the header name appears as lower-case
Expand All @@ -28,18 +29,22 @@ export enum TokenType {
context = "context"
}

export function extractJwtFromRequest(req: Request): string | undefined {
const tokenInQuery = req.query?.[JWT_PARAM];
let secureJiraHostInCookies;
Copy link
Contributor

Choose a reason for hiding this comment

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

This var will bleed between different requests

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what we want :) Once the FF is on, the value will eventually be updated (2nd call after the flip). Given the distributed nature of feature-flags, this is OK imho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, understand that, and the full story is:

  1. We want to have some feature flags to guard this code. But the middleware is NOT async at the moment, therefore can't use the feature flag (unless we make it async)
  2. I understand that making the middleware async should not have problem. But I also think turning middleware async vs have a global var here, the latter one is less risky.
  3. I understand the race condition of multi request, but I think they will all lead to expected result (which is "the flag will be used eventually" if turned on.

Unless you think there's still impact on this global var?



function extractJwtFromRequest(req: Request): string | undefined {

// JWT appears in both parameter and body will result query hash being invalid.
booleanFlag(BooleanFlags.SECURE_JIRAHOST_IN_COOKIES, false)
Copy link
Contributor

Choose a reason for hiding this comment

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

please use await instead

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to change the whole chain to be async @mboudreau cause it is pretty long.

.then(flag=>secureJiraHostInCookies = flag); //ignore error

const tokenInQuery = req.query?.[JWT_PARAM];
const tokenInBody = req.body?.[JWT_PARAM];
if (tokenInQuery && tokenInBody) {
req.log.info("JWT token can only appear in either query parameter or request body.");
return;
}
let token = tokenInQuery || tokenInBody;

// if there was no token in the query-string then fall back to checking the Authorization header
const authHeader = req.headers?.[AUTH_HEADER];
if (authHeader?.startsWith("JWT ")) {
if (token) {
Expand All @@ -50,6 +55,13 @@ export function extractJwtFromRequest(req: Request): string | undefined {
}
}

if (!token && secureJiraHostInCookies) {
token = req.cookies?.[JWT_PARAM];
if (token) {
req.log.info("JWT token found in cookies (last resort)");
}
}

// JWT is missing in query and we don't have a valid body.
if (!token) {
req.log.info("JWT token is missing in the request");
Expand Down
2 changes: 1 addition & 1 deletion src/middleware/jira-jwt-middleware.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { Installation } from "models/installation";
import { NextFunction, Request, Response } from "express";
import { sendError, TokenType, verifySymmetricJwtTokenMiddleware } from "../jira/util/jwt";

const verifyJiraJwtMiddleware = (tokenType: TokenType) => async (
export const verifyJiraJwtMiddleware = (tokenType: TokenType) => async (
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be exported

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, we kinda need that...

req: Request,
res: Response,
next: NextFunction
Expand Down
Loading