Add Discord verification to user profiles #12
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
CodeSee Review Map:Review in an interactive map View more CodeSee Maps Legend |
There was a problem hiding this comment.
This looks pretty good! I made a bunch of suggestions, questions, and comments inline, mostly to delete or improve some comments in the code. Most are non-blocking, please take a moment to review, and possibly address before merging this?
General thought/suggestion: Given that we're using a NoSQL store, do we have any documentation of the structure/schema of the objects we're storing in it? I think such a document could be useful given that the store itself is effectively schemaless.
:🚢
| import { updateUser } from "~/database.server"; | ||
|
|
||
| export const loader: LoaderFunction = async ({ request }) => { | ||
| const session = await getSession(request.headers.get("Cookie")); |
There was a problem hiding this comment.
Thought/suggestion (non-blocking): The code to get the session and currentUser, and redirect to /login if the user is not logged in feels like it will repeat a lot. Does @remix-run have any concept of middleware? That feels like the place for this sort of thing?
There was a problem hiding this comment.
I agree, and they do! https://linear.app/codesee/issue/ENG-615/middleware-for-user-auth
app/routes/verify-discord.ts
Outdated
| throw new Error("No code"); | ||
| } | ||
|
|
||
| // Grab an access token from the user's code |
There was a problem hiding this comment.
Suggestion (non-blocking): This comment feels a little redundant given that you chose good names for the variable and function below. Consider deleting? If the function being called doesn't have a docstring, consider adding one where it is defined?
There was a problem hiding this comment.
You're right, I'll remove it.
app/routes/verify-discord.ts
Outdated
| // Grab an access token from the user's code | ||
| const accessToken = await getDiscordAccessToken(code); | ||
|
|
||
| // Grab the user's Discord id |
There was a problem hiding this comment.
Suggestion (non-blocking): This comment feels a little redundant given that you chose good names for the variable and function below. Consider deleting? If the function being called doesn't have a docstring, consider adding one where it is defined?
app/routes/verify-discord.ts
Outdated
| // Grab the user's Discord id | ||
| const discordUserId = await getDiscordUserId(accessToken); | ||
|
|
||
| // Save the user's Discord id in our database |
There was a problem hiding this comment.
Suggestion (non-blocking): This comment feels a little redundant given that you chose good names for the code below. Consider deleting?
app/routes/verify-discord.ts
Outdated
| }; | ||
|
|
||
| export const action: ActionFunction = async () => { | ||
| // Make sure the user is logged in |
There was a problem hiding this comment.
Suggestion (blocking): I think these comments could be deleted or improved. Are they TODO statements? Are they saying what the code does?
There was a problem hiding this comment.
They were TODOs from when I was figuring out how to do the thing. I'll remove them!
| @@ -124,6 +123,26 @@ export async function updateProfileForUser( | |||
| .set(updatedProfile, { merge: true }); | |||
| } | |||
|
|
|||
| /** | |||
| * Updates a User in the database. The following fields are stripped from the | |||
There was a problem hiding this comment.
Praise: Thanks for writing good docstrings!
| import DiscordOauth2 from "discord-oauth2"; | ||
|
|
||
| const DISCORD_REDIRECT_URI = | ||
| process.env.NODE_ENV === "production" |
There was a problem hiding this comment.
Thought (non-blocking): If we do checks like this in many places, we might want to adopt something like the codesee_metaconfig.json file for storing non-sensitive constants that are env specific.
There was a problem hiding this comment.
Good idea. This is the only place where we're doing this so far but I'll keep an eye on it.
| ? "https://opensourcehub.io/verify-discord" | ||
| : "http://localhost:3000/verify-discord"; | ||
|
|
||
| const DISCORD_SCOPES = ["identify"]; |
There was a problem hiding this comment.
Suggestion (non-blocking): Is there any documentation of Discord's available scopes? Maybe add a link to such documentation in a comment?
|
|
||
| const token = await discordAuth.tokenRequest({ | ||
| clientId: process.env.DISCORD_CLIENT_ID, | ||
| clientSecret: process.env.DISCORD_CLIENT_SECRET, |
There was a problem hiding this comment.
Question (maybe blocking): How are we keeping these values secret?
There was a problem hiding this comment.
They're pulled from a .env file that isn't checked into source control. These variables will only be available on production and, because all this code happens on the server, they won't be exposed to any client. I do need to fail gracefully for local development, though.
|
|
||
| const url = oauth.generateAuthUrl({ | ||
| scope: ["identify"], | ||
| state: crypto.randomBytes(16).toString("hex"), |
There was a problem hiding this comment.
Praise: Thanks for using a real source of entropy!
|
@cubes to answer your question:
The schema is documented in Coda and the types are an exact match for the shape of the database. |
Thanks @deammer, you've thought of everything. 😄 |
Enable users to tie their Discord profile with their OSH account. Users who are logged in will now see a "Verify with Discord" button on their own profile page. This takes them through the OAuth flow, allowing us to store their Discord id in our database. We don't store anything else! Once we have a Discord id, we know users have verified and we can display a badge on their profile (that only they can see, for privacy.)
Here's what that looks like:
Screen.Recording.2022-05-19.at.1.12.19.PM.mov