Skip to content

Commit 2ad8f92

Browse files
committed
Cache GitHub user tokens
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.
1 parent 752a649 commit 2ad8f92

24 files changed

+230
-97
lines changed
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
import { entityFromPkOnlyEntity, typePredicateParser } from '@symphoniacloud/dynamodb-entity-store'
2+
import { GITHUB_USER_TOKEN } from '../entityTypes'
3+
import { GithubUserToken, isGithubUserToken } from '../../types/GithubUserToken'
4+
5+
export const GithubUserTokenEntity = entityFromPkOnlyEntity({
6+
type: GITHUB_USER_TOKEN,
7+
parse: typePredicateParser(isGithubUserToken, GITHUB_USER_TOKEN),
8+
pk(source: Pick<GithubUserToken, 'token'>) {
9+
return `USER_TOKEN#${source.token}`
10+
}
11+
})

src/app/domain/entityStore/entityTypes.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,4 +7,5 @@ export const GITHUB_PUSH = 'githubPush'
77
export const GITHUB_ACCOUNT_MEMBERSHIP = 'githubAccountMembership'
88
export const GITHUB_REPOSITORY = 'githubRepository'
99
export const GITHUB_USER = 'githubUser'
10+
export const GITHUB_USER_TOKEN = 'githubUserToken'
1011
export const WEB_PUSH_SUBSCRIPTION = 'webPushSubscription'

