Skip to content

Commit

Permalink
Cache GitHub user tokens
Browse files Browse the repository at this point in the history
Avoids having to call DynamoDB on every request.

Cache local value for 1 hour before calling GitHub again.

This still requires the user to refresh / login with GitHub every 8 hours. Consider later using GitHub refresh tokens.
  • Loading branch information
mikebroberts committed Jul 8, 2024
1 parent 752a649 commit 2ad8f92
Show file tree
Hide file tree
Showing 24 changed files with 230 additions and 97 deletions.
11 changes: 11 additions & 0 deletions src/app/domain/entityStore/entities/GithubUserTokenEntity.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { entityFromPkOnlyEntity, typePredicateParser } from '@symphoniacloud/dynamodb-entity-store'
import { GITHUB_USER_TOKEN } from '../entityTypes'
import { GithubUserToken, isGithubUserToken } from '../../types/GithubUserToken'

export const GithubUserTokenEntity = entityFromPkOnlyEntity({
type: GITHUB_USER_TOKEN,
parse: typePredicateParser(isGithubUserToken, GITHUB_USER_TOKEN),
pk(source: Pick<GithubUserToken, 'token'>) {
return `USER_TOKEN#${source.token}`
}
})
1 change: 1 addition & 0 deletions src/app/domain/entityStore/entityTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@ export const GITHUB_PUSH = 'githubPush'
export const GITHUB_ACCOUNT_MEMBERSHIP = 'githubAccountMembership'
export const GITHUB_REPOSITORY = 'githubRepository'
export const GITHUB_USER = 'githubUser'
export const GITHUB_USER_TOKEN = 'githubUserToken'
export const WEB_PUSH_SUBSCRIPTION = 'webPushSubscription'
6 changes: 6 additions & 0 deletions src/app/domain/entityStore/initEntityStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
GITHUB_PUSH,
GITHUB_REPOSITORY,
GITHUB_USER,
GITHUB_USER_TOKEN,
GITHUB_WORKFLOW_RUN,
GITHUB_WORKFLOW_RUN_EVENT,
WEB_PUSH_SUBSCRIPTION
Expand Down Expand Up @@ -54,6 +55,10 @@ export function setupEntityStore(
entityTypes: [GITHUB_INSTALLATION],
...entityStoreConfigFor(tableNames, 'github-installations')
},
{
entityTypes: [GITHUB_USER_TOKEN],
...entityStoreConfigFor(tableNames, 'github-user-tokens')
},
{
entityTypes: [GITHUB_USER],
...entityStoreConfigFor(tableNames, 'github-users')
Expand Down Expand Up @@ -100,6 +105,7 @@ function entityStoreConfigFor(tableNames: TableNames, tableId: CicadaTableId): T
pk: 'PK',
entityType: '_et',
lastUpdated: '_lastUpdated',
...(config.useTtl ? { ttl: 'ttl' } : {}),
...(config.hasSortKey ? { sk: 'SK' } : {}),
...(config.hasGSI1
? {
Expand Down
23 changes: 16 additions & 7 deletions src/app/domain/github/githubUser.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { AppState } from '../../environment/AppState'
import { logger } from '../../util/logging'
import { GithubUserEntity } from '../entityStore/entities/GithubUserEntity'
import { fromRawGithubUser, GithubUser } from '../types/GithubUser'
import { GithubInstallation } from '../types/GithubInstallation'
import { RawGithubUser } from '../types/rawGithub/RawGithubUser'
import { setMemberships } from './githubMembership'
import { GithubUserToken } from '../types/GithubUserToken'
import { isGithubCheckRequired, saveOrRefreshGithubUserToken } from './githubUserToken'

export async function processRawUsers(
appState: AppState,
Expand All @@ -24,18 +25,26 @@ export async function saveUsers(appState: AppState, users: GithubUser[]) {
}

export async function getUserByAuthToken(appState: AppState, token: string) {
// TOEventually - don't require calling GitHub API for all user requests - cache for some period
const rawGithubUser = await appState.githubClient.getGithubUser(token)
if (!rawGithubUser) return undefined

const cicadaUser = await getUserById(appState, rawGithubUser.id)
if (!cicadaUser) {
logger.info(
`User ${rawGithubUser.login} is a valid GitHub user but does not have access to any Cicada resources`
)
}
if (cicadaUser)
await saveOrRefreshGithubUserToken(appState, {
token,
userId: cicadaUser.id,
userLogin: cicadaUser.login
})

return cicadaUser
}

export async function getUserByTokenRecord(appState: AppState, tokenRecord: GithubUserToken) {
return isGithubCheckRequired(appState.clock, tokenRecord)
? getUserByAuthToken(appState, tokenRecord.token)
: getUserById(appState, tokenRecord.userId)
}

export async function getUserById(appState: AppState, id: number) {
return await appState.entityStore.for(GithubUserEntity).getOrUndefined({ id })
}
2 changes: 1 addition & 1 deletion src/app/domain/github/githubUserAuth/oauthCallback.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ async function tryOauthCallback(

// For now the cookie token Cicada uses is precisely the GitHub user token. In theory Cicada
// could generate its own tokens and then keep a database table mapping those tokens to users, but
// for now using Github's token is sufficient
// for now using Github's token is sufficient. Besides, we need the user's token anyway for later checks
const webHostname = `${await appState.config.webHostname()}`
return redirectResponseWithCookies(
`https://${webHostname}/app`,
Expand Down
30 changes: 30 additions & 0 deletions src/app/domain/github/githubUserToken.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { AppState } from '../../environment/AppState'
import { GithubUserTokenEntity } from '../entityStore/entities/GithubUserTokenEntity'
import { Clock, secondsTimestampInFutureHours, timestampSecondsIsInPast } from '../../util/dateAndTime'
import { GithubUserToken } from '../types/GithubUserToken'

const EXPIRE_CACHED_GITHUB_TOKENS_HOURS = 1

export async function saveOrRefreshGithubUserToken(
appState: AppState,
tokenRecord: Pick<GithubUserToken, 'token' | 'userId' | 'userLogin'>
) {
await appState.entityStore.for(GithubUserTokenEntity).put(
{
...tokenRecord,
nextCheckTime: secondsTimestampInFutureHours(appState.clock, EXPIRE_CACHED_GITHUB_TOKENS_HOURS)
},
{
ttlInFutureDays: 7
}
)
}

export async function getGithubUserTokenOrUndefined(appState: AppState, token: string) {
return await appState.entityStore.for(GithubUserTokenEntity).getOrUndefined({ token })
}

// If token record was saved more than EXPIRE_CACHED_GITHUB_TOKENS_HOURS ago then check user token with GitHub agaion
export function isGithubCheckRequired(clock: Clock, token: GithubUserToken) {
return timestampSecondsIsInPast(clock, token.nextCheckTime)
}
16 changes: 16 additions & 0 deletions src/app/domain/types/GithubUserToken.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
export interface GithubUserToken {
token: string
userId: number
userLogin: string
nextCheckTime: number
}

export function isGithubUserToken(x: unknown): x is GithubUserToken {
const candidate = x as GithubUserToken
return (
candidate.token !== undefined &&
candidate.userId !== undefined &&
candidate.userLogin !== undefined &&
candidate.nextCheckTime !== undefined
)
}
45 changes: 12 additions & 33 deletions src/app/domain/webAuth/userAuthorizer.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
import { AppState } from '../../environment/AppState'
import { logger } from '../../util/logging'
import { processTestToken } from './userAuthorizerForTests'
import { getUserByAuthToken } from '../github/githubUser'
import { isTestUserToken, processTestToken } from './userAuthorizerForTests'
import { getUserByTokenRecord } from '../github/githubUser'
import { getAllAccountIdsForUser } from '../github/githubMembership'

export type WithHeadersEvent = {
headers: { [name: string]: string | undefined } | null
multiValueHeaders: {
[name: string]: string[] | undefined
} | null
}
import { getToken } from './webAuthToken'
import { WithHeadersEvent } from '../../inboundInterfaces/lambdaTypes'
import { getGithubUserTokenOrUndefined } from '../github/githubUserToken'

export type AuthorizationResult = SuccessfulAuthorizationResult | undefined

Expand All @@ -19,7 +15,7 @@ export type SuccessfulAuthorizationResult = {
}

// Common code used by both API Gateway Lambda Authorizer AND /appa/... Lambda function
// TODO - eventually do a cached lookup to Github to avoid calling GitHub every time
// TOEventually - get some Dynamodb caching here
export async function authorizeUserRequest(
appState: AppState,
event: WithHeadersEvent
Expand All @@ -31,14 +27,15 @@ export async function authorizeUserRequest(
}

// Use a special token for integration tests so that they don't need to cause calls to GitHub
if (token.indexOf('testuser') >= 0) {
if (isTestUserToken(token)) {
return await processTestToken(appState, token)
}

// Makes a call to GitHub to get user ID, then use that to get Cicada user from DB
// If either fail (e.g. token no longer valid at GitHub, or user not registered in Cicada)
// then unauthorized
const cicadaUser = await getUserByAuthToken(appState, token)
const tokenRecord = await getGithubUserTokenOrUndefined(appState, token)
if (!tokenRecord) return undefined

const cicadaUser = await getUserByTokenRecord(appState, tokenRecord)
// Github token has expired or user has been removed from Cicada
if (!cicadaUser) return undefined

// If the user exists in Cicada, but no longer has any memberships, then unauthorized
Expand All @@ -52,21 +49,3 @@ export async function authorizeUserRequest(
username: cicadaUser.login
}
}

function getToken(event: WithHeadersEvent) {
const tokenCookie = getTokenCookie(event)
return tokenCookie ? tokenCookie.split('=')[1] : undefined
}

// Using both single and multi value headers because there may only be one cookie
// if user manually delete "isLoggedIn" cookie, otherwise more than one
function getTokenCookie(event: WithHeadersEvent) {
const singleHeaderTokenCookie = (event.headers?.Cookie ?? '')
.split(';')
.map((x) => x.trim())
.find((x) => x.startsWith('token='))

if (singleHeaderTokenCookie) return singleHeaderTokenCookie

return (event.multiValueHeaders?.Cookie ?? []).find((x) => x.startsWith('token='))
}
4 changes: 4 additions & 0 deletions src/app/domain/webAuth/userAuthorizerForTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ import { AppState } from '../../environment/AppState'
import { SSM_PARAM_NAMES } from '../../../multipleContexts/ssmParams'
import { paramsForAppName } from '../../environment/config'

export function isTestUserToken(token: string) {
return token.indexOf('testuser') >= 0
}

export async function processTestToken(appState: AppState, token: string) {
try {
const { username, userId, secret } = JSON.parse(token)
Expand Down
19 changes: 19 additions & 0 deletions src/app/domain/webAuth/webAuthToken.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { WithHeadersEvent } from '../../inboundInterfaces/lambdaTypes'

export function getToken(event: WithHeadersEvent) {
const tokenCookie = getTokenCookie(event)
return tokenCookie ? tokenCookie.split('=')[1] : undefined
}

// Using both single and multi value headers because there may only be one cookie
// if user manually delete "isLoggedIn" cookie, otherwise more than one
function getTokenCookie(event: WithHeadersEvent) {
const singleHeaderTokenCookie = (event.headers?.Cookie ?? '')
.split(';')
.map((x) => x.trim())
.find((x) => x.startsWith('token='))

if (singleHeaderTokenCookie) return singleHeaderTokenCookie

return (event.multiValueHeaders?.Cookie ?? []).find((x) => x.startsWith('token='))
}
11 changes: 10 additions & 1 deletion src/app/inboundInterfaces/lambdaTypes.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
import { APIGatewayProxyWithLambdaAuthorizerEvent } from 'aws-lambda/trigger/api-gateway-proxy'
import {
APIGatewayProxyEventHeaders,
APIGatewayProxyEventMultiValueHeaders,
APIGatewayProxyWithLambdaAuthorizerEvent
} from 'aws-lambda/trigger/api-gateway-proxy'
import { APIGatewayProxyEvent, APIGatewayProxyWithLambdaAuthorizerHandler } from 'aws-lambda'

// We store userId in database as a number (because it's a number on the GitHub API),
Expand All @@ -10,3 +14,8 @@ export type CicadaAPIAuthorizedAPIEvent = APIGatewayProxyWithLambdaAuthorizerEve
export type CicadaAPIAuthorizedAPIHandler = APIGatewayProxyWithLambdaAuthorizerHandler<WebAuthorizerContext>

export type CicadaAuthorizedAPIEvent = APIGatewayProxyEvent & { username: string; userId: number }

export type WithHeadersEvent = {
headers: APIGatewayProxyEventHeaders | null
multiValueHeaders: APIGatewayProxyEventMultiValueHeaders | null
}
8 changes: 6 additions & 2 deletions src/app/util/dateAndTime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ export function dateTimeAddDays(date: Date, days: number) {
return dateTimeAddHours(date, days * 24)
}

export function secondsTimestampInFutureDays(clock: Clock, days: number): number {
return dateToTimestampSeconds(dateTimeAddDays(clock.now(), days))
export function secondsTimestampInFutureHours(clock: Clock, hours: number): number {
return dateToTimestampSeconds(dateTimeAddHours(clock.now(), hours))
}

export function dateToTimestampSeconds(date: Date) {
Expand All @@ -53,6 +53,10 @@ export function isoDifferenceMs(isoOne: string, isoTwo: string) {
return timestampDifferenceMs(Date.parse(isoOne), Date.parse(isoTwo))
}

export function timestampSecondsIsInPast(clock: Clock, timestampSeconds: number) {
return dateToTimestampSeconds(clock.now()) > timestampSeconds
}

export function timestampDifferenceMs(tsOne: number, tsTwo: number) {
return Math.abs(tsOne.valueOf() - tsTwo.valueOf())
}
Expand Down
3 changes: 2 additions & 1 deletion src/cdk/stacks/main/githubInteraction.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ function defineAuth(scope: Construct, props: GithubInteractionProps, githubApiRe
const lambdaFunction = new CicadaFunction(
scope,
cicadaFunctionProps(props, 'githubAuth', {
tablesReadAccess: ['github-users']
tablesReadAccess: ['github-users'],
tablesReadWriteAccess: ['github-user-tokens']
})
)
githubApiResource
Expand Down
6 changes: 4 additions & 2 deletions src/cdk/stacks/main/userFacingWeb.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ function defineAuthorizer(scope: Construct, props: UserFacingWebEndpointsProps)
scope,
cicadaFunctionProps(props, 'apiGatewayAuthorizer', {
timeoutSeconds: 10,
tablesReadAccess: ['github-users', 'github-account-memberships']
tablesReadAccess: ['github-users', 'github-account-memberships'],
tablesReadWriteAccess: ['github-user-tokens']
})
)
return new RequestAuthorizer(scope, 'RestRequestAuthorizer', {
Expand All @@ -46,7 +47,8 @@ function defineAuthenticatedWeb(scope: Construct, props: UserFacingWebEndpointsP
'github-latest-workflow-runs',
'github-latest-pushes-per-ref',
'github-repo-activity'
]
],
tablesReadWriteAccess: ['github-user-tokens']
})
)
props.restApi.root
Expand Down
7 changes: 6 additions & 1 deletion src/multipleContexts/dynamoDBTables.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

export const CICADA_TABLE_IDS = [
'github-installations',
'github-user-tokens',
'github-users',
'github-account-memberships',
'github-repositories',
Expand All @@ -19,16 +20,19 @@ interface CicadaTableConfig {
readonly hasSortKey: boolean
readonly hasGSI1: boolean
readonly stream: boolean
readonly useTtl: boolean
}

const allFalseConfig: CicadaTableConfig = {
hasGSI1: false,
hasSortKey: false,
stream: false
stream: false,
useTtl: false
}

export const tableConfigurations: Record<CicadaTableId, CicadaTableConfig> = {
'github-installations': allFalseConfig,
'github-user-tokens': { ...allFalseConfig, useTtl: true },
'github-users': allFalseConfig,
'github-account-memberships': {
...allFalseConfig,
Expand All @@ -41,6 +45,7 @@ export const tableConfigurations: Record<CicadaTableId, CicadaTableConfig> = {
hasGSI1: false
},
'github-repo-activity': {
...allFalseConfig,
hasSortKey: true,
hasGSI1: true,
stream: true
Expand Down
1 change: 1 addition & 0 deletions src/multipleContexts/ssmParams.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export const SSM_PARAM_NAMES = {
// Typescript Question - 1-1 with CICADA_TABLE_IDS - is it possible to remove duplication?
type SsmTableNameParamName =
| 'resources/table/github-installations'
| 'resources/table/github-user-tokens'
| 'resources/table/github-users'
| 'resources/table/github-repositories'
| 'resources/table/github-account-memberships'
Expand Down
8 changes: 8 additions & 0 deletions test/examples/cicada/githubDomainObjects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { GithubAccountMembership } from '../../../src/app/domain/types/GithubAcc
import { GithubRepository } from '../../../src/app/domain/types/GithubRepository'
import { GithubWorkflowRunEvent } from '../../../src/app/domain/types/GithubWorkflowRunEvent'
import { GithubPush } from '../../../src/app/domain/types/GithubPush'
import { GithubUserToken } from '../../../src/app/domain/types/GithubUserToken'

export const testPersonalInstallation: GithubInstallation = {
accountId: 162360409,
Expand All @@ -23,6 +24,13 @@ export const testOrgInstallation: GithubInstallation = {
installationId: 48133709
}

export const testTestUserTokenRecord: GithubUserToken = {
token: 'validUserToken',
userId: 162360409,
userLogin: 'cicada-test-user',
nextCheckTime: 1800000000
}

export const testTestUser: GithubUser = {
avatarUrl: 'https://avatars.githubusercontent.com/u/162360409?v=4',
htmlUrl: 'https://github.com/cicada-test-user',
Expand Down
Loading

0 comments on commit 2ad8f92

Please sign in to comment.