Refactor auth and enable static file serving #577

elliotbonneville · 2022-05-27T22:30:35Z

Purpose

Make it possible to run the Next application without relying on the Node server by completely rewriting authentication layer and disabling image optimization.

Changes

Rewrite authentication layer and introduce auth slice to store auth state client-side
Remove next-redux-wrapper and all hydration since we no longer render anything serverside
Clean up user management code to use new auth slice
Disable image optimization
Update the build script to execute next build && next export to the out dir for static files

Checklist

Ticket

Fixes https://github.com/ethyca/fidesui/issues/2

elliotbonneville · 2022-06-07T18:52:53Z

clients/admin-ui/src/features/privacy-requests/DenyPrivacyRequestModal.tsx

@@ -44,37 +44,37 @@ const DenyPrivacyRequestModal = ({
    returnFocusOnClose={false}
  >
    <ModalOverlay />
-    <ModalContent width='100%' maxWidth='456px'>
+    <ModalContent width="100%" maxWidth="456px">


@TheAndrewJackson I know you had set jsxSingleQuote to true, but is there any chance we can keep double quotes in JSX? I've never worked with JSX single quotes before. I don't want to be pedantic but I'm wondering what the purpose of them is? Just consistency?

We can definitely keep double quotes. I used single because I was following someone else who used single quotes. If double quotes are preferred then let's use them instead. Especially if the fidesctl repo is using double quotes.

It's not a big deal anyway. As long as the formatter automatically sets it I'm fine with either.

Cool. Let's update the Prettier file to use double quotes then – thanks for your flexibility here!

Ah yes, the great debate.

TheAndrewJackson

It looks good! I like a lot of the refactors and the tests. I finished my first pass of reading all of the code. I just have a couple small nits and questions. I learned a few things while reading this PR!

I'm going to do some functional testing now and see if anything visually pops up that I couldn't see while reviewing it.

TheAndrewJackson · 2022-06-07T20:15:33Z

clients/admin-ui/README.md


-To enable stable authentication you must supply a `NEXTAUTH_SECRET` environment
-variable. The best way to do this is by creating a `.env.local` file, which Next


Is the .env.local file no longer needed for auth to work properly?

Nope. This was an artifact of the previous library, which we're no longer using.

TheAndrewJackson · 2022-06-07T20:24:31Z

clients/admin-ui/src/app/store.ts

+      storedAuthState = JSON.parse(storedAuthStateString);
+    } catch (error) {
+      // eslint-disable-next-line no-console
+      console.log(error);


This is minor. We could have the log show up as an error.

Suggested change

console.log(error);

console.error(error);

Good one! Let's do that.

TheAndrewJackson · 2022-06-07T20:26:56Z

clients/admin-ui/src/features/auth/ProtectedRoute.tsx

+  const router = useRouter();
+  const token = useSelector(selectToken);
+
+  // TODO: check for token invalidation


Is this still a TODO?

Yes. Pending input from @seanpreston regarding whether or not we should have a token validation endpoint. I left this out of this PR because it's not currently supported either, so while definitely nice to have and important, it's an additional feature here.

TheAndrewJackson · 2022-06-07T20:41:58Z

clients/admin-ui/src/features/auth/ProtectedRoute.tsx

+  children: JSX.Element;
+}
+
+const ProtectedRoute = ({


This is a cool component. I like this pattern.

TheAndrewJackson · 2022-06-07T20:48:22Z

clients/admin-ui/src/features/auth/auth.slice.ts

+} from '@reduxjs/toolkit';
+import { createApi, fetchBaseQuery } from '@reduxjs/toolkit/query/react';
+
+import type { RootState } from '../../app/store';


Oh this is cool. I didn't know about importing/exporting types. I'll make sure to use this from now on.

It's most helpful when you need types from two files that depend on one another – it avoids the circular dependency problem because the types can be statically inferred and the files don't need to be imported and executed, which is where you'd get that import loop.

TheAndrewJackson · 2022-06-07T20:58:54Z

clients/admin-ui/src/features/auth/auth.slice.ts

+
+export const { login, logout } = authSlice.actions;
+
+export const credentialStorage = createListenerMiddleware();


createListenerMiddleware seems really convenient. I didn't know RTK had it. I like it.

Yup! They make it really easy to build middleware in, and this is where any side effects of actions should be specified (you don't want to do that in reducers, since they need to be pure and not affect any external state whatsoever).

TheAndrewJackson · 2022-06-07T21:06:57Z

clients/admin-ui/src/features/auth/auth.slice.ts

+    prepareHeaders: (headers, { getState }) => {
+      const token = selectToken(getState() as RootState);
+      headers.set('Access-Control-Allow-Origin', '*');
+      if (token) {
+        headers.set('authorization', `Bearer ${token}`);
+      }
+      return headers;
+    },


It looks like all of the prepareHeaders in each createApi have the same code. Maybe the logic could be refactored out into one function that is shared between them.

I think there might be some new headers that get added to every request in an upcoming update for fideslog for analytics. Something like X-Fides-Source: fidesops-admin-ui. That would help simplify adding headers that are applied to each request.

Yep, pretty sure they do! That's a great idea.

TheAndrewJackson · 2022-06-07T21:14:06Z

clients/admin-ui/src/features/common/Image.tsx

@@ -0,0 +1,11 @@
+// components/Image.js
+import NextImage from 'next/image';


Is there any benefit to using the NextImage over Chakra UIs Image since we aren't optimizing images anymore?

https://v1.chakra-ui.com/docs/components/media-and-icons/image

Hmm, I would say the only advantage is that it leaves the door open for easily refactoring to include image optimization when we do re-introduce the Node server, but honestly that's pretty trivial. I mainly made this change to avoid having to update any places where the Image component was being used in order get static building working.

In the future we should probably just reach for Chakra's Image component.

TheAndrewJackson · 2022-06-08T13:13:37Z

clients/admin-ui/src/pages/_app.tsx

 import theme from '../theme';

 if (process.env.NEXT_PUBLIC_MOCK_API) {
  // eslint-disable-next-line global-require
  require('../mocks');
 }

+const SafeHydrate: React.FC = ({ children }) => (
+  <div suppressHydrationWarning>
+    {typeof window === 'undefined' ? null : children}


What is the hydration warning saying and doing? I'm guessing it's an annoying error message that we don't need while developing

That's correct. Basically it's noting that what gets rendered on the server and what gets rendered on the client isn't the same, because we're no longer taking advantage of serverside rendering. This won't apply at all without a server, so the warning is meaningless.

TheAndrewJackson · 2022-06-08T13:16:05Z

clients/admin-ui/src/pages/user-management/new.tsx

-                  User Management
-                </BreadcrumbLink>
-              </BreadcrumbItem>
+  <ProtectedRoute>


The ProtectedRoute component seems more elegant than the original getServerSideProps
solution. Just wrap the JSX with 1 more component and it's being authenticated.

Yep! To be fair, we can definitely improve on the getServerSideProps pattern, but I hadn't cleaned that up because I knew we were intending to rip it out.

I will say that one thing I noticed with this pattern is that it's easily possible to forget to authenticate a page, so we just need to either a) wrap all of the pages by default at the App.tsx level, or stay on top of that while developing. I'm not super concerned, since the requests wouldn't be authenticated anyhow, so even if we do forget to wrap a page it's not the end of the world – and we don't create pages that frequently, so the error surface area is low. But it's just another place where we could introduce an error.

To be fair, that same logic applied to the previous solution too, so I guess it's not a new problem. Something to think about...

TheAndrewJackson · 2022-06-08T14:04:08Z

clients/admin-ui/src/pages/subject-request/[id].tsx

+  const { data } = useGetAllPrivacyRequestsQuery({
+    id: Array.isArray(id) ? id[0] : id,
+    verbose: true,
+  });


In my functional testing I found that this page was doing 2 requests to the backend. The first was with no filtering id so it would get back multiple privacy requests. The second would have the id from the url as expected so it would filter and get the data.

This update makes it skip getting the data the url until the const { id = '' } = router.query; line above has parsed the id as expected. Now it will only do 1 request.

Suggested change

const { data } = useGetAllPrivacyRequestsQuery({

id: Array.isArray(id) ? id[0] : id,

verbose: true,

});

const { data } = useGetAllPrivacyRequestsQuery(

{

id: Array.isArray(id) ? id[0] : id,

verbose: true,

},

{

skip: id === '',

}

);

Ahh, really good catch. This is page is part I was the most worried about, since I just quickly made a few changes to get it up to date with the new auth system and no longer using the Next server. That's a great solution, and I didn't know about the skip flag!

TheAndrewJackson · 2022-06-08T14:27:53Z

clients/admin-ui/src/pages/subject-request/[id].tsx

  const body =
-    data?.items.length === 0 ? (
+    !data || data?.items.length === 0 ? (
      <Text>404 no subject request found</Text>


It looks like this page is flashing the 404 message for a split second while the page is loading the data. If the user has a slow connection it will display the 404 message while the page is loading. I guess we'll need to incorporate some kind of loading behavior now since we're no longer doing SSR. That might be out of scope of this PR though

Good spot. Between Chakra UI and Redux Toolkit Query we actually have everything we need to solve this in like 5min, so I pushed up a quick fix for this. :)

allisonking

so much refactoring, amazing! 🤩 I don't have much to add outside of what Andrew already caught (👏 ), just some comments here and there. thanks for doing this!!

allisonking · 2022-06-08T14:33:50Z

clients/admin-ui/__tests__/index.test.tsx

-    );
+  it('renders the Subject Requests page by default when logged in', () => {
+    render(<Home />, {
+      preloadedState: {


whoa this is very cool 🤩

allisonking · 2022-06-08T14:36:32Z

clients/admin-ui/package.json

  "private": true,
  "scripts": {
    "dev": "next dev",
    "dev:mock": "echo '🚨 Running with mock API'; NEXT_PUBLIC_MOCK_API=true next dev",
-    "build": "next build",
+    "build": "next build && next export",


we'll probably want to standardize this across the repos. fidesctl looks like this right now: https://github.com/ethyca/fides/blob/aking-706-edit-collection/clients/admin-ui/package.json/#L15-L17

Yeah. Do you know off the top of your head if Next can export to a location in a parent directory? Would simplify a lot of the copy / paste / file moving stuff.

looks like the answer is no, it has to stay in the project dir

https://nextjs.org/docs/api-reference/next.config.js/setting-a-custom-build-directory

distDir should not leave your project directory. For example, ../build is an invalid directory.

allisonking · 2022-06-08T14:38:11Z

clients/admin-ui/src/constants.ts

-export interface UserResponse {
-  id: string;
-}
+// eslint-disable-next-line import/prefer-default-export


I think there are >1 exports now, so don't need this line anymore?

Yup yup yup!

allisonking · 2022-06-08T14:40:22Z

clients/admin-ui/src/features/auth/auth.slice.ts

+} from '@reduxjs/toolkit';
+import { createApi, fetchBaseQuery } from '@reduxjs/toolkit/query/react';
+
+import type { RootState } from '../../app/store';


allisonking · 2022-06-08T14:49:58Z

clients/admin-ui/src/pages/login.tsx

-      }
+    if (!values.password) {
+      errors.password = 'Required';
+    }


a note for the future, I'd love to start using yup for validation

Sounds good. Happy to bring that in!

@allisonking Is there a benefit to using yup that I'm not seeing? 👀

I like that yup makes validation declarative (you declare all the requirements a field has to meet at the same time you define the form (i.e. required, minimum 8 letters, etc.) and then it'll do the validation on submit (or I think you can also do it on touch) so then you don't have to do manual validation by looking through all the fields yourself with a bunch of ifs. since validation is defined in a schema, you can also reuse that schema which is handy. it's also nice that formik supports yup out of the box and they recommend it in their tutorial too: https://formik.org/docs/tutorial#schema-validation-with-yup

elliotbonneville · 2022-06-08T15:51:41Z

Thanks very much for your thorough review and great catches, folks! Really appreciate you taking the time to look this over and again, sorry about such a giant PR! Heh.

LKCSmith · 2022-06-08T15:52:34Z

clients/admin-ui/src/app/store.ts

+    try {
+      storedAuthState = JSON.parse(storedAuthStateString);
+    } catch (error) {
+      // TODO: build in formal error logging system


What would this generally look like in practice?

LKCSmith · 2022-06-08T15:57:58Z

clients/admin-ui/src/features/common/Header.tsx

+            <MenuList shadow="xl">
+              <Stack px={3} py={2} spacing={0}>
+                <Text fontWeight="medium">{username}</Text>
+                {/* This text should only show if actually an admin */}


I think this is an artifact from when I was working on the admin ui. It's been a minute since I've worked on this, and you don't need to do this, but other than checking against a user's privileges, is there a way to check a users' "role"? Is it all based on specific privileges a user has? I'm a little confused on which privileges specifically make a user an "Admin" vs not.

LKCSmith · 2022-06-08T15:58:50Z

clients/admin-ui/src/features/privacy-requests/DenyPrivacyRequestModal.tsx

@@ -44,37 +44,37 @@ const DenyPrivacyRequestModal = ({
    returnFocusOnClose={false}
  >
    <ModalOverlay />
-    <ModalContent width='100%' maxWidth='456px'>
+    <ModalContent width="100%" maxWidth="456px">


Ah yes, the great debate.

LKCSmith · 2022-06-08T16:00:06Z

clients/admin-ui/src/features/user-management/EditUserForm.tsx

-            {/* Only the associated user can change their own password */}
-            {id === user.id && <UpdatePasswordModal id={id} />}
+            {/* Only the owner of this profile can change their password */}
+            {isOwnProfile ? <UpdatePasswordModal id={profileId} /> : null}


LKCSmith · 2022-06-08T16:00:38Z

clients/admin-ui/src/features/user-management/NewUserForm.tsx

            _hover={{ bg: 'primary.400' }}
            _active={{ bg: 'primary.500' }}
-            colorScheme='primary'
+            colorScheme="primary"
            // disabled={!(isValid && dirty)}


I dunno if this state is needed anymore.

LKCSmith · 2022-06-08T16:02:54Z

clients/admin-ui/src/pages/login.tsx

-      }
+    if (!values.password) {
+      errors.password = 'Required';
+    }


@allisonking Is there a benefit to using yup that I'm not seeing? 👀

TheAndrewJackson

I just finished my second pass and I think everything looks good now 🙂 I will be comfortable with merging once this PR also gets an approval from @allisonking and @LKCSmith

allisonking

🚀

This is blocking the merge. I think any leftover issues can be addressed in future PRs.

* Rewrite auth * Remove unnecessary API files * Improve error handling in new user creation * Enable static build export and serving * Remove next-redux-wrapper * Correct breaking ESLint file * Clean up linting errors * Clean up merge artifacts * Update existing tests to use new auth * Implement auth testing * Cleanup merge artifact * Cleanup merge artifacts * Update Subject Requests detail page to use new auth * Update docs and changelog * Stop colocating tests in pages/ directory * Require authentication for request details page * Update error logging to use console.error * Improve privacy request details page loading sequence * Remove extraneous comment

elliotbonneville added the DON'T MERGE label May 27, 2022

elliotbonneville force-pushed the feature/refactor-auth branch from 1c2fa6b to 4e8a0af Compare May 30, 2022 19:13

seanpreston assigned TheAndrewJackson Jun 1, 2022

elliotbonneville marked this pull request as draft June 2, 2022 15:19

elliotbonneville added 12 commits June 7, 2022 13:49

Rewrite auth

5533c70

Remove unnecessary API files

a1e054b

Improve error handling in new user creation

7555497

Enable static build export and serving

ac050e6

Remove next-redux-wrapper

190a741

Correct breaking ESLint file

c0876d0

Clean up linting errors

3760c8b

Clean up merge artifacts

156647c

Update existing tests to use new auth

1aac32e

Implement auth testing

09b8925

Cleanup merge artifact

8855660

Cleanup merge artifacts

75fbdd0

elliotbonneville force-pushed the feature/refactor-auth branch from 2189c2d to 75fbdd0 Compare June 7, 2022 18:46

elliotbonneville commented Jun 7, 2022

View reviewed changes

elliotbonneville removed the DON'T MERGE label Jun 7, 2022

elliotbonneville marked this pull request as ready for review June 7, 2022 18:57

elliotbonneville added 4 commits June 7, 2022 15:51

Update Subject Requests detail page to use new auth

0439fe9

Update docs and changelog

0148c81

Stop colocating tests in pages/ directory

d3e351d

Require authentication for request details page

819d657

elliotbonneville changed the title ~~[WIP] Refactor auth and enable static file serving~~ Refactor auth and enable static file serving Jun 8, 2022

elliotbonneville requested a review from allisonking June 8, 2022 03:29

TheAndrewJackson reviewed Jun 8, 2022

View reviewed changes

TheAndrewJackson suggested changes Jun 8, 2022

View reviewed changes

allisonking reviewed Jun 8, 2022

View reviewed changes

Update error logging to use console.error

673548f

TheAndrewJackson requested a review from LKCSmith June 8, 2022 15:32

elliotbonneville added 3 commits June 8, 2022 11:44

Improve privacy request details page loading sequence

4564194

Remove extraneous comment

e76cd24

Merge branch 'main' into feature/refactor-auth

13176a4

LKCSmith previously requested changes Jun 8, 2022

View reviewed changes

TheAndrewJackson self-requested a review June 8, 2022 19:53

TheAndrewJackson approved these changes Jun 8, 2022

View reviewed changes

allisonking approved these changes Jun 9, 2022

View reviewed changes

TheAndrewJackson requested a review from LKCSmith June 9, 2022 15:18

TheAndrewJackson merged commit b8110c5 into main Jun 9, 2022

TheAndrewJackson deleted the feature/refactor-auth branch June 9, 2022 16:09

allisonking mentioned this pull request Jul 1, 2022

[821] ui/config-wizard: Shared state in redux slice ethyca/fides#837

Merged

9 tasks


		To enable stable authentication you must supply a `NEXTAUTH_SECRET` environment
		variable. The best way to do this is by creating a `.env.local` file, which Next


		export const { login, logout } = authSlice.actions;

		export const credentialStorage = createListenerMiddleware();

		@@ -0,0 +1,11 @@
		// components/Image.js
		import NextImage from 'next/image';

Refactor auth and enable static file serving #577

Refactor auth and enable static file serving #577

Conversation

elliotbonneville commented May 27, 2022 • edited Loading

Purpose

Changes

Checklist

Ticket

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheAndrewJackson left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheAndrewJackson Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheAndrewJackson Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheAndrewJackson Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elliotbonneville commented Jun 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheAndrewJackson left a comment

Choose a reason for hiding this comment

allisonking left a comment

Choose a reason for hiding this comment

elliotbonneville commented May 27, 2022 •

edited

Loading

TheAndrewJackson left a comment •

edited

Loading

TheAndrewJackson Jun 8, 2022 •

edited

Loading

TheAndrewJackson Jun 8, 2022 •

edited

Loading

TheAndrewJackson Jun 8, 2022 •

edited

Loading