From d26eb990e42aab3ec4a970420d4c6b6fc57f10a2 Mon Sep 17 00:00:00 2001 From: Wolfgang Rathgeb Date: Thu, 5 Mar 2020 14:12:34 +0100 Subject: [PATCH 1/6] added hash --- config/default.json | 6 +++--- src/middleware/socket.js | 3 ++- src/services/lessons/models/index.js | 1 + src/services/lessons/schemes/lessons.create.schema.json | 5 +++++ src/services/lessons/schemes/lessons.patch.schema.json | 5 +++++ src/services/sections/schemas/section.create.schema.json | 3 +++ src/services/sections/schemas/section.patch.schema.json | 5 ++++- 7 files changed, 23 insertions(+), 5 deletions(-) diff --git a/config/default.json b/config/default.json index 4d82d55..c4bd76d 100644 --- a/config/default.json +++ b/config/default.json @@ -1,11 +1,11 @@ { - "host": "localhost", + "host": "api.edtr.l", "port": 4001, "protocol": "http", - "mongodb": "mongodb://localhost:27017/schulcloud-editor", + "mongodb": "mongodb://mongodb:27017/schulcloud-editor", "routes": { "server": { - "baseURL": "http://localhost:3030", + "baseURL": "http://server:3030", "coursePermissionsUri": "/courses/:courseId/userPermissions", "meUri": "/me", "courseMembersUri": "/courses/:courseId/members" diff --git a/src/middleware/socket.js b/src/middleware/socket.js index 433312a..7ea9fc0 100644 --- a/src/middleware/socket.js +++ b/src/middleware/socket.js @@ -5,7 +5,7 @@ module.exports = app => app.configure( socketio((io) => { io.on('connection', (socket) => { // do some authorization things - app.channel('anonymous').join(socket.feathers); + // app.channel('anonymous').join(socket.feathers); socket.on('authorization', (data) => { const { token } = data; @@ -14,6 +14,7 @@ module.exports = app => app.configure( }); }); + // socketJwtHandler(io); io.use((socket, next) => { diff --git a/src/services/lessons/models/index.js b/src/services/lessons/models/index.js index 20f99a2..fc95f99 100644 --- a/src/services/lessons/models/index.js +++ b/src/services/lessons/models/index.js @@ -13,6 +13,7 @@ const lessonSchema = new Schema({ deletedAt: { type: Date, expires: (60 * 60 * 24 * 30) }, createdBy: { type: Schema.Types.ObjectId }, updatedBy: { type: Schema.Types.ObjectId }, + hash: { type: String }, position: { type: Number, default: Date.now }, fork: { type: Schema.Types.ObjectId }, // is forked from }, { diff --git a/src/services/lessons/schemes/lessons.create.schema.json b/src/services/lessons/schemes/lessons.create.schema.json index f496bb0..be62315 100644 --- a/src/services/lessons/schemes/lessons.create.schema.json +++ b/src/services/lessons/schemes/lessons.create.schema.json @@ -33,6 +33,11 @@ "$id": "#/properties/fork", "type": "string", "pattern": "[a-f0-9]{24}" + }, + "hash": { + "$id": "#/properties/hash", + "type": "string", + "pattern": "[a-f0-9]{24}" } }, "additionalProperties": false diff --git a/src/services/lessons/schemes/lessons.patch.schema.json b/src/services/lessons/schemes/lessons.patch.schema.json index 80cc4c8..668a2c9 100644 --- a/src/services/lessons/schemes/lessons.patch.schema.json +++ b/src/services/lessons/schemes/lessons.patch.schema.json @@ -22,6 +22,11 @@ "position": { "$id": "#/properties/position", "type": "number" + }, + "hash": { + "$id": "#/properties/hash", + "type": "string", + "pattern": "[a-f0-9]{24}" } }, "additionalProperties": false diff --git a/src/services/sections/schemas/section.create.schema.json b/src/services/sections/schemas/section.create.schema.json index a302d2d..d00a73c 100644 --- a/src/services/sections/schemas/section.create.schema.json +++ b/src/services/sections/schemas/section.create.schema.json @@ -12,6 +12,9 @@ "state": { "type": "object" }, + "hash": { + "type": "string" + }, "createdBy": { "type": "string", "pattern": "[a-f0-9]{24}" diff --git a/src/services/sections/schemas/section.patch.schema.json b/src/services/sections/schemas/section.patch.schema.json index a9c3f1d..5761d77 100644 --- a/src/services/sections/schemas/section.patch.schema.json +++ b/src/services/sections/schemas/section.patch.schema.json @@ -14,7 +14,10 @@ }, "stateDiff": { "type": "object" - }, + }, + "hash": { + "type": "string" + }, "updatedBy": { "type": "string", "pattern": "[a-f0-9]{24}" From 9db30477e993724db8a0dcf4ab085517a565a0d0 Mon Sep 17 00:00:00 2001 From: Thomas Feldtkeller Date: Fri, 6 Mar 2020 15:28:10 +0100 Subject: [PATCH 2/6] deal with hashes in section GET --- src/services/sections/models/index.js | 1 + src/services/sections/section.service.js | 26 ++++++++---- src/services/sections/section.service.test.js | 42 +++++++++++++++++++ 3 files changed, 60 insertions(+), 9 deletions(-) diff --git a/src/services/sections/models/index.js b/src/services/sections/models/index.js index f3d3be0..317537f 100644 --- a/src/services/sections/models/index.js +++ b/src/services/sections/models/index.js @@ -37,6 +37,7 @@ const sectionSchema = new Schema({ title: { type: String, default: '' }, state: { type: Object, default: {} }, permissions: [{ type: permissionSchema }], + hash: { type: String }, deletedAt: { type: Date, expires: (60 * 60 * 24 * 30) }, createdBy: { type: Schema.Types.ObjectId, immutable: true }, updatedBy: { type: Schema.Types.ObjectId }, diff --git a/src/services/sections/section.service.js b/src/services/sections/section.service.js index eade709..3f6cecd 100644 --- a/src/services/sections/section.service.js +++ b/src/services/sections/section.service.js @@ -93,16 +93,24 @@ class SectionService { return setUserScopePermissionForFindRequests(sections, params.user); } - get(id, params) { + async get(id, params) { const internParams = this.setScope(prepareParams(params)); - return this.app.service(this.baseService) - .get(id, internParams) - .then((section) => { - if (!permissions.hasRead(section.permissions, params.user)) { - throw new Forbidden(this.err.noAccess); - } - return setUserScopePermission(section, section.permissions, params.user); - }); + let section = await this.app.service(this.baseService).get(id, internParams); + if (!permissions.hasRead(section.permissions, params.user)) { + throw new Forbidden(this.err.noAccess); + } + section.timestamp = section.updatedAt; + const clientAlreadyHasNewestState = params.query.hash && params.query.hash === section.hash; + if (clientAlreadyHasNewestState) { + section = { + _id: section._id, + permissions: section.permissions, + title: section.title, + hash: section.hash, + }; + } + const result = await setUserScopePermission(section, section.permissions, params.user); + return result; } async remove(_id, params) { diff --git a/src/services/sections/section.service.test.js b/src/services/sections/section.service.test.js index fdb2db1..f42fb8d 100644 --- a/src/services/sections/section.service.test.js +++ b/src/services/sections/section.service.test.js @@ -184,6 +184,48 @@ describe('sections/section.service.js', () => { expect(status).to.equal(403); }); + it('get with hash only sends state if hashes dont match', async () => { + const { _id: lessonId } = await lessonService.createWithDefaultData({ courseId }, readPermission); + const state = { abc: 123, x: [] }; + const ref = { + target: lessonId, + targetModel: 'lesson', + }; + const hash = 'thisisthecurrenthash'; + const section = await sectionService.createWithDefaultData({ state, ref, hash }, readPermission); + + expect(lessonId.toString()).to.equal(section.ref.target.toString()); + + const { + status: matchingHashStatus, data: matchingHashData, + } = await sectionService.sendRequestToThisService('get', { + id: section._id, userId, lessonId, query: { hash: 'thisisthecurrenthash' }, + }); + + expect(matchingHashStatus).to.equal(200); + expect(matchingHashData).to.an('object'); + expect(matchingHashData).to.have.property('hash'); + expect(matchingHashData).to.not.have.property('ref'); + expect(matchingHashData).to.not.have.property('state'); + expect(matchingHashData).to.not.have.property('permissions'); + expect(matchingHashData).to.have.property(userScopePermissionKey); + + const { + status: otherHashStatus, data: otherHashData, + } = await sectionService.sendRequestToThisService('get', { + id: section._id, userId, lessonId, query: { hash: 'someotherhash' }, + }); + + expect(otherHashStatus).to.equal(200); + expect(otherHashData).to.an('object'); + expect(otherHashData.ref.target.toString()).to.equal(lessonId.toString()); + expect(otherHashData.state).to.deep.equal(state); + expect(otherHashData).to.not.have.property('permissions'); + expect(otherHashData).to.have.property(userScopePermissionKey); + expect(otherHashData).to.have.property('timestamp'); + expect(otherHashData).to.have.property('hash'); + }); + it('find with read permissions should work', async () => { const { _id: lessonId } = await lessonService.createWithDefaultData({ courseId }, writePermission); const ref = { From ceedf908cfc43d7a0b9ed813bf1245f4fc29ed46 Mon Sep 17 00:00:00 2001 From: Thomas Feldtkeller Date: Mon, 9 Mar 2020 10:30:13 +0100 Subject: [PATCH 3/6] multiple classes in testhelper.js --- src/testHelpers/testHelper.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/testHelpers/testHelper.js b/src/testHelpers/testHelper.js index 4558ab5..4a17635 100644 --- a/src/testHelpers/testHelper.js +++ b/src/testHelpers/testHelper.js @@ -1,4 +1,4 @@ -/* eslint-disable arrow-body-style */ +/* eslint-disable arrow-body-style, max-classes-per-file */ const mongoose = require('mongoose'); const axios = require('axios'); const queryString = require('query-string'); From aae7ab8f461a04c6d189b854dba3cbd7e6674d8d Mon Sep 17 00:00:00 2001 From: Thomas Feldtkeller Date: Mon, 9 Mar 2020 15:29:05 +0100 Subject: [PATCH 4/6] should not send states when hashes match --- src/routes/ServiceRoutes.js | 2 ++ src/services/sections/section.service.js | 27 ++++++++++++++++--- src/services/sections/section.service.test.js | 27 +++++++++++++++++++ 3 files changed, 53 insertions(+), 3 deletions(-) diff --git a/src/routes/ServiceRoutes.js b/src/routes/ServiceRoutes.js index 9c90a0a..cefb03b 100644 --- a/src/routes/ServiceRoutes.js +++ b/src/routes/ServiceRoutes.js @@ -1,3 +1,5 @@ +/* eslint-disable max-classes-per-file */ + const axios = require('axios'); const queryString = require('query-string'); diff --git a/src/services/sections/section.service.js b/src/services/sections/section.service.js index 3f6cecd..75a1ac3 100644 --- a/src/services/sections/section.service.js +++ b/src/services/sections/section.service.js @@ -82,9 +82,30 @@ class SectionService { const internParams = this.setScope(prepareParams(params)); const sections = await this.app.service(this.baseService) .find(internParams); - const filtered = permissions.filterHasRead(sections.data, params.user); - - filtered.map((section) => { + let filtered = permissions.filterHasRead(sections.data, params.user); + if (params.query.hashes) { + let hashes; + try { + hashes = JSON.parse(params.query.hashes); + } catch (err) { + throw new BadRequest('hashes should be valid stringified JSON'); + } + filtered = filtered.map((section) => { + const clientHasNewestState = hashes[section._id.toString()] + && hashes[section._id.toString()] === section.hash; + if (clientHasNewestState) { + // eslint-disable-next-line no-param-reassign + section = { + _id: section._id, + permissions: section.permissions, + title: section.title, + hash: section.hash, + }; + } + return section; + }); + } + filtered.forEach((section) => { section.visible = !permissions.couldAnyoneOnlyRead(section.permissions); return section; }); diff --git a/src/services/sections/section.service.test.js b/src/services/sections/section.service.test.js index f42fb8d..089a13a 100644 --- a/src/services/sections/section.service.test.js +++ b/src/services/sections/section.service.test.js @@ -248,6 +248,33 @@ describe('sections/section.service.js', () => { expect(data.data).all.have.property(userScopePermissionKey); }); + it('find should filter states if hashes match', async () => { + const { _id: lessonId } = await lessonService.createWithDefaultData({ courseId }, writePermission); + const ref = { + target: lessonId, + targetModel: 'lesson', + }; + const sections = await Promise.all([ + sectionService.createWithDefaultData({ ref, hash: 'firsthash' }, writePermission), + sectionService.createWithDefaultData({ ref, hash: 'secondhash' }, readPermission), + sectionService.createWithDefaultData({ ref, hash: undefined }, readPermission), + ]); + + const query = { hashes: {} }; + query.hashes[sections[0]._id] = 'firsthash'; + query.hashes[sections[1]._id] = 'wronghash'; + query.hashes = JSON.stringify(query.hashes); + const result = await sectionService.sendRequestToThisService('find', { + userId, lessonId, query, + }); + expect(result.status).to.equal(200); + expect(result.data.data).to.have.lengthOf(3); + expect(result.data.data).all.have.property(userScopePermissionKey); + expect(result.data.data[0]).to.not.have.property('state'); + expect(result.data.data[1]).to.have.property('state'); + expect(result.data.data[2]).to.have.property('state'); + }); + it('patch with write permissions should work', async () => { const { _id: lessonId } = await lessonService.createWithDefaultData({ courseId }, writePermission); From 752a0098c89c5ec6e8f0db2877f5e00e2e054e2d Mon Sep 17 00:00:00 2001 From: Thomas Feldtkeller Date: Tue, 10 Mar 2020 11:34:30 +0100 Subject: [PATCH 5/6] extract filterByHash methods --- src/services/sections/section.service.js | 39 +++---------------- src/services/sections/utils/index.js | 5 ++- src/services/sections/utils/versionHashes.js | 40 ++++++++++++++++++++ 3 files changed, 50 insertions(+), 34 deletions(-) create mode 100644 src/services/sections/utils/versionHashes.js diff --git a/src/services/sections/section.service.js b/src/services/sections/section.service.js index 75a1ac3..46c55e1 100644 --- a/src/services/sections/section.service.js +++ b/src/services/sections/section.service.js @@ -17,6 +17,8 @@ const { } = require('../../utils'); const { diffToMongo, + filterManyByHashes, + filterStateByHash, } = require('./utils'); const { create: createSchema, @@ -83,28 +85,7 @@ class SectionService { const sections = await this.app.service(this.baseService) .find(internParams); let filtered = permissions.filterHasRead(sections.data, params.user); - if (params.query.hashes) { - let hashes; - try { - hashes = JSON.parse(params.query.hashes); - } catch (err) { - throw new BadRequest('hashes should be valid stringified JSON'); - } - filtered = filtered.map((section) => { - const clientHasNewestState = hashes[section._id.toString()] - && hashes[section._id.toString()] === section.hash; - if (clientHasNewestState) { - // eslint-disable-next-line no-param-reassign - section = { - _id: section._id, - permissions: section.permissions, - title: section.title, - hash: section.hash, - }; - } - return section; - }); - } + filtered = filterManyByHashes(filtered, params.query.hashes); filtered.forEach((section) => { section.visible = !permissions.couldAnyoneOnlyRead(section.permissions); return section; @@ -116,21 +97,13 @@ class SectionService { async get(id, params) { const internParams = this.setScope(prepareParams(params)); - let section = await this.app.service(this.baseService).get(id, internParams); + const section = await this.app.service(this.baseService).get(id, internParams); if (!permissions.hasRead(section.permissions, params.user)) { throw new Forbidden(this.err.noAccess); } section.timestamp = section.updatedAt; - const clientAlreadyHasNewestState = params.query.hash && params.query.hash === section.hash; - if (clientAlreadyHasNewestState) { - section = { - _id: section._id, - permissions: section.permissions, - title: section.title, - hash: section.hash, - }; - } - const result = await setUserScopePermission(section, section.permissions, params.user); + const filteredSection = filterStateByHash(section, params.query.hash); + const result = await setUserScopePermission(filteredSection, filteredSection.permissions, params.user); return result; } diff --git a/src/services/sections/utils/index.js b/src/services/sections/utils/index.js index 01efda4..202e102 100644 --- a/src/services/sections/utils/index.js +++ b/src/services/sections/utils/index.js @@ -1 +1,4 @@ -exports.diffToMongo = require('./diffToMongo'); +const { filterManyByHashes, filterStateByHash } = require('./versionHashes'); +const diffToMongo = require('./diffToMongo'); + +module.exports = { diffToMongo, filterManyByHashes, filterStateByHash }; diff --git a/src/services/sections/utils/versionHashes.js b/src/services/sections/utils/versionHashes.js new file mode 100644 index 0000000..0890a63 --- /dev/null +++ b/src/services/sections/utils/versionHashes.js @@ -0,0 +1,40 @@ +const { BadRequest } = require('@feathersjs/errors'); + +/** + * if the hash matches the hash of the section, this function filters any data that is not required by the client + * (since he already has the most recent version of the section) + * @param {Object} section a section + * @param {String} hash string, representing a hash of a version of the section + */ +const filterStateByHash = (section, hash) => { + const clientAlreadyHasNewestState = hash && hash === section.hash; + if (clientAlreadyHasNewestState) { + return { + _id: section._id, + permissions: section.permissions, + title: section.title, + hash: section.hash, + }; + } + return section; +}; + +/** + * applies filterStateByHash on an array of sections, using a map of hashes + * @param {Array} sections an array of sections. + * @param {stringified JSON} hashes Object with sectionIds as keys, and hashes as values. + */ +const filterManyByHashes = (sections, hashes) => { + if (hashes) { + try { + // eslint-disable-next-line no-param-reassign + hashes = JSON.parse(hashes); + } catch (err) { + throw new BadRequest('hashes should be valid stringified JSON'); + } + return sections.map((section) => filterStateByHash(section, hashes[section._id.toString()])); + } + return sections; +}; + +module.exports = { filterManyByHashes, filterStateByHash }; From 34513e60a96979c5a99c9f3f4858efadd6525370 Mon Sep 17 00:00:00 2001 From: Wolfgang Rathgeb Date: Tue, 24 Mar 2020 09:57:56 +0100 Subject: [PATCH 6/6] publish date --- src/global/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/global/index.js b/src/global/index.js index 05a1b98..c067753 100644 --- a/src/global/index.js +++ b/src/global/index.js @@ -77,7 +77,7 @@ exports.before = { const after = { // todo select is better but need more stable implementations all: [iff(isProvider('external'), - filterOutResults('__v', 'createdAt', 'updatedAt')), + filterOutResults('__v', 'createdBy', 'updatedBy')), ], find: [], get: [],