Server-side user authentication+previewing, and server- and client-side error logging #274
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Benaiah
commented
Jul 5, 2023
Benaiah
commented
Jul 5, 2023
LouisFettet
reviewed
Jul 6, 2023
There was a problem hiding this comment.
Local issues I'm having aside, this is looking good. Appreciate all the tests and comments! I definitely owe this a more thorough review when I get back from PTO if y'all haven't merged it by then.
I wasn't able to get Docker working, but I was able to get things running with a normal install of Redis. The initial auth is the only thing that's working for me though; I'm able to provide a token and it gives me a success message, but then when redirecting back to a URL with ?preview I get a 401 error telling me to log in again (this isn't an issue on non-preview pages; everything looks right and I get the welcome message and logout link in the header). Not sure if this is intentional or not for this stage, or if you're seeing something different on your end, so I figured I would call it out.
Also, is there a timeout on the Redis connection? After a few minutes of inactivity I notice that I get a Socket closed unexpectedly error with the Redis client. Could be something wrong with my install if you're not seeing this though.
I've got some other comments below; only other note is that it might be good to update the README with instructions for running with Docker / installing Redis.
src/lib/components/ContentfulLoginLink/ContentfulLoginLink.svelte
Outdated
Show resolved
Hide resolved
src/lib/components/ContentfulLoginLink/ContentfulLoginLink.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Would be good to write a test for this, even if it doesn't cover the on:click handler (I assume that would be a bit of a mocking nightmare).
Comment on lines
+13
to
+17
| // Regular setContext _only exists on the server_. On the client, _always_ use | ||
| // setPublicContext. setPublicContext can also be used on the server to set context that will be | ||
| // accessible by the client logger (so it must be serializable!). Note that "public" means "public | ||
| // to the browser session of the current user", so things like the current user data are safe to store | ||
| // in public context because only that user will see them. |
There was a problem hiding this comment.
Thanks for this note! Maybe to avoid confusion we could replace Public with UserSession instead? Not sure if that's any more or less accurate but I am worried that "public" could be misinterpreted when reading it outside of this file.
There was a problem hiding this comment.
Hmm, I don't like that name because the backend logging is also for a specific user session. What about replacing Public with Client?
src/lib/util/catchMeIfYouCan.ts
Outdated
Benaiah
force-pushed
the
benaiah/server-side-previewing-squashed-and-rebased
branch
from
e6e95ec to
5cb7ca3
Compare
|
Coverage after merging benaiah/server-side-previewing-squashed-and-rebased into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Reports for acc11ec have been deployed to Vercel: |
hinzed1127
reviewed
Jul 12, 2023
There was a problem hiding this comment.
Thanks for all your work on this, it's a lot but looks pretty solid. I had a few small documentation things to note, but otherwise not a whole lot jumped out.
Local development is broken for me right now (I'm currently getting a 500 page with "got malformed submenu"), which I believe is due to some of the recent navigation updates. I'll give things another go locally to make sure things run as expected once that's sorted out, but other than that, stuff looks good 👍
| @@ -0,0 +1 @@ | |||
| export default 60 * 60 * 24 * 7; | |||
There was a problem hiding this comment.
maybe just add a clarifying comment that this is (if I'm correctly interpreting) creating a week-long token where the base unit is 1 second that's getting multiplied through seconds*minutes*hours*days
| params, | ||
| locals: { contentfulClient }, | ||
| }): Promise<{ text: string }> => { | ||
| // TODO: create fallback fixture |
There was a problem hiding this comment.
just highlighting this in case we want to specifically capture this in a follow-up story
Benaiah
force-pushed
the
benaiah/server-side-previewing-squashed-and-rebased
branch
from
03a88b8 to
4160c7e
Compare
Benaiah
force-pushed
the
benaiah/server-side-previewing-squashed-and-rebased
branch
2 times, most recently
from
5426db4 to
5163ba1
Compare
2 tasks
SQUASHME: client creation in Nav rename contentful implementation file remove unnecessary type declaration don't override page data with the document; pass query to frontend rename contentful/client to contentful/graphqlClient convert under construction page to use GraphQL client fix e2e header tests fix loadMainNav when there is no Contentful connection remove unnecessary query variable from page data add server-side contentful previewing w/contentful auth ignore unused vars that start with _ improve server loading enable previewing in nav graphql query WIP fix import add management client to try block delete cookie on logout don't store the user token in localStorage; let it expire fix oauth callback page make sure user is activated add some debug logging decode the cookie values fiddling with the cookie move logout logic to an action use sveltekit's fetch function instead of global fetch when possible debugging reduce token scope can we use the user's token instead of a global one? move codegen config; separate schema and schema type generation switch default scalar type to "unknown" to avoid "any" pollution update package scripts to generate schema types correctly make preview authentication universal let contentful embed the site fix package scripts throw preview authentication errors in layout not handler fix broken oauth link request correct token WIP disable layout error WIP catch preview auth errors with an actual layout import component remove unused preview client add missing baseURL variable stop throwing errors in the root layout make errors more detailed silly mistake another silly mistake set CSP in a hook in addition to vercel settings try it again try with x-frame-options allow cookies to pass to frame fix sameSite setting WIP does this work fix cookie samesite handling tighten up CSP improve CSP headers create the server-side contentful client in a single place add previewing to all gql queries update schemas fix homepage fix linting; add query variable typing add a trailing newline add new env variables to env.d.ts make LoginLink accept anchor element props open login link in new window if it's in an iframe fix a test make contentful oauth endpoint an environment variable refresh the page on logout make LoginLink a little nicer improve parseHashQuery parseHashQuery should return Record<string, string> logout should only be available in the browser run prebuild before building storybook avoid errors when $page or $page.url are undefined update schemas use null to explicitly delete cookie and localStorage entries improve cookie deletion add trailing newline reuse setCookie to define deleteCookie undo "fix" to parseHashQuery add preview parameter when navigating from a preview set preview to previous preview value use workaround to preserve query parameter cf. sveltejs/kit#10122 small refactors to root layout only add preview param to URLs that don't already have it move a variable refactor logout action with "get" from svelte/store improve graphqlClient tests get current user on the server fix User component work around weird test error use new /login and /logout endpoints for http-only contentful auth remove unnecessary type annotations make login/logout logic work without hard refreshing mock $app/stores to fix tests disable flaky eslint rule that's already covered by ts use "import type" in app.d.ts remove unused imports add tests of cookie functionality more tests of cookies remove a bunch of dead code various changes prior to setting up redis token handling tests for handleToken set up docker compose with redis redis authentication complete; still need to fix tests update office/organization page to support new preview functionality also queries based on the slug instead of grabbing all office page entries support passing "unknown" to ContentfulRichText fix test-contentful-content page lots of testing of handleToken; some improvements to it shorten a line don't merge office page with layout data add missing test file lots of testing and improvements lots more progress and tests, including adding a logging service add handleError hooks to the client and server set url context in handleError on both client and server add some comments rearrange logging code; fix tests and dev; use new message format fix a type error remove a weird emacs thing in tsconfig stop exposing logger on window expand some comments fix file capitalization fix syntax error add type definitions for new env variables add example values for new env variables remove .only from describe remove unused imports simplify ContentfulLoginLink show footer when there is a previewAuthenticationError use Link for special links; fix a LoginLink bug update schema and migrate from "metadata" to "pageMetadata" await error logging in handleError actually run the error handling fix some types fix some prop types improve the nav loading to get rid of type assertions fix test data move redis client creation into its own hook fix type errors don't import devalue parse ahead of time fix type errors get the redis client working in docker get types checking and some tests passing again fix nav fix a type issue move server hooks into their own files (still need to fix tests) big refactoring and improved tests remove old redis service move contentful service into a server-only folder improvements to error handling and logging use dynamic env for KV_URL run end-to-end tests with docker compose fix formatting don't show logout button when not logged in small updates to login page fix bad import add comment to token duration
Benaiah
force-pushed
the
benaiah/server-side-previewing-squashed-and-rebased
branch
from
5163ba1 to
f9c6176
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Jira ticket: LDAF-201
Proposed changes
Adds a
handleTokenhook that authenticates provided user tokens (in either theAuthorizationheader or anHttpOnlyldafUserTokencookie) against a Redis instance (set up in production and runnable locally using eitherredis-serverwith no arguments ordocker compose up) containing user objects that list the data required to render the current user (which is also sent to the root layout and the browser) and the user's Contentful Management API token (which is not). (Note that theldafUserTokenis not the Contentful Management API token.)The
handleTokenhook initializes the Contentful client that requests normal Contentful content.In the
handleTokenhook, if the request is for a url containing apreview, gets the management API token from Redis and makes a call to the Contentful space to ensure the user is still allowed to view preview content in that space. If the authentication+authorization succeeds, we replace the Contentful client with one that uses the preview API token and passes apreview: truevariable to every GraphQL request (I haven't found a good way to automatically pass the variable as an argument to the queries, so the queries do need to be written with thepreviewvariable in mind for previewing to work).Adds
/loginroute that has a default form action taking aPOSTrequest containing a Contentful Management API token, authenticates with Contentful using the token (it only requests the current user's information - the actual authorization to see unpublished content in the space is done on every preview request by thehandleTokenhook), and then stores the user info in Redis to use for further authentication.Adds
/logoutroute that clears theldafUserTokencookie and (attempts to) delete the stored user info in Redis (this is given an expiration time of one week in Redis itself, so unsuccessful deletes will not leave data around indefinitely).Adds an OAuth2 callback at
/callback/oauth2that receives a request from Contentful containing a Management API token (which is passed in the hash, so the OAuth2 callback requires client-side JS to work - you can still log in without JS by entering a personal access token on the/loginpage) and then requests/loginfrom the client side to save the token to Redis. This is the only time that client-side code has access to the Management API token (and the Management API token is not stored in the cookie either, so we're not sending it to third parties in HTTP requests - the cookie has to beSameSite=Nonein order to work inside the iframe used by the Contentful preview in the Contentful interface.)Adds a "Login with Contentful" link to the
/loginpage that takes you to Contentful, where you can log in and authorize the OAuth app to use your account and then be redirected back to the OAuth2 callback page to save the Contentful Management API token to Redis.Adds a server-side and client-side logger that currently logs to the console (in dev or tests) or to the #ldaf-eng-logs Slack channel (in production, including
npm run build && npm run preview).Adds
handleErrorhooks to both the server and the client that use the respective logger.Sets the appropriate
Content-Security-Policyto allow embedding the site in the preview iframe in the Contentful interface.Adds lots of tests for everything above (except the logger, which was verified manually - it's possible to test the logger as well and we should probably do it eventually but this PR is already quite large).
Adds a
Dockerfileanddocker-compose.ymlthat allow spinning up other services in addition to the main web app. This can be completely ignored if you don't want to deal with Docker - the site works (or at least should work) exactly the same without Redis running if you're not requesting preview content. Even if you need to work with Redis, you can runredis-serverwith no arguments and then start the app and everything including user sessions and authentication will work.Acceptance criteria validation
CONTRIBUTING.mdexplaining how to work with all the new features like logins and logging in development.Requested feedback
See my upcoming self-review.