Don't fill context.session if the session is invalid (#6841)

* s/keystone.com/keystonejs.com * add whitespace to unit test workflows * move initialiseData to utils as seed(...) * add tests * trigger test failure * dont return a session if data is undefined * update examples * throw if no .session exists on the configuration * yarn format * add failing test for invalid sessionData * catch errors from invalid sessionData * yarn format * add changeset * fix types for example Co-authored-by: Daniel Cousens <[email protected]>
keystonejs · Oct 28, 2021 · 36174f8 · 36174f8
1 parent 903080c
commit 36174f8
Show file tree

Hide file tree

Showing 10 changed files with 310 additions and 182 deletions.
diff --git a/.changeset/slimy-rings-type.md b/.changeset/slimy-rings-type.md
@@ -0,0 +1,5 @@
+---
+"@keystone-next/auth": patch
+---
+
+Don't fill `context.session` if the session is invalid.
diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml
@@ -20,14 +20,16 @@ jobs:
           filters: |
             shouldRunTests:
               - '!{{design-system}/**,**/*.md}'
+
       - run: echo "SHOULD_RUN_TESTS=$SHOULD_RUN" >> $GITHUB_ENV
         if: github.event_name == 'pull_request'
         env:
           SHOULD_RUN: ${{ steps.filter.outputs.shouldRunTests }}
+
       - run: echo "SHOULD_RUN_TESTS=true" >> $GITHUB_ENV
         if: github.event_name != 'pull_request'
 
-  graphql-api-tests:
+  graphql_api_tests:
     name: API Tests
     needs: should_run_tests
     runs-on: ubuntu-latest
@@ -75,6 +77,7 @@ jobs:
       - name: Install Dependencies
         run: yarn
         if: needs.should_run_tests.outputs.shouldRunTests == 'true'
+
       - name: Unit tests
         if: needs.should_run_tests.outputs.shouldRunTests == 'true'
         run: yarn jest --ci --runInBand api-tests
@@ -87,7 +90,7 @@ jobs:
           TEST_ADAPTER: ${{ matrix.adapter }}
           DATABASE_URL: ${{ matrix.adapter == 'sqlite' && 'file:./dev.db' || 'postgres://keystone5:k3yst0n3@localhost:5432/test_db' }}
 
-  non-api-tests:
+  unit_tests:
     name: Package Unit Tests
     needs: should_run_tests
     runs-on: ubuntu-latest
@@ -129,14 +132,15 @@ jobs:
 
       - name: Install Dependencies
         run: yarn
+
       - name: Unit tests
         run: yarn jest --ci --runInBand --testPathIgnorePatterns=admin-ui-tests --testPathIgnorePatterns=api-tests --testPathIgnorePatterns=examples-smoke-tests --testPathIgnorePatterns=examples/testing
         env:
           CLOUDINARY_CLOUD_NAME: ${{ secrets.CLOUDINARY_CLOUD_NAME }}
           CLOUDINARY_KEY: ${{ secrets.CLOUDINARY_KEY }}
           CLOUDINARY_SECRET: ${{ secrets.CLOUDINARY_SECRET }}
 
-  example-testing:
+  examples_tests:
     name: Testing example project
     needs: should_run_tests
     runs-on: ubuntu-latest
@@ -167,7 +171,8 @@ jobs:
 
       - name: Install Dependencies
         run: yarn
-      - name: Unit tests
+
+      - name: Example unit tests
         run: cd examples/testing; yarn test
 
   examples_smoke_tests:
@@ -229,12 +234,15 @@ jobs:
       - name: Install Dependencies
         if: needs.should_run_tests.outputs.shouldRunTests == 'true'
         run: yarn
+
       - name: Install Dependencies of Browsers
         if: needs.should_run_tests.outputs.shouldRunTests == 'true'
         run: npx playwright install-deps
+
       - name: Install Browsers
         if: needs.should_run_tests.outputs.shouldRunTests == 'true'
         run: npx playwright install
+
       - name: Unit tests
         if: needs.should_run_tests.outputs.shouldRunTests == 'true'
         run: yarn jest --ci --runInBand tests/examples-smoke-tests/${{ matrix.test }}
@@ -286,12 +294,15 @@ jobs:
       - name: Install Dependencies
         if: needs.should_run_tests.outputs.shouldRunTests == 'true'
         run: yarn
+
       - name: Install Dependencies of Browsers
         if: needs.should_run_tests.outputs.shouldRunTests == 'true'
         uses: microsoft/playwright-github-action@v1
+
       - name: Install Browsers for Playwright
         if: needs.should_run_tests.outputs.shouldRunTests == 'true'
         run: node ./node_modules/playwright/install.js
+
       - name: Unit tests
         if: needs.should_run_tests.outputs.shouldRunTests == 'true'
         run: yarn jest --ci --runInBand tests/admin-ui-tests/${{ matrix.test }}
@@ -324,17 +335,24 @@ jobs:
 
       - name: Install Dependencies
         run: yarn
+
       - name: Prettier
         run: yarn lint:prettier
+
       - name: TypeScript
         run: yarn lint:types
+
       - name: ESLint
         run: yarn lint:eslint
+
       - name: Preconstruct
         run: yarn build
+
       - name: Remark
         run: yarn lint:markdown
+
       - name: Example schemas
         run: yarn lint:examples
+
       - name: Prisma Filters
         run: yarn lint:filters
diff --git a/examples-staging/auth/schema.ts b/examples-staging/auth/schema.ts
@@ -6,12 +6,12 @@ export const lists = {
     access: {
       operation: {
         // Only allow admins to delete users
-        delete: ({ session }) => session?.data?.isAdmin,
+        delete: ({ session }) => session?.data.isAdmin,
       },
     },
     ui: {
       // Since you can't delete users unless you're an admin, we hide the UI for it
-      hideDelete: ({ session }) => !session?.data?.isAdmin,
+      hideDelete: ({ session }) => !session?.data.isAdmin,
       listView: {
         // These are the default columns that will be displayed in the list view
         initialColumns: ['name', 'email', 'isAdmin'],
@@ -34,7 +34,7 @@ export const lists = {
           // Based on the same logic as update access, the password field is editable.
           // The password field is hidden from non-Admin users (except for themselves)
           // createView: {
-          //   fieldMode: ({ session }) => (session?.data?.isAdmin ? 'edit' : 'hidden'),
+          //   fieldMode: ({ session }) => (session?.data.isAdmin ? 'edit' : 'hidden'),
           // },
           itemView: {
             fieldMode: ({ session, item }) =>

diff --git a/examples-staging/basic/schema.ts b/examples-staging/basic/schema.ts
@@ -16,20 +16,20 @@ import { KeystoneListsAPI } from '@keystone-next/keystone/types';
 import { componentBlocks } from './admin/fieldViews/Content';
 import { KeystoneListsTypeInfo } from '.keystone/types';
 
-// TODO: Can we generate this type based on sessionData in the main config?
 type AccessArgs = {
   session?: {
     itemId?: string;
     listKey?: string;
-    data?: {
+    data: {
       name?: string;
       isAdmin: boolean;
     };
   };
   item?: any;
 };
+
 export const access = {
-  isAdmin: ({ session }: AccessArgs) => !!session?.data?.isAdmin,
+  isAdmin: ({ session }: AccessArgs) => !!session?.data.isAdmin,
 };
 
 const randomNumber = () => Math.round(Math.random() * 10);

diff --git a/examples-staging/ecommerce/keystone.ts b/examples-staging/ecommerce/keystone.ts
@@ -71,9 +71,7 @@ export default withAuth(
     extendGraphqlSchema,
     ui: {
       // Show the UI only for poeple who pass this test
-      isAccessAllowed: ({ session }) =>
-        // console.log(session);
-        !!session?.data,
+      isAccessAllowed: ({ session }) => !!session,
     },
     session: statelessSessions(sessionConfig),
   })

diff --git a/examples-staging/graphql-api-endpoint/keystone.ts b/examples-staging/graphql-api-endpoint/keystone.ts
@@ -37,7 +37,7 @@ export default auth.withAuth(
       url: process.env.DATABASE_URL || 'postgres://username:password@localhost/database-name',
     },
     ui: {
-      isAccessAllowed: context => !!context.session?.data,
+      isAccessAllowed: context => !!context.session,
     },
     lists,
     session: statelessSessions({ maxAge: sessionMaxAge, secret: sessionSecret }),

diff --git a/packages/auth/src/index.ts b/packages/auth/src/index.ts
@@ -232,20 +232,17 @@ export function createAuth<GeneratedListTypes extends BaseGeneratedListTypes>({
           return;
         }
 
-        // NOTE: This is wrapped in a try-catch block because a "not found" result will currently
-        // throw; I think this needs to be reviewed, but for now this prevents a system crash when
-        // the session item is invalid
         try {
-          // If no field selection is specified, just load the id. We still load the item,
-          // because doing so validates that it exists in the database
           const data = await sudoContext.query[listKey].findOne({
             where: { id: session.itemId },
             query: sessionData || 'id',
           });
+          if (!data) return;
+
           return { ...session, itemId: session.itemId, listKey, data };
         } catch (e) {
-          // TODO: This swallows all errors, we need a way to differentiate between "not found" and
-          // actual exceptions that should be thrown
+          // TODO: the assumption is this should only be from an invalid sessionData configuration
+          //   it could be something else though, either way, result is a bad session
           return;
         }
       },
@@ -292,10 +289,10 @@ export function createAuth<GeneratedListTypes extends BaseGeneratedListTypes>({
         },
       };
     }
-    let session = keystoneConfig.session;
-    if (session && sessionData) {
-      session = withItemData(session);
-    }
+
+    if (!keystoneConfig.session) throw new TypeError('Missing .session configuration');
+    const session = withItemData(keystoneConfig.session);
+
     const existingExtendGraphQLSchema = keystoneConfig.extendGraphqlSchema;
     const listConfig = keystoneConfig.lists[listKey];
     return {