From b2520258a3c7454fdab8facdb261f4913e60b9d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Sch=C3=A4fer?= Date: Mon, 4 Mar 2019 18:35:02 +0100 Subject: [PATCH 1/6] Improve specification of posts resolver * only authors are alllowed to update/delete their own posts * set disabled+deleted to false if not provided --- src/resolvers/posts.spec.js | 195 +++++++++++++++++++++++++++++++----- 1 file changed, 168 insertions(+), 27 deletions(-) diff --git a/src/resolvers/posts.spec.js b/src/resolvers/posts.spec.js index a6c1d7e3e..5603683eb 100644 --- a/src/resolvers/posts.spec.js +++ b/src/resolvers/posts.spec.js @@ -3,6 +3,7 @@ import { GraphQLClient } from 'graphql-request' import { host, login } from '../jest/helpers' const factory = Factory() +let client beforeEach(async () => { await factory.create('User', { @@ -16,46 +17,186 @@ afterEach(async () => { }) describe('CreatePost', () => { + const mutation = ` + mutation { + CreatePost(title: "I am a title", content: "Some content") { + title + content + slug + disabled + deleted + } + } + ` + describe('unauthenticated', () => { - let client it('throws authorization error', async () => { client = new GraphQLClient(host) - await expect(client.request(`mutation { - CreatePost( - title: "I am a post", - content: "Some content" - ) { slug } - }`)).rejects.toThrow('Not Authorised') + await expect(client.request(mutation)).rejects.toThrow('Not Authorised') + }) + }) + + describe('authenticated', () => { + let headers + beforeEach(async () => { + headers = await login({ email: 'test@example.org', password: '1234' }) + client = new GraphQLClient(host, { headers }) }) - describe('authenticated', () => { - let headers - let response - beforeEach(async () => { - headers = await login({ email: 'test@example.org', password: '1234' }) - client = new GraphQLClient(host, { headers }) - response = await client.request(`mutation { - CreatePost( - title: "A title", - content: "Some content" - ) { title, content } - }`, { headers }) - }) + it('creates a post', async () => { + const expected = { + CreatePost: { + title: 'I am a title', + content: 'Some content' + } + } + await expect(client.request(mutation)).resolves.toMatchObject(expected) + }) - it('creates a post', () => { - expect(response).toEqual({ CreatePost: { title: 'A title', content: 'Some content' } }) - }) - - it('assigns the authenticated user as author', async () => { - const { User } = await client.request(`{ + it('assigns the authenticated user as author', async () => { + await client.request(mutation) + const { User } = await client.request(`{ User(email:"test@example.org") { contributions { title } } }`, { headers }) - expect(User).toEqual([ { contributions: [ { title: 'A title' } ] } ]) + expect(User).toEqual([ { contributions: [ { title: 'I am a title' } ] } ]) + }) + + describe('disabled and deleted', () => { + it('initially false', async () => { + const expected = { CreatePost: { disabled: false, deleted: false } } + await expect(client.request(mutation)).resolves.toMatchObject(expected) }) }) }) }) + +describe('UpdatePost', () => { + const mutation = ` + mutation($id: ID!, $content: String) { + UpdatePost(id: $id, content: $content) { + id + content + } + } + ` + + let variables = { + id: 'p1', + content: 'New content' + } + + beforeEach(async () => { + const asAuthor = Factory() + await asAuthor.create('User', { + email: 'author@example.org', + password: '1234' + }) + await asAuthor.authenticateAs({ + email: 'author@example.org', + password: '1234' + }) + await asAuthor.create('Post', { + id: 'p1', + content: 'Old content' + }) + }) + + describe('unauthenticated', () => { + it('throws authorization error', async () => { + client = new GraphQLClient(host) + await expect(client.request(mutation, variables)).rejects.toThrow('Not Authorised') + }) + }) + + describe('authenticated but not the author', () => { + let headers + beforeEach(async () => { + headers = await login({ email: 'test@example.org', password: '1234' }) + client = new GraphQLClient(host, { headers }) + }) + + it('throws authorization error', async () => { + await expect(client.request(mutation, variables)).rejects.toThrow('Not Authorised') + }) + }) + + describe('authenticated as author', () => { + let headers + beforeEach(async () => { + headers = await login({ email: 'author@example.org', password: '1234' }) + client = new GraphQLClient(host, { headers }) + }) + + it('updates a post', async () => { + const expected = { UpdatePost: { id: 'p1', content: 'New content' } } + await expect(client.request(mutation, variables)).resolves.toEqual(expected) + }) + }) +}) + +describe('DeletePost', () => { + const mutation = ` + mutation($id: ID!) { + DeletePost(id: $id) { + id + content + } + } + ` + + let variables = { + id: 'p1' + } + + beforeEach(async () => { + const asAuthor = Factory() + await asAuthor.create('User', { + email: 'author@example.org', + password: '1234' + }) + await asAuthor.authenticateAs({ + email: 'author@example.org', + password: '1234' + }) + await asAuthor.create('Post', { + id: 'p1', + content: 'To be deleted' + }) + }) + + describe('unauthenticated', () => { + it('throws authorization error', async () => { + client = new GraphQLClient(host) + await expect(client.request(mutation, variables)).rejects.toThrow('Not Authorised') + }) + }) + + describe('authenticated but not the author', () => { + let headers + beforeEach(async () => { + headers = await login({ email: 'test@example.org', password: '1234' }) + client = new GraphQLClient(host, { headers }) + }) + + it('throws authorization error', async () => { + await expect(client.request(mutation, variables)).rejects.toThrow('Not Authorised') + }) + }) + + describe('authenticated as author', () => { + let headers + beforeEach(async () => { + headers = await login({ email: 'author@example.org', password: '1234' }) + client = new GraphQLClient(host, { headers }) + }) + + it('deletes a post', async () => { + const expected = { DeletePost: { id: 'p1', content: 'To be deleted' } } + await expect(client.request(mutation, variables)).resolves.toEqual(expected) + }) + }) +}) From c869724d29784ed9af282c4a590ddd7b231dacc8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Sch=C3=A4fer?= Date: Mon, 4 Mar 2019 18:36:56 +0100 Subject: [PATCH 2/6] Let all tests pass :green_heart: --- src/middleware/permissionsMiddleware.js | 2 -- src/middleware/softDeleteMiddleware.js | 3 +++ src/resolvers/posts.js | 33 +++++++++++++++++++++---- 3 files changed, 31 insertions(+), 7 deletions(-) diff --git a/src/middleware/permissionsMiddleware.js b/src/middleware/permissionsMiddleware.js index 5515d5b7a..b755f3bab 100644 --- a/src/middleware/permissionsMiddleware.js +++ b/src/middleware/permissionsMiddleware.js @@ -34,8 +34,6 @@ const permissions = shield({ }, Mutation: { CreatePost: isAuthenticated, - // TODO UpdatePost: isOwner, - // TODO DeletePost: isOwner, report: isAuthenticated, CreateBadge: isAdmin, UpdateBadge: isAdmin, diff --git a/src/middleware/softDeleteMiddleware.js b/src/middleware/softDeleteMiddleware.js index bed7b6ca0..0c12e7a72 100644 --- a/src/middleware/softDeleteMiddleware.js +++ b/src/middleware/softDeleteMiddleware.js @@ -19,5 +19,8 @@ export default { User: async (resolve, root, args, context, info) => { return resolve(root, setDefaults(args), context, info) } + }, + Mutation: async (resolve, root, args, context, info) => { + return resolve(root, setDefaults(args), context, info) } } diff --git a/src/resolvers/posts.js b/src/resolvers/posts.js index 6a8a0c25f..f59050b5f 100644 --- a/src/resolvers/posts.js +++ b/src/resolvers/posts.js @@ -1,22 +1,45 @@ import { neo4jgraphql } from 'neo4j-graphql-js' +const isAuthor = async (params, { user, driver }) => { + if (!user) return false + const session = driver.session() + const { id: postId } = params + const result = await session.run(` + MATCH (post:Post {id: $postId})<-[:WROTE]-(author) + RETURN author + `, { postId }) + const [author] = result.records.map((record) => { + return record.get('author') + }) + const { properties: { id: authorId } } = author + session.close() + return authorId === user.id +} + export default { Mutation: { - CreatePost: async (object, params, ctx, resolveInfo) => { - const result = await neo4jgraphql(object, params, ctx, resolveInfo, false) + CreatePost: async (object, params, context, resolveInfo) => { + const result = await neo4jgraphql(object, params, context, resolveInfo, false) - const session = ctx.driver.session() + const session = context.driver.session() await session.run( 'MATCH (author:User {id: $userId}), (post:Post {id: $postId}) ' + 'MERGE (post)<-[:WROTE]-(author) ' + 'RETURN author', { - userId: ctx.user.id, + userId: context.user.id, postId: result.id }) session.close() return result + }, + UpdatePost: async (object, params, context, resolveInfo) => { + if (!await isAuthor(params, context)) return Error('Not Authorised!') + return neo4jgraphql(object, params, context, resolveInfo, false) + }, + DeletePost: async (object, params, context, resolveInfo) => { + if (!await isAuthor(params, context)) return Error('Not Authorised!') + return neo4jgraphql(object, params, context, resolveInfo, false) } - } } From b64ea750115670aeead8441ba25f8b770eae0611 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Sch=C3=A4fer?= Date: Mon, 4 Mar 2019 18:38:36 +0100 Subject: [PATCH 3/6] Add a deleted post and a disabled post to seeds --- src/seed/seed-db.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/seed/seed-db.js b/src/seed/seed-db.js index b2ee8fbdb..ed46a5716 100644 --- a/src/seed/seed-db.js +++ b/src/seed/seed-db.js @@ -82,12 +82,12 @@ import Factory from './factories' await Promise.all([ asAdmin.create('Post', { id: 'p0' }), asModerator.create('Post', { id: 'p1' }), - asUser.create('Post', { id: 'p2' }), + asUser.create('Post', { id: 'p2', deleted: true }), asTick.create('Post', { id: 'p3' }), asTrick.create('Post', { id: 'p4' }), asTrack.create('Post', { id: 'p5' }), asAdmin.create('Post', { id: 'p6' }), - asModerator.create('Post', { id: 'p7' }), + asModerator.create('Post', { id: 'p7', disabled: true }), asUser.create('Post', { id: 'p8' }), asTick.create('Post', { id: 'p9' }), asTrick.create('Post', { id: 'p10' }), From 180491c08caef423c48ee413faed7b6bb9fbd3e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Sch=C3=A4fer?= Date: Mon, 4 Mar 2019 19:40:49 +0100 Subject: [PATCH 4/6] Put `isAuthor` in permissions middleware I find it dirty to access the database in a middleware, ie. I would like to put all access on the database as close to the resolver as possible. However, in this case that would mean to put the authorization check in the resolver, where nobody expects it to be. CC @appinteractive --- src/middleware/permissionsMiddleware.js | 18 ++++++++++++++++++ src/resolvers/posts.js | 24 ------------------------ 2 files changed, 18 insertions(+), 24 deletions(-) diff --git a/src/middleware/permissionsMiddleware.js b/src/middleware/permissionsMiddleware.js index b755f3bab..c40803e00 100644 --- a/src/middleware/permissionsMiddleware.js +++ b/src/middleware/permissionsMiddleware.js @@ -25,6 +25,22 @@ const onlyEnabledContent = rule({ cache: 'strict' })(async (parent, args, ctx, i return !(disabled || deleted) }) +const isAuthor = rule({ cache: 'no_cache' })(async (parent, args, { user, driver }) => { + if (!user) return false + const session = driver.session() + const { id: postId } = args + const result = await session.run(` + MATCH (post:Post {id: $postId})<-[:WROTE]-(author) + RETURN author + `, { postId }) + const [author] = result.records.map((record) => { + return record.get('author') + }) + const { properties: { id: authorId } } = author + session.close() + return authorId === user.id +}) + // Permissions const permissions = shield({ Query: { @@ -34,6 +50,8 @@ const permissions = shield({ }, Mutation: { CreatePost: isAuthenticated, + UpdatePost: isAuthor, + DeletePost: isAuthor, report: isAuthenticated, CreateBadge: isAdmin, UpdateBadge: isAdmin, diff --git a/src/resolvers/posts.js b/src/resolvers/posts.js index f59050b5f..abf91e047 100644 --- a/src/resolvers/posts.js +++ b/src/resolvers/posts.js @@ -1,21 +1,5 @@ import { neo4jgraphql } from 'neo4j-graphql-js' -const isAuthor = async (params, { user, driver }) => { - if (!user) return false - const session = driver.session() - const { id: postId } = params - const result = await session.run(` - MATCH (post:Post {id: $postId})<-[:WROTE]-(author) - RETURN author - `, { postId }) - const [author] = result.records.map((record) => { - return record.get('author') - }) - const { properties: { id: authorId } } = author - session.close() - return authorId === user.id -} - export default { Mutation: { CreatePost: async (object, params, context, resolveInfo) => { @@ -32,14 +16,6 @@ export default { session.close() return result - }, - UpdatePost: async (object, params, context, resolveInfo) => { - if (!await isAuthor(params, context)) return Error('Not Authorised!') - return neo4jgraphql(object, params, context, resolveInfo, false) - }, - DeletePost: async (object, params, context, resolveInfo) => { - if (!await isAuthor(params, context)) return Error('Not Authorised!') - return neo4jgraphql(object, params, context, resolveInfo, false) } } } From 0db91530fea70010d1f90983f743f0c71d20e940 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" Date: Tue, 5 Mar 2019 04:18:26 +0000 Subject: [PATCH 5/6] Bump eslint from 5.15.0 to 5.15.1 Bumps [eslint](https://github.com/eslint/eslint) from 5.15.0 to 5.15.1. - [Release notes](https://github.com/eslint/eslint/releases) - [Changelog](https://github.com/eslint/eslint/blob/master/CHANGELOG.md) - [Commits](https://github.com/eslint/eslint/compare/v5.15.0...v5.15.1) Signed-off-by: dependabot[bot] --- package.json | 2 +- yarn.lock | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/package.json b/package.json index cefa46953..1c6e8f2ce 100644 --- a/package.json +++ b/package.json @@ -75,7 +75,7 @@ "babel-eslint": "~10.0.1", "babel-jest": "~24.1.0", "chai": "~4.2.0", - "eslint": "~5.15.0", + "eslint": "~5.15.1", "eslint-config-standard": "~12.0.0", "eslint-plugin-import": "~2.16.0", "eslint-plugin-jest": "~22.3.0", diff --git a/yarn.lock b/yarn.lock index eb0f3efd9..18d336c7a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2582,10 +2582,10 @@ eslint-visitor-keys@^1.0.0: resolved "https://registry.yarnpkg.com/eslint-visitor-keys/-/eslint-visitor-keys-1.0.0.tgz#3f3180fb2e291017716acb4c9d6d5b5c34a6a81d" integrity sha512-qzm/XxIbxm/FHyH341ZrbnMUpe+5Bocte9xkmFMzPMjRaZMcXww+MpBptFvtU+79L362nqiLhekCxCxDPaUMBQ== -eslint@~5.15.0: - version "5.15.0" - resolved "https://registry.yarnpkg.com/eslint/-/eslint-5.15.0.tgz#f313a2f7c7628d39adeefdba4a9c41f842012c9e" - integrity sha512-xwG7SS5JLeqkiR3iOmVgtF8Y6xPdtr6AAsN6ph7Q6R/fv+3UlKYoika8SmNzmb35qdRF+RfTY35kMEdtbi+9wg== +eslint@~5.15.1: + version "5.15.1" + resolved "https://registry.yarnpkg.com/eslint/-/eslint-5.15.1.tgz#8266b089fd5391e0009a047050795b1d73664524" + integrity sha512-NTcm6vQ+PTgN3UBsALw5BMhgO6i5EpIjQF/Xb5tIh3sk9QhrFafujUOczGz4J24JBlzWclSB9Vmx8d+9Z6bFCg== dependencies: "@babel/code-frame" "^7.0.0" ajv "^6.9.1" From 2813de4f8b79968b6c8d032295c4edb125df66ca Mon Sep 17 00:00:00 2001 From: Grzegorz Leoniec Date: Wed, 6 Mar 2019 13:20:33 +0100 Subject: [PATCH 6/6] Fixed organization seeder --- src/seed/factories/organizations.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/seed/factories/organizations.js b/src/seed/factories/organizations.js index 783edac26..e0b2e52d4 100644 --- a/src/seed/factories/organizations.js +++ b/src/seed/factories/organizations.js @@ -4,7 +4,7 @@ import uuid from 'uuid/v4' export default function create (params) { const { id = uuid(), - name = faker.comany.companyName(), + name = faker.company.companyName(), description = faker.company.catchPhrase(), disabled = false, deleted = false