src/app/domain/entityStore/initEntityStore.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import {
1515
GITHUB_PUSH,
1616
GITHUB_REPOSITORY,
1717
GITHUB_USER,
18+
GITHUB_USER_TOKEN,
1819
GITHUB_WORKFLOW_RUN,
1920
GITHUB_WORKFLOW_RUN_EVENT,
2021
WEB_PUSH_SUBSCRIPTION
@@ -54,6 +55,10 @@ export function setupEntityStore(
5455
entityTypes: [GITHUB_INSTALLATION],
5556
...entityStoreConfigFor(tableNames, 'github-installations')
5657
},
58+
{
59+
entityTypes: [GITHUB_USER_TOKEN],
60+
...entityStoreConfigFor(tableNames, 'github-user-tokens')
61+
},
5762
{
5863
entityTypes: [GITHUB_USER],
5964
...entityStoreConfigFor(tableNames, 'github-users')
@@ -100,6 +105,7 @@ function entityStoreConfigFor(tableNames: TableNames, tableId: CicadaTableId): T
100105
pk: 'PK',
101106
entityType: '_et',
102107
lastUpdated: '_lastUpdated',
108+
...(config.useTtl ? { ttl: 'ttl' } : {}),
103109
...(config.hasSortKey ? { sk: 'SK' } : {}),
104110
...(config.hasGSI1
105111
? {

src/app/domain/github/githubUser.ts

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
11
import { AppState } from '../../environment/AppState'
2-
import { logger } from '../../util/logging'
32
import { GithubUserEntity } from '../entityStore/entities/GithubUserEntity'
43
import { fromRawGithubUser, GithubUser } from '../types/GithubUser'
54
import { GithubInstallation } from '../types/GithubInstallation'
65
import { RawGithubUser } from '../types/rawGithub/RawGithubUser'
76
import { setMemberships } from './githubMembership'
7+
import { GithubUserToken } from '../types/GithubUserToken'
8+
import { isGithubCheckRequired, saveOrRefreshGithubUserToken } from './githubUserToken'
89

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

2627
export async function getUserByAuthToken(appState: AppState, token: string) {
27-
// TOEventually - don't require calling GitHub API for all user requests - cache for some period
2828
const rawGithubUser = await appState.githubClient.getGithubUser(token)
2929
if (!rawGithubUser) return undefined
30+
3031
const cicadaUser = await getUserById(appState, rawGithubUser.id)
31-
if (!cicadaUser) {
32-
logger.info(
33-
`User ${rawGithubUser.login} is a valid GitHub user but does not have access to any Cicada resources`
34-
)
35-
}
32+
if (cicadaUser)
33+
await saveOrRefreshGithubUserToken(appState, {
34+
token,
35+
userId: cicadaUser.id,
36+
userLogin: cicadaUser.login
37+
})
38+
3639
return cicadaUser
3740
}
3841

42+
export async function getUserByTokenRecord(appState: AppState, tokenRecord: GithubUserToken) {
43+
return isGithubCheckRequired(appState.clock, tokenRecord)
44+
? getUserByAuthToken(appState, tokenRecord.token)
45+
: getUserById(appState, tokenRecord.userId)
46+
}
47+
3948
export async function getUserById(appState: AppState, id: number) {
4049
return await appState.entityStore.for(GithubUserEntity).getOrUndefined({ id })
4150
}

src/app/domain/github/githubUserAuth/oauthCallback.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ async function tryOauthCallback(
5555

5656
// For now the cookie token Cicada uses is precisely the GitHub user token. In theory Cicada
5757
// could generate its own tokens and then keep a database table mapping those tokens to users, but
58-
// for now using Github's token is sufficient
58+
// for now using Github's token is sufficient. Besides, we need the user's token anyway for later checks
5959
const webHostname = `${await appState.config.webHostname()}`
6060
return redirectResponseWithCookies(
6161
`https://${webHostname}/app`,
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
import { AppState } from '../../environment/AppState'
2+
import { GithubUserTokenEntity } from '../entityStore/entities/GithubUserTokenEntity'
3+
import { Clock, secondsTimestampInFutureHours, timestampSecondsIsInPast } from '../../util/dateAndTime'
4+
import { GithubUserToken } from '../types/GithubUserToken'
5+
6+
const EXPIRE_CACHED_GITHUB_TOKENS_HOURS = 1
7+
8+
export async function saveOrRefreshGithubUserToken(
9+
appState: AppState,
10+
tokenRecord: Pick<GithubUserToken, 'token' | 'userId' | 'userLogin'>
11+
) {
12+
await appState.entityStore.for(GithubUserTokenEntity).put(
13+
{
14+
...tokenRecord,
15+
nextCheckTime: secondsTimestampInFutureHours(appState.clock, EXPIRE_CACHED_GITHUB_TOKENS_HOURS)
16+
},
17+
{
18+
ttlInFutureDays: 7
19+
}
20+
)
21+
}
22+
23+
export async function getGithubUserTokenOrUndefined(appState: AppState, token: string) {
24+
return await appState.entityStore.for(GithubUserTokenEntity).getOrUndefined({ token })
25+
}
26+
27+
// If token record was saved more than EXPIRE_CACHED_GITHUB_TOKENS_HOURS ago then check user token with GitHub agaion
28+
export function isGithubCheckRequired(clock: Clock, token: GithubUserToken) {
29+
return timestampSecondsIsInPast(clock, token.nextCheckTime)
30+
}
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
export interface GithubUserToken {
2+
token: string
3+
userId: number
4+
userLogin: string
5+
nextCheckTime: number
6+
}
7+
8+
export function isGithubUserToken(x: unknown): x is GithubUserToken {
9+
const candidate = x as GithubUserToken
10+
return (
11+
candidate.token !== undefined &&
12+
candidate.userId !== undefined &&
13+
candidate.userLogin !== undefined &&
14+
candidate.nextCheckTime !== undefined
15+
)
16+
}
Lines changed: 12 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,11 @@
11
import { AppState } from '../../environment/AppState'
22
import { logger } from '../../util/logging'
3-
import { processTestToken } from './userAuthorizerForTests'
4-
import { getUserByAuthToken } from '../github/githubUser'
3+
import { isTestUserToken, processTestToken } from './userAuthorizerForTests'
4+
import { getUserByTokenRecord } from '../github/githubUser'
55
import { getAllAccountIdsForUser } from '../github/githubMembership'
6-
7-
export type WithHeadersEvent = {
8-
headers: { [name: string]: string | undefined } | null
9-
multiValueHeaders: {
10-
[name: string]: string[] | undefined
11-
} | null
12-
}
6+
import { getToken } from './webAuthToken'
7+
import { WithHeadersEvent } from '../../inboundInterfaces/lambdaTypes'
8+
import { getGithubUserTokenOrUndefined } from '../github/githubUserToken'
139

1410
export type AuthorizationResult = SuccessfulAuthorizationResult | undefined
1511

@@ -19,7 +15,7 @@ export type SuccessfulAuthorizationResult = {
1915
}
2016

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

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

38-
// Makes a call to GitHub to get user ID, then use that to get Cicada user from DB
39-
// If either fail (e.g. token no longer valid at GitHub, or user not registered in Cicada)
40-
// then unauthorized
41-
const cicadaUser = await getUserByAuthToken(appState, token)
34+
const tokenRecord = await getGithubUserTokenOrUndefined(appState, token)
35+
if (!tokenRecord) return undefined
36+
37+
const cicadaUser = await getUserByTokenRecord(appState, tokenRecord)
38+
// Github token has expired or user has been removed from Cicada
4239
if (!cicadaUser) return undefined
4340

4441
// If the user exists in Cicada, but no longer has any memberships, then unauthorized
@@ -52,21 +49,3 @@ export async function authorizeUserRequest(
5249
username: cicadaUser.login
5350
}
5451
}
55-
56-
function getToken(event: WithHeadersEvent) {
57-
const tokenCookie = getTokenCookie(event)
58-
return tokenCookie ? tokenCookie.split('=')[1] : undefined
59-
}
60-
61-
// Using both single and multi value headers because there may only be one cookie
62-
// if user manually delete "isLoggedIn" cookie, otherwise more than one
63-
function getTokenCookie(event: WithHeadersEvent) {
64-
const singleHeaderTokenCookie = (event.headers?.Cookie ?? '')
65-
.split(';')
66-
.map((x) => x.trim())
67-
.find((x) => x.startsWith('token='))
68-
69-
if (singleHeaderTokenCookie) return singleHeaderTokenCookie
70-
71-
return (event.multiValueHeaders?.Cookie ?? []).find((x) => x.startsWith('token='))
72-
}

src/app/domain/webAuth/userAuthorizerForTests.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,10 @@ import { AppState } from '../../environment/AppState'
33
import { SSM_PARAM_NAMES } from '../../../multipleContexts/ssmParams'
44
import { paramsForAppName } from '../../environment/config'
55

6+
export function isTestUserToken(token: string) {
7+
return token.indexOf('testuser') >= 0
8+
}
9+
610
export async function processTestToken(appState: AppState, token: string) {
711
try {
812
const { username, userId, secret } = JSON.parse(token)
Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
import { WithHeadersEvent } from '../../inboundInterfaces/lambdaTypes'
2+
3+
export function getToken(event: WithHeadersEvent) {
4+
const tokenCookie = getTokenCookie(event)
5+
return tokenCookie ? tokenCookie.split('=')[1] : undefined
6+
}
7+
8+
// Using both single and multi value headers because there may only be one cookie
9+
// if user manually delete "isLoggedIn" cookie, otherwise more than one
10+
function getTokenCookie(event: WithHeadersEvent) {
11+
const singleHeaderTokenCookie = (event.headers?.Cookie ?? '')
12+
.split(';')
13+
.map((x) => x.trim())
14+
.find((x) => x.startsWith('token='))
15+
16+
if (singleHeaderTokenCookie) return singleHeaderTokenCookie
17+
18+
return (event.multiValueHeaders?.Cookie ?? []).find((x) => x.startsWith('token='))
19+
}

0 commit comments

Comments
 (0)