Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
18 changes: 12 additions & 6 deletions app/routes/_auth+/auth.$provider.callback.test.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { invariant } from '@epic-web/invariant'
import { faker } from '@faker-js/faker'
import { SetCookie } from '@mjackson/headers'
import { http } from 'msw'
import { afterEach, expect, test } from 'vitest'
import { twoFAVerificationType } from '#app/routes/settings+/profile.two-factor.tsx'
import { getSessionExpirationDate, sessionKey } from '#app/utils/auth.server.ts'
import { connectionSessionStorage } from '#app/utils/connections.server.ts'
import { GITHUB_PROVIDER_NAME } from '#app/utils/connections.tsx'
import { prisma } from '#app/utils/db.server.ts'
import { authSessionStorage } from '#app/utils/session.server.ts'
Expand Down Expand Up @@ -219,19 +219,25 @@ async function setupRequest({
const state = faker.string.uuid()
url.searchParams.set('state', state)
url.searchParams.set('code', code)
const connectionSession = await connectionSessionStorage.getSession()
connectionSession.set('oauth2:state', state)
const authSession = await authSessionStorage.getSession()
if (sessionId) authSession.set(sessionKey, sessionId)
const setSessionCookieHeader =
await authSessionStorage.commitSession(authSession)
const setConnectionSessionCookieHeader =
await connectionSessionStorage.commitSession(connectionSession)
const searchParams = new URLSearchParams({ code, state })
let authCookie = new SetCookie({
name: 'github',
value: searchParams.toString(),
path: '/',
sameSite: 'Lax',
httpOnly: true,
maxAge: 60 * 10,
secure: process.env.NODE_ENV === 'production' || undefined,
})
const request = new Request(url.toString(), {
method: 'GET',
headers: {
cookie: [
convertSetCookieToCookie(setConnectionSessionCookieHeader),
authCookie.toString(),
convertSetCookieToCookie(setSessionCookieHeader),
].join('; '),
},
Expand Down
2 changes: 1 addition & 1 deletion app/routes/_auth+/auth.$provider.callback.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export async function loader({ request, params }: Route.LoaderArgs) {
const label = providerLabels[providerName]

const authResult = await authenticator
.authenticate(providerName, request, { throwOnError: true })
.authenticate(providerName, request)
.then(
(data) =>
({
Expand Down
10 changes: 1 addition & 9 deletions app/routes/_auth+/onboarding_.$provider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,11 @@
import { Spacer } from '#app/components/spacer.tsx'
import { StatusButton } from '#app/components/ui/status-button.tsx'
import {
authenticator,

Check warning on line 21 in app/routes/_auth+/onboarding_.$provider.tsx

View workflow job for this annotation

GitHub Actions / ⬣ ESLint

'authenticator' is defined but never used. Allowed unused vars must match /^ignored/u
sessionKey,
signupWithConnection,
requireAnonymous,
} from '#app/utils/auth.server.ts'
import { connectionSessionStorage } from '#app/utils/connections.server'
import { ProviderNameSchema } from '#app/utils/connections.tsx'
import { prisma } from '#app/utils/db.server.ts'
import { useIsPending } from '#app/utils/misc.tsx'
Expand Down Expand Up @@ -78,24 +77,17 @@

export async function loader({ request, params }: Route.LoaderArgs) {
const { email } = await requireData({ request, params })
const connectionSession = await connectionSessionStorage.getSession(
request.headers.get('cookie'),
)

const verifySession = await verifySessionStorage.getSession(
request.headers.get('cookie'),
)
const prefilledProfile = verifySession.get(prefilledProfileKey)

const formError = connectionSession.get(authenticator.sessionErrorKey)
const hasError = typeof formError === 'string'

return {
email,
status: 'idle',
submission: {
status: hasError ? 'error' : undefined,
initialValue: prefilledProfile ?? {},
error: { '': hasError ? [formError] : [] },
} as SubmissionResult,
}
}
Expand Down
6 changes: 2 additions & 4 deletions app/utils/auth.server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import bcrypt from 'bcryptjs'
import { redirect } from 'react-router'
import { Authenticator } from 'remix-auth'
import { safeRedirect } from 'remix-utils/safe-redirect'
import { connectionSessionStorage, providers } from './connections.server.ts'
import { providers } from './connections.server.ts'
import { prisma } from './db.server.ts'
import { combineHeaders, downloadFile } from './misc.tsx'
import { type ProviderUser } from './providers/provider.ts'
Expand All @@ -16,9 +16,7 @@ export const getSessionExpirationDate = () =>

export const sessionKey = 'sessionId'

export const authenticator = new Authenticator<ProviderUser>(
connectionSessionStorage,
)
export const authenticator = new Authenticator<ProviderUser>()

for (const [providerName, provider] of Object.entries(providers)) {
authenticator.use(provider.getAuthStrategy(), providerName)
Expand Down
14 changes: 1 addition & 13 deletions app/utils/connections.server.ts
Original file line number Diff line number Diff line change
@@ -1,21 +1,9 @@
import { createCookieSessionStorage } from 'react-router'
// import { createCookieSessionStorage } from 'react-router'
import { type ProviderName } from './connections.tsx'
import { GitHubProvider } from './providers/github.server.ts'
import { type AuthProvider } from './providers/provider.ts'
import { type Timings } from './timing.server.ts'

export const connectionSessionStorage = createCookieSessionStorage({
cookie: {
name: 'en_connection',
sameSite: 'lax', // CSRF protection is advised if changing to 'none'
path: '/',
httpOnly: true,
maxAge: 60 * 10, // 10 minutes
secrets: process.env.SESSION_SECRET.split(','),
secure: process.env.NODE_ENV === 'production',
},
})

Comment on lines -7 to -18
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused why changing this was necessary because it looks like we've now lost the ability to display errors as the user goes between different pages in the OAuth flow. And I don't think that this change was necessary for upgrading to Remix auth. Can you help me understand why this change was made?

Choose a reason for hiding this comment

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

This is no needed anymore since Remix Auth doesn't receive a session storage instance anymore, instead the strategy uses @mjackson/headers to save the data it needs in a cookie during the auth process.

Then after the user is back in the app from GitHub it you need to save the data in your session storage yourself, AFAIK you already did this anyway so there's no change in that part.

Choose a reason for hiding this comment

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

Also related to error handling, now the strategies always throw an error, so you handle them with a try/catch

Copy link
Member

Choose a reason for hiding this comment

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

Great to know! Thanks!

export const providers: Record<ProviderName, AuthProvider> = {
github: new GitHubProvider(),
}
Expand Down
74 changes: 56 additions & 18 deletions app/utils/providers/github.server.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { SetCookie } from '@mjackson/headers'
import { createId as cuid } from '@paralleldrive/cuid2'
import { redirect } from 'react-router'
import { GitHubStrategy } from 'remix-auth-github'
import { z } from 'zod'
import { cache, cachified } from '../cache.server.ts'
import { connectionSessionStorage } from '../connections.server.ts'
import { type Timings } from '../timing.server.ts'
import { MOCK_CODE_GITHUB_HEADER, MOCK_CODE_GITHUB } from './constants.ts'
import { type AuthProvider } from './provider.ts'
Expand All @@ -24,27 +24,62 @@ const shouldMock =
process.env.GITHUB_CLIENT_ID?.startsWith('MOCK_') ||
process.env.NODE_ENV === 'test'

type GitHubEmailsResponse = {
email: string
verified: boolean
primary: boolean
visibility: string | null
}[]

type GitHubUserResponse = {
login: string
id: string
name: string | undefined
avatar_url: string | undefined
}

export class GitHubProvider implements AuthProvider {
getAuthStrategy() {
return new GitHubStrategy(
{
clientID: process.env.GITHUB_CLIENT_ID,
clientId: process.env.GITHUB_CLIENT_ID,
clientSecret: process.env.GITHUB_CLIENT_SECRET,
callbackURL: '/auth/github/callback',
redirectURI: '/auth/github/callback',
},
async ({ profile }) => {
const email = profile.emails[0]?.value.trim().toLowerCase()
async ({ tokens }) => {
// we need to fetch the user and the emails separately, this is a change in remix-auth-github
// from the previous version that supported fetching both in one call
const userResponse = await fetch('https://api.github.com/user', {
headers: {
Accept: 'application/vnd.github+json',
Authorization: `Bearer ${tokens.accessToken()}`,
'X-GitHub-Api-Version': '2022-11-28',
},
})
const user = (await userResponse.json()) as GitHubUserResponse

const emailsResponse = await fetch(
'https://api.github.com/user/emails',
{
headers: {
Accept: 'application/vnd.github+json',
Authorization: `Bearer ${tokens.accessToken()}`,
'X-GitHub-Api-Version': '2022-11-28',
},
},
)
const emails = (await emailsResponse.json()) as GitHubEmailsResponse
const email = emails.find((e) => e.primary)?.email
if (!email) {
throw new Error('Email not found')
}
const username = profile.displayName
const imageUrl = profile.photos[0]?.value

return {
id: user.id,
email,
id: profile.id,
username,
name: profile.name.givenName,
imageUrl,
name: user.name,
username: user.login,
imageUrl: user.avatar_url,
}
},
)
Expand Down Expand Up @@ -85,21 +120,24 @@ export class GitHubProvider implements AuthProvider {
async handleMockAction(request: Request) {
if (!shouldMock) return

const connectionSession = await connectionSessionStorage.getSession(
request.headers.get('cookie'),
)
const state = cuid()
connectionSession.set('oauth2:state', state)

// allows us to inject a code when running e2e tests,
// but falls back to a pre-defined 🐨 constant
const code =
request.headers.get(MOCK_CODE_GITHUB_HEADER) || MOCK_CODE_GITHUB
const searchParams = new URLSearchParams({ code, state })
let cookie = new SetCookie({
name: 'github',
value: searchParams.toString(),
path: '/',
sameSite: 'Lax',
httpOnly: true,
maxAge: 60 * 10,
secure: process.env.NODE_ENV === 'production' || undefined,
})
throw redirect(`/auth/github/callback?${searchParams}`, {
headers: {
'set-cookie':
await connectionSessionStorage.commitSession(connectionSession),
'Set-Cookie': cookie.toString(),
},
})
}
Expand Down
2 changes: 1 addition & 1 deletion app/utils/providers/provider.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { type Strategy } from 'remix-auth'
import { type Strategy } from 'remix-auth/strategy'
import { type Timings } from '../timing.server.ts'

// Define a user type for cleaner typing
Expand Down
Loading
Loading