Skip to content

Implement Clerk authentication with @wisc.edu and GitHub org restrict…#12

Open
Fangrui-Zhang wants to merge 14 commits intomainfrom
authentication---clerk
Open

Implement Clerk authentication with @wisc.edu and GitHub org restrict…#12
Fangrui-Zhang wants to merge 14 commits intomainfrom
authentication---clerk

Conversation

@Fangrui-Zhang
Copy link
Copy Markdown
Contributor

  • Add @clerk/expo integration with ClerkProvider and secure token caching
  • Create sign-in screen with Google and GitHub OAuth options
  • Implement authentication guard to protect app routes
  • Add webhook handler to validate @wisc.edu emails and badgerloop-software org membership
  • Update ProfileScreen with user data from Clerk and sign-out functionality
  • Add comprehensive setup documentation (CLERK_QUICKSTART.md, CLERK_AUTH_SETUP.md)
  • Configure expo-secure-store for token storage

…ions

- Add @clerk/expo integration with ClerkProvider and secure token caching
- Create sign-in screen with Google and GitHub OAuth options
- Implement authentication guard to protect app routes
- Add webhook handler to validate @wisc.edu emails and badgerloop-software org membership
- Update ProfileScreen with user data from Clerk and sign-out functionality
- Add comprehensive setup documentation (CLERK_QUICKSTART.md, CLERK_AUTH_SETUP.md)
- Configure expo-secure-store for token storage
Copilot AI review requested due to automatic review settings March 9, 2026 03:44
- Required by @clerk/expo package
- Satisfies expo-doctor peer dependency check
- Prevents CI/CD failures
- expo-modules-core is required as peer dependency by @clerk/expo
- expo-doctor has conflicting checks: peer dep vs direct install
- CI now accepts this specific known warning while catching other issues
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR integrates Clerk authentication into the SC2 Mobile App, restricting access to users with @wisc.edu Google accounts or membership in the badgerloop-software GitHub organization.

Changes:

  • Adds @clerk/clerk-expo and expo-secure-store packages, wraps the app in ClerkProvider with secure token caching, and implements an auth guard that redirects unauthenticated users to a new sign-in screen
  • Creates a sign-in screen with Google and GitHub OAuth buttons and updates ProfileScreen with real Clerk user data and sign-out functionality
  • Adds a serverless webhook handler (deployable to Vercel/AWS Lambda) that validates new users by email domain or GitHub org membership, along with comprehensive setup documentation

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
app/_layout.js Wraps app in ClerkProvider with expo-secure-store token caching
app/(tabs)/_layout.js Adds auth guard via useAuthGuard hook; has a React hooks ordering bug
app/sign-in.js New sign-in screen with Google/GitHub OAuth buttons; uses wrong OAuth API for mobile
src/hooks/useAuthGuard.js Auth redirect hook; has unused variable
src/screens/ProfileScreen.js Populates profile from Clerk user data; adds sign-out menu item
app.config.js Adds expo-secure-store plugin
package.json Adds @clerk/expo and expo-secure-store dependencies
package-lock.json Lock file updated with new dependencies
webhook/clerk-webhook-handler.js Webhook for validating new signups; has svix body and org check bugs
webhook/api/clerk.js Vercel route entry point
webhook/vercel.json Vercel deployment config; has incorrect file paths
webhook/package.json Webhook-specific package config
webhook/README.md Webhook deployment documentation
docs/CLERK_QUICKSTART.md Quick-start guide for Clerk setup
docs/CLERK_AUTH_SETUP.md Detailed Clerk authentication setup guide
.env.example Documents EXPO_PUBLIC_CLERK_PUBLISHABLE_KEY

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +14 to +16
if (!isLoaded || !isSignedIn) {
return null;
}
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

When TabLayout returns null (while auth is loading or the user is not signed in), the useEffect hook for injecting CSS (which conditionally depends on the null early return being bypassed) is placed after the early return. This violates the React Rules of Hooks: hooks must not be called conditionally or after an early return. The current layout calls useAuthGuard() and checks !isLoaded || !isSignedIn before the useEffect, meaning the effect is never called when auth is pending — but React will still track it as a conditional hook call position, which can cause issues. The useEffect must be moved before any conditional return.

Copilot uses AI. Check for mistakes.
Comment on lines +82 to +110
<TouchableOpacity
style={[styles.button, styles.googleButton]}
onPress={handleGoogleSignIn}
disabled={loading}
>
{loading ? (
<ActivityIndicator color="#fff" />
) : (
<>
<Ionicons name="logo-google" size={24} color="#fff" />
<Text style={styles.buttonText}>Sign in with Google</Text>
</>
)}
</TouchableOpacity>

<TouchableOpacity
style={[styles.button, styles.githubButton]}
onPress={handleGitHubSignIn}
disabled={loading}
>
{loading ? (
<ActivityIndicator color="#fff" />
) : (
<>
<Ionicons name="logo-github" size={24} color="#fff" />
<Text style={styles.buttonText}>Sign in with GitHub</Text>
</>
)}
</TouchableOpacity>
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

The loading state is shared between both the Google and GitHub sign-in buttons, so clicking one button while the other is loading will disable both buttons with no visual differentiation. More critically, when Google sign-in is in progress and loading is true, the GitHub button also shows a spinner instead of its label. Consider using separate loading state per button (e.g., googleLoading and githubLoading) so each button independently reflects its own loading state.

Copilot uses AI. Check for mistakes.
Comment on lines +192 to +196
async function lambdaHandler(event) {
const req = {
body: JSON.parse(event.body),
headers: event.headers,
};
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

In lambdaHandler, the Lambda event body is decoded with JSON.parse(event.body), but event.body can be base64-encoded when API Gateway is configured with binary media types or isBase64Encoded: true. There is no check for event.isBase64Encoded. More importantly, after JSON.parse, the same svix signature verification issue applies: handleClerkWebhook re-serializes the body with JSON.stringify(payload), losing the original raw bytes needed for HMAC verification.

Copilot uses AI. Check for mistakes.
Comment on lines +20 to +43
const handleGoogleSignIn = async () => {
if (!signIn || !signUp) return;

setLoading(true);
try {
// Start OAuth flow with Google
const { createdSessionId, setActive } = await signIn.create({
strategy: 'oauth_google',
});

if (createdSessionId) {
await setActive({ session: createdSessionId });
router.replace('/');
}
} catch (err) {
console.error('Google sign in error:', err);
Alert.alert(
'Sign In Error',
err.errors?.[0]?.message || 'Failed to sign in with Google'
);
} finally {
setLoading(false);
}
};
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

The OAuth sign-in flow uses signIn.create({ strategy: 'oauth_google' }) which is a web-based API. For Expo (native iOS/Android), Clerk's recommended approach is to use the useOAuth hook from @clerk/clerk-expo and call startOAuthFlow(), which opens the system browser via expo-web-browser. The current implementation will not correctly open the OAuth browser on native platforms and the returned createdSessionId will be falsy. Additionally, expo-web-browser and expo-auth-session (required peer dependencies for useOAuth) are not installed in package.json. The same issue exists in handleGitHubSignIn.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

This application can we launched in th web version or the mobile version. In that case, dosen't it make sense to check the platform, and handle authentication according to that?

useEffect(() => {
if (!isLoaded) return;

const inAuthGroup = segments[0] === '(auth)';
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

The inAuthGroup variable is declared and assigned but never used. This is dead code that should either be removed or used in the guard logic.

Suggested change
const inAuthGroup = segments[0] === '(auth)';

Copilot uses AI. Check for mistakes.
Comment on lines +35 to +36
phone: user?.primaryPhoneNumber?.phoneNumber || '+1 (555) 123-4567',
memberSince: user?.createdAt ? new Date(user.createdAt).getFullYear() : '2023',
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

The profileData object is reconstructed on every render with user?.createdAt ? new Date(user.createdAt).getFullYear() : '2023'. However, createdAt in Clerk's JavaScript SDK is a Date object, not a number. Calling new Date(user.createdAt) on an already-existing Date object works (though redundant), but this is a minor concern. More importantly, the fallback phone number '+1 (555) 123-4567' is a placeholder that leaks fake data when a user has no phone number set. Consider using an empty string or 'Not set' as the fallback instead.

Suggested change
phone: user?.primaryPhoneNumber?.phoneNumber || '+1 (555) 123-4567',
memberSince: user?.createdAt ? new Date(user.createdAt).getFullYear() : '2023',
phone: user?.primaryPhoneNumber?.phoneNumber || 'Not set',
memberSince: user?.createdAt ? user.createdAt.getFullYear() : '2023',

Copilot uses AI. Check for mistakes.
app/sign-in.js Outdated
Comment on lines +10 to +16
import { useSignIn, useSignUp } from '@clerk/clerk-expo';
import { useRouter } from 'expo-router';
import { Ionicons } from '@expo/vector-icons';

export default function SignInScreen() {
const { signIn } = useSignIn();
const { signUp } = useSignUp();
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

The useSignUp hook is imported and destructured but signUp is never used in either handleGoogleSignIn or handleGitHubSignIn. The only usage of signUp is in the guard if (!signIn || !signUp) return;. This import is unnecessary for the current OAuth flow implementation.

Copilot uses AI. Check for mistakes.
</TouchableOpacity>

<Text style={styles.disclaimer}>
Only @wisc.edu emails and Badgerloop-software organization members can
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

The disclaimer text says "Badgerloop-software" with an uppercase "B" in "software", which is inconsistent with the actual organization name "badgerloop-software" (all lowercase). This could confuse users or developers trying to reference the correct organization name.

Suggested change
Only @wisc.edu emails and Badgerloop-software organization members can
Only @wisc.edu emails and badgerloop-software organization members can

Copilot uses AI. Check for mistakes.
username: username,
});

return data !== undefined;
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

The checkMembershipForUser API from @octokit/rest returns HTTP 204 (No Content) when the user is a confirmed member. In this case, data is an empty object {} or undefined, so the check data !== undefined will evaluate to true only by coincidence or to false depending on the Octokit version and response parsing — the logic is unreliable. The correct approach is that any successful (non-throwing) response from this call means membership is confirmed. The return value inside the try block should simply be return true; (since the call only throws on non-membership), not return data !== undefined.

Suggested change
return data !== undefined;
return true;

Copilot uses AI. Check for mistakes.
Comment on lines +5 to +12
"src": "webhook/api/clerk.js",
"use": "@vercel/node"
}
],
"routes": [
{
"src": "/api/clerk-webhook",
"dest": "webhook/api/clerk.js"
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

The vercel.json file lives in webhook/, but the src and dest paths reference webhook/api/clerk.js — these are paths that Vercel resolves relative to the project root, not relative to the vercel.json file itself. Since Vercel is being deployed from the webhook/ directory (as instructed in the README), these paths should be api/clerk.js (relative to the webhook/ root), not webhook/api/clerk.js. With the current paths, Vercel would fail to find the source file during build and route matching would also be incorrect.

Suggested change
"src": "webhook/api/clerk.js",
"use": "@vercel/node"
}
],
"routes": [
{
"src": "/api/clerk-webhook",
"dest": "webhook/api/clerk.js"
"src": "api/clerk.js",
"use": "@vercel/node"
}
],
"routes": [
{
"src": "/api/clerk-webhook",
"dest": "api/clerk.js"

Copilot uses AI. Check for mistakes.
Fangrui-Zhang and others added 6 commits March 8, 2026 22:56
- Add '|| true' to prevent early exit on expo-doctor failure
- Allow script to evaluate grep conditions before deciding pass/fail
- Accept known expo-modules-core warning from @clerk/expo
- Updated all imports to use correct package name @clerk/expo
- Fixes CI build error: module not found @clerk/clerk-expo
- Files updated: _layout.js, sign-in.js, useAuthGuard.js, ProfileScreen.js
- Install expo-web-browser required by @clerk/expo for OAuth flows
- Add expo-web-browser plugin to app.config.js
- Fixes CI error: Unable to resolve module expo-web-browser
…sion

- Replace signIn.create() with useOAuth() hook for proper OAuth flow
- Install expo-auth-session package required for OAuth
- Add expo-web-browser import and maybeCompleteAuthSession
- Add debug logging for OAuth flow
- OAuth now successfully creates sessions
Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 23, 2026

@sbasu107 I've opened a new pull request, #15, to work on those changes. Once the pull request is ready, I'll request review from you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants