Skip to content

Commit

Permalink
fix: google login works in staging
Browse files Browse the repository at this point in the history
* The major change here is supporting /src/http/* handlers as isolated modules. They don't need to be isolated in sandbox, so it worked. When deploying to lambda they get isolated and everything broke. So using code sharing (https://arc.codes/docs/en/guides/developer-experience/sharing-code)
  • Loading branch information
activescott committed Jan 16, 2021
1 parent 826e91f commit 3cc0c10
Show file tree
Hide file tree
Showing 42 changed files with 119 additions and 102 deletions.
7 changes: 0 additions & 7 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,17 +1,10 @@
node_modules/
server/src/http/**/*.js
server/src/lib/**/*.js
server/test/**/*.js
*.jic
.arc-env
sam.json
sam.yaml
/server/coverage/
/client/coverage/
# react-app compiled assets (these are regengerated by re-building src/react-app/
/server/public/
/server/preferences.arc
!/server/public/readme.md
/client/src/**/*.js
/client/public/static/vendor/svg-injector/
/client/public/static/vendor/bootstrap/
Expand Down
2 changes: 1 addition & 1 deletion client/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
"scripts": {
"start": "react-scripts start",
"build": "react-scripts build",
"postbuild": "mkdir -p ../server/public ; cp -R ./build/* ../server/public/",
"postbuild": "rm -rf ../server/public ; mkdir -p ../server/public ; cp -R ./build/* ../server/public/",
"test": "react-scripts test --watchAll=false",
"dev": "react-scripts test --watchAll=true"
},
Expand Down
6 changes: 6 additions & 0 deletions server/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/src/**/*.js
/test/**/*.js
/coverage/
/public/
/preferences.arc
!/public/readme.md
2 changes: 1 addition & 1 deletion server/jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ module.exports = {
globalTeardown: "./test/support/globalTeardown.ts",
globals: {
"ts-jest": {
tsconfig: "tsconfig.json",
tsconfig: "tsconfig.prod.json",
},
},
collectCoverageFrom: ["src/**/*.ts", "!src/react-app/**"],
Expand Down
6 changes: 6 additions & 0 deletions server/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 5 additions & 4 deletions server/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,23 +8,24 @@
"build-production": "npm run -s build-react-production && npm run -s build-server",
"build-staging": "npm run -s build-react-staging && npm run -s build-server",
"build-testing": "npm run -s build-production",
"build-react-staging": "pushd . && cd ../client && PUBLIC_URL=/staging npm run build && popd",
"build-react-staging": "pushd . && cd ../client && PUBLIC_URL='' npm run build && popd",
"build-react-production": "pushd . && cd ../client && PUBLIC_URL='' npm run build && popd",
"build-react-testing": "npm run -s build-react-production",
"build-server": "./node_modules/.bin/tsc --project tsconfig.prod.json",
"build-server": "./node_modules/.bin/arc hydrate && ./node_modules/.bin/tsc --project tsconfig.prod.json",
"start": "npm run build-testing && ./node_modules/.bin/arc sandbox",
"clean": "find ./src -type f -name '*.js' -delete; find ./test -type f -name '*.js' -delete",
"//deploy": "PUBLIC_URL for react-app static assets. If more variables are needed, switch to using env-cmd",
"deploy": "echo 'Please execute `npm run deploy-production` or `npm run deploy-staging`\n'",
"deploy-production": "PUBLIC_URL='' npm run build && ./node_modules/.bin/arc deploy",
"deploy-staging": "PUBLIC_URL='/staging' npm run build-staging && ./node_modules/.bin/arc deploy",
"deploy-production": "run build-production && ./node_modules/.bin/arc deploy production",
"deploy-staging": "npm run build-staging && ./node_modules/.bin/arc deploy staging --verbose",
"test": "./node_modules/.bin/jest --coverage",
"dev": "./node_modules/.bin/jest --watch"
},
"author": "Scott Willeke <scott@willeke.com>",
"license": "ISC",
"devDependencies": {
"@architect/architect": "^8.4.5",
"@tsconfig/node12": "^1.0.7",
"@types/cookie": "^0.4.0",
"@types/jest": "^26.0.19",
"@types/node": "^14.14.19",
Expand Down
2 changes: 1 addition & 1 deletion server/src/http/get-api-echo/index.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {
ArchitectHttpRequestPayload,
ArchitectHttpResponsePayload,
} from "../../types/http"
} from "@architect/shared/types/http"
import * as arc from "@architect/functions"

const handlerImp = async function handlerImp(
Expand Down
2 changes: 1 addition & 1 deletion server/src/http/get-auth-login-000provider/index.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import * as arc from "@architect/functions"
import login from "../../lib/architect/oauth/handlers/login"
import login from "@architect/shared/architect/oauth/handlers/login"

export const handler = arc.http.async(login)
4 changes: 2 additions & 2 deletions server/src/http/get-auth-me/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as arc from "@architect/functions"
import userRepositoryFactory from "../../lib/architect/oauth/repository/UserRepository"
import meHandlerFactory from "../../lib/architect/oauth/handlers/me"
import userRepositoryFactory from "@architect/shared/architect/oauth/repository/UserRepository"
import meHandlerFactory from "@architect/shared/architect/oauth/handlers/me"

const handlerImp = meHandlerFactory(userRepositoryFactory())
export const handler = arc.http.async(handlerImp)
8 changes: 4 additions & 4 deletions server/src/http/get-auth-redirect-000provider/index.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import * as arc from "@architect/functions"
import oAuthRedirectHandlerFactory from "../../lib/architect/oauth/handlers/redirect"
import { tokenRepositoryFactory } from "../../lib/architect/oauth/repository/TokenRepository"
import userRepositoryFactory from "../../lib/architect/oauth/repository/UserRepository"
import { fetchJson } from "../../lib/fetch"
import oAuthRedirectHandlerFactory from "@architect/shared/architect/oauth/handlers/redirect"
import { tokenRepositoryFactory } from "@architect/shared/architect/oauth/repository/TokenRepository"
import userRepositoryFactory from "@architect/shared/architect/oauth/repository/UserRepository"
import { fetchJson } from "@architect/shared/fetch"

const impl = oAuthRedirectHandlerFactory(
fetchJson,
Expand Down
4 changes: 4 additions & 0 deletions server/src/shared/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
NOTE: This is specific to Architect to allow each http handler to be bundled (since in essence each http handler is it's own completely isolated module).
See https://arc.codes/docs/en/guides/developer-experience/sharing-code

NOTE: A potentially cleaner and less-specific approach to architect would be to put all code from shared into it's own package that is referenced by the web project itself (this is essentially what architect is doing but it kinda hides it).
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ describe("Tokenater", () => {

describe("isValid", () => {
it("should reject null/undefined tokens", async () => {
expect(ater.isValid(undefined)).toBeFalsy()
expect(ater.isValid(null)).toBeFalsy()
expect(ater.isValid((undefined as any) as string)).toBeFalsy()
expect(ater.isValid((null as any) as string)).toBeFalsy()
})

it("should accept valid signature + unexpired", async () => {
Expand Down
File renamed without changes.
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { createMockRequest } from "../../../../test/support/architect"
import {
ArchitectHttpRequestPayload,
ArchitectHttpResponsePayload,
} from "../../../types/http"
} from "../../types/http"
import {
addCsrfTokenToResponse,
expectCsrfTokenWithRequest,
Expand Down Expand Up @@ -34,9 +34,11 @@ describe("csrf", () => {

// add token to request:
req.headers = {}
req.headers[CSRF_HEADER_NAME] = tempResponse.headers[CSRF_HEADER_NAME]
req.headers[CSRF_HEADER_NAME] = tempResponse.headers
? tempResponse.headers[CSRF_HEADER_NAME]
: ""

// not ensure that it is accepted (request middleware will return no response if all is well):
// ensure that it is accepted (request middleware will return no response if all is well):
const res = expectCsrfTokenWithRequest(req)
expect(res).toBeUndefined()
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {
ArchitectHttpRequestPayload,
ArchitectHttpResponsePayload,
HttpHandler,
} from "../../../types/http"
} from "../../types/http"
import Tokenater from "../../Tokenater"
import { readSessionID } from "./session"

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export function writeSessionID(
/** Returns the session id for the given request. Assumes the request already has a session id */
export function readSessionID(req: HttpRequestLike): string {
if (!req.session || !req.session[SESSION_ID_KEY]) {
return null
return ""
}
return req.session[SESSION_ID_KEY]
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export class OAuthProviderConfig {
* Returns the value of the config setting for this provider.
*/
public value(template: Config): string {
return process.env[this.name(template)]
return process.env[this.name(template)] || ""
}

/**
Expand All @@ -27,7 +27,7 @@ export class OAuthProviderConfig {
*/
public validate(): string {
const missingConfigs = this.getMissingConfigNames()
if (missingConfigs) {
if (missingConfigs.length > 0) {
return (
`Provider "${this.providerName}" is not configured properly. Missing configuration: ` +
missingConfigs.join(", ")
Expand All @@ -54,7 +54,7 @@ export class OAuthProviderConfig {
}
}
if (missing.length > 0) return missing
else return null
else return []
}
}

Expand Down
File renamed without changes.
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {
ArchitectHttpRequestPayload,
ArchitectHttpResponsePayload,
} from "../../../../types/http"
} from "../../../types/http"
import { writeSessionID } from "../../middleware/session"
import { BAD_REQUEST, INTERNAL_SERVER_ERROR } from "./httpStatus"

Expand All @@ -13,14 +13,14 @@ export function getProviderName(
): [string, ArchitectHttpResponsePayload | null] {
const PROVIDER_NAME_PARAM = "provider"
const provider = req.pathParameters[PROVIDER_NAME_PARAM]
let err: ArchitectHttpResponsePayload = null
if (!provider) {
err = errorResponse(
const err = errorResponse(
BAD_REQUEST,
"provider path parameter must be specified"
)
return ["", err]
}
return [provider, err]
return [provider, null]
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { createMockRequest } from "../../../../../test/support/architect"
import { ArchitectHttpRequestPayload } from "../../../../types/http"
import { ArchitectHttpRequestPayload } from "../../../types/http"
import { readSessionID } from "../../middleware/session"
import login from "./login"
import { URL } from "url"
import assert from "assert"

describe("login handler", () => {
// preserve environment
Expand Down Expand Up @@ -97,9 +99,10 @@ describe("login handler", () => {
const res = await login(req)
expect(res).toHaveProperty("statusCode", 302)
expect(res).toHaveProperty("headers.location")
assert(res.headers)
const location = new URL(res.headers.location)
expect(location.searchParams.get("response_type")).toEqual("code")
expect(location.searchParams.get("scope").split(" ")).toContain("openid")
expect(location.searchParams.get("scope")?.split(" ")).toContain("openid")
expect(location.searchParams.get("client_id")).toEqual(
process.env.OAUTH_GOO_CLIENT_ID
)
Expand All @@ -120,6 +123,7 @@ describe("login handler", () => {
const res = await login(req)

// make sure it created a session
expect(res).toHaveProperty("statusCode", 302)
const foundSession = readSessionID(res)
expect(typeof foundSession).toStrictEqual("string")
expect(foundSession.length).toBeGreaterThan(0)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {
ArchitectHttpRequestPayload,
ArchitectHttpResponsePayload,
} from "../../../../types/http"
} from "../../../types/http"
import { createCSRFToken } from "../../middleware/csrf"
import {
createAnonymousSessionID,
Expand All @@ -10,6 +10,7 @@ import {
import { OAuthProviderConfig, Config } from "../OAuthProviderConfig"
import { addResponseSession, errorResponse, getProviderName } from "./common"
import { BAD_REQUEST } from "./httpStatus"
import { URL } from "url"

export default async function login(
req: ArchitectHttpRequestPayload
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {
ArchitectHttpRequestPayload,
ArchitectHttpResponsePayload,
HttpHandler,
} from "../../../../types/http"
} from "../../../types/http"
import { readSessionID } from "../../middleware/session"
import { StoredUser, UserRepository } from "../repository/UserRepository"

Expand All @@ -24,7 +24,7 @@ export default function meHandlerFactory(
statusCode: STATUS.UNAUTHENTICATED,
}
}
const user: StoredUser = await userRepository.get(sessionID)
const user = await userRepository.get(sessionID)
if (!user) {
return {
statusCode: STATUS.NOT_FOUND,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { randomBytes } from "crypto"
import { createMockRequest } from "../../../../../test/support/architect"
import { ArchitectHttpRequestPayload } from "../../../../types/http"
import { ArchitectHttpRequestPayload } from "../../../types/http"
import { createCSRFToken } from "../../middleware/csrf"
import {
createAnonymousSessionID,
Expand All @@ -13,6 +13,7 @@ import oAuthRedirectHandlerFactory from "./redirect"
import * as jwt from "node-webtokens"
import { randomEmail, randomInt } from "../../../../../test/support"
import sinon from "sinon"
import { URL } from "url"

// note to self: Jest's auto-mocking voodoo wastes more time than it saves. Just inject dependencies (e.g. w/ oAuthRedirectHandlerFactory)

Expand Down Expand Up @@ -222,6 +223,7 @@ describe("redirect", () => {

// NOTE: could mock the userRepo and just return a new user with an ID, to not need a functioning userRepo, but this works for now.
const newUser = await userRepo.getFromEmail(email)
if (!newUser) throw new Error("newUser must not be null")

expect(tokenRepoUpsert.callCount).toEqual(1)
const actualToken = tokenRepoUpsert.firstCall.args[0]
Expand Down Expand Up @@ -273,11 +275,11 @@ describe("redirect", () => {
expect(res).toHaveProperty("statusCode", 302)
expect(res).toHaveProperty("headers.location", "/")

// invoke handler for staging (/staging)
// invoke handler for staging (it used to be deployed to /staging, but we deploy everything to / now. Consider removing this test)
process.env.NODE_ENV = "staging"
res = await oauthRedirectHandler(req)
expect(res).toHaveProperty("statusCode", 302)
expect(res).toHaveProperty("headers.location", "/staging")
expect(res).toHaveProperty("headers.location", "/")
})
})

Expand Down

0 comments on commit 3cc0c10

Please sign in to comment.