diff --git a/settings.json.template b/settings.json.template index 61d6db117..e4b2b3d3c 100644 --- a/settings.json.template +++ b/settings.json.template @@ -497,19 +497,6 @@ */ "importMaxFileSize": 52428800, // 50 * 1024 * 1024 - - /* - * From Etherpad 1.8.3 onwards import was restricted to authors who had - * content within the pad. - * - * This setting will override that restriction and allow any user to import - * without the requirement to add content to a pad. - * - * This setting is useful for when you use a plugin for authentication so you - * can already trust each user. - */ - "allowAnyoneToImport": false, - /* * From Etherpad 1.9.0 onwards, when Etherpad is in production mode commits from individual users are rate limited * diff --git a/src/locales/en.json b/src/locales/en.json index ec861b772..a65e62885 100644 --- a/src/locales/en.json +++ b/src/locales/en.json @@ -188,6 +188,5 @@ "pad.impexp.importfailed": "Import failed", "pad.impexp.copypaste": "Please copy paste", "pad.impexp.exportdisabled": "Exporting as {{type}} format is disabled. Please contact your system administrator for details.", - "pad.impexp.maxFileSize": "File too big. Contact your site administrator to increase the allowed file size for import", - "pad.impexp.permission": "Import is disabled because you never contributed to this pad. Please contribute at least once before importing" + "pad.impexp.maxFileSize": "File too big. Contact your site administrator to increase the allowed file size for import" } diff --git a/src/node/handler/PadMessageHandler.js b/src/node/handler/PadMessageHandler.js index 9e0e349ce..0794a7b1d 100644 --- a/src/node/handler/PadMessageHandler.js +++ b/src/node/handler/PadMessageHandler.js @@ -942,16 +942,6 @@ async function handleClientReady(client, message, authorID) }); })); - let thisUserHasEditedThisPad = false; - if (historicalAuthorData[authorID]) { - /* - * This flag is set to true when a user contributes to a specific pad for - * the first time. It is used for deciding if importing to that pad is - * allowed or not. - */ - thisUserHasEditedThisPad = true; - } - // glue the clientVars together, send them and tell the other clients that a new one is there // Check that the client is still here. It might have disconnected between callbacks. @@ -1135,8 +1125,6 @@ async function handleClientReady(client, message, authorID) "percentageToScrollWhenUserPressesArrowUp": settings.scrollWhenFocusLineIsOutOfViewport.percentageToScrollWhenUserPressesArrowUp, }, "initialChangesets": [], // FIXME: REMOVE THIS SHIT - "thisUserHasEditedThisPad": thisUserHasEditedThisPad, - "allowAnyoneToImport": settings.allowAnyoneToImport } // Add a username to the clientVars if one avaiable diff --git a/src/node/hooks/express/importexport.js b/src/node/hooks/express/importexport.js index efb89b3c9..d5e6c3bbd 100644 --- a/src/node/hooks/express/importexport.js +++ b/src/node/hooks/express/importexport.js @@ -8,6 +8,7 @@ var readOnlyManager = require("../../db/ReadOnlyManager"); var authorManager = require("../../db/AuthorManager"); const rateLimit = require("express-rate-limit"); const securityManager = require("../../db/SecurityManager"); +const webaccess = require("./webaccess"); settings.importExportRateLimiting.onLimitReached = function(req, res, options) { // when the rate limiter triggers, write a warning in the logs @@ -63,36 +64,11 @@ exports.expressCreateServer = function (hook_name, args, cb) { args.app.use('/p/:pad/import', limiter); args.app.post('/p/:pad/import', async function(req, res, next) { const {session: {user} = {}} = req; - const {accessStatus, authorID} = await securityManager.checkAccess( + const {accessStatus} = await securityManager.checkAccess( req.params.pad, req.cookies.sessionID, req.cookies.token, req.cookies.password, user); - if (accessStatus !== 'grant') return res.status(403).send('Forbidden'); - assert(authorID); - - /* - * Starting from Etherpad 1.8.3 onwards, importing into a pad is allowed - * only if a user has his browser opened and connected to the pad (i.e. a - * Socket.IO session is estabilished for him) and he has already - * contributed to that specific pad. - * - * Note that this does not have anything to do with the "session", used - * for logging into "group pads". That kind of session is not needed here. - * - * This behaviour does not apply to API requests, only to /p/$PAD$/import - * - * See: https://github.com/ether/etherpad-lite/pull/3833#discussion_r407490205 - */ - if (!settings.allowAnyoneToImport) { - const authorsPads = await authorManager.listPadsOfAuthor(authorID); - if (!authorsPads) { - console.warn(`Unable to import file into "${req.params.pad}". Author "${authorID}" exists but he never contributed to any pad`); - return next(); - } - if (authorsPads.padIDs.indexOf(req.params.pad) === -1) { - console.warn(`Unable to import file into "${req.params.pad}". Author "${authorID}" exists but he never contributed to this pad`); - return next(); - } + if (accessStatus !== 'grant' || !webaccess.userCanModify(req.params.pad, req)) { + return res.status(403).send('Forbidden'); } - - importHandler.doImport(req, res, req.params.pad); + await importHandler.doImport(req, res, req.params.pad); }); } diff --git a/src/node/utils/Settings.js b/src/node/utils/Settings.js index 6f3ccea89..baf4d539e 100644 --- a/src/node/utils/Settings.js +++ b/src/node/utils/Settings.js @@ -385,20 +385,6 @@ exports.commitRateLimiting = { */ exports.importMaxFileSize = 50 * 1024 * 1024; - -/* - * From Etherpad 1.8.3 onwards import was restricted to authors who had - * content within the pad. - * - * This setting will override that restriction and allow any user to import - * without the requirement to add content to a pad. - * - * This setting is useful for when you use a plugin for authentication so you - * can already trust each user. - */ -exports.allowAnyoneToImport = false, - - // checks if abiword is avaiable exports.abiwordAvailable = function() { diff --git a/src/static/css/pad.css b/src/static/css/pad.css index 2043c3fff..9455e657c 100644 --- a/src/static/css/pad.css +++ b/src/static/css/pad.css @@ -71,7 +71,3 @@ input { @media (max-width: 800px) { .hide-for-mobile { display: none; } } - -#importmessagepermission { - display: none; -} diff --git a/src/static/js/collab_client.js b/src/static/js/collab_client.js index a5620f7b5..b04e6aa2c 100644 --- a/src/static/js/collab_client.js +++ b/src/static/js/collab_client.js @@ -312,16 +312,6 @@ function getCollabClient(ace2editor, serverVars, initialUserInfo, options, _pad) } else if (msg.type == "ACCEPT_COMMIT") { - /* - * this is the first time this user contributed to this pad. Let's record - * it, because it will be used for allowing import. - * - * TODO: here, we are changing this variable on the client side only. The - * server has all the informations to make the same deduction, and - * broadcast to the client. - */ - clientVars.thisUserHasEditedThisPad = true; - var newRev = msg.newRev; if (msgQueue.length > 0) { diff --git a/src/static/js/pad_editbar.js b/src/static/js/pad_editbar.js index 0e40bb990..0035b0023 100644 --- a/src/static/js/pad_editbar.js +++ b/src/static/js/pad_editbar.js @@ -415,17 +415,6 @@ var padeditbar = (function() toolbar.registerCommand("import_export", function () { toolbar.toggleDropDown("import_export", function(){ - - if (clientVars.thisUserHasEditedThisPad || clientVars.allowAnyoneToImport) { - // the user has edited this pad historically or in this session - $('#importform').show(); - $('#importmessagepermission').hide(); - } else { - // this is the first time this user visits this pad - $('#importform').hide(); - $('#importmessagepermission').show(); - } - // If Import file input exists then focus on it.. if($('#importfileinput').length !== 0){ setTimeout(function(){ diff --git a/src/templates/pad.html b/src/templates/pad.html index 49aa20204..0ba811095 100644 --- a/src/templates/pad.html +++ b/src/templates/pad.html @@ -195,7 +195,6 @@ -
<% e.end_block(); %>
diff --git a/tests/backend/specs/api/importexportGetPost.js b/tests/backend/specs/api/importexportGetPost.js index 27f4758a0..5c716ea0c 100644 --- a/tests/backend/specs/api/importexportGetPost.js +++ b/tests/backend/specs/api/importexportGetPost.js @@ -7,7 +7,10 @@ const common = require('../../common'); const superagent = require(__dirname+'/../../../../src/node_modules/superagent'); const fs = require('fs'); const settings = require(__dirname+'/../../../../src/node/utils/Settings'); +const padManager = require(__dirname+'/../../../../src/node/db/PadManager'); const path = require('path'); +const plugins = require(__dirname+'/../../../../src/static/js/pluginfw/plugin_defs'); + const padText = fs.readFileSync("../tests/backend/specs/api/test.txt"); const etherpadDoc = fs.readFileSync("../tests/backend/specs/api/test.etherpad"); const wordDoc = fs.readFileSync("../tests/backend/specs/api/test.doc"); @@ -20,7 +23,8 @@ let agent; var apiKey = fs.readFileSync(filePath, {encoding: 'utf-8'}); apiKey = apiKey.replace(/\n$/, ""); var apiVersion = 1; -var testPadId = makeid(); +const testPadId = makeid(); +const testPadIdEnc = encodeURIComponent(testPadId); before(async function() { agent = await common.init(); }); @@ -67,14 +71,6 @@ Example Curl command for testing import URI: */ describe('Imports and Exports', function(){ - before(function() { - if (!settings.allowAnyoneToImport) { - console.warn('not anyone can import so not testing -- ' + - 'to include this test set allowAnyoneToImport to true in settings.json'); - this.skip(); - } - }); - const backups = {}; beforeEach(async function() { @@ -219,6 +215,137 @@ describe('Imports and Exports', function(){ .expect((res) => assert.doesNotMatch(res.text, /FrameCall\('undefined', 'ok'\);/)); }); + describe('Import authorization checks', function() { + let authorize; + + const deleteTestPad = async () => { + if (await padManager.doesPadExist(testPadId)) { + const pad = await padManager.getPad(testPadId); + await pad.remove(); + } + }; + + const createTestPad = async (text) => { + const pad = await padManager.getPad(testPadId); + if (text) await pad.setText(text); + return pad; + }; + + beforeEach(async function() { + await deleteTestPad(); + settings.requireAuthentication = false; + settings.requireAuthorization = true; + settings.users = {user: {password: 'user-password'}}; + authorize = () => true; + backups.hooks = {}; + backups.hooks.authorize = plugins.hooks.authorize || []; + plugins.hooks.authorize = [{hook_fn: (hookName, {req}, cb) => cb([authorize(req)])}]; + }); + + afterEach(async function() { + await deleteTestPad(); + Object.assign(plugins.hooks, backups.hooks); + }); + + it('!authn !exist -> create', async function() { + await agent.post(`/p/${testPadIdEnc}/import`) + .attach('file', padText, {filename: '/test.txt', contentType: 'text/plain'}) + .expect(200); + assert(await padManager.doesPadExist(testPadId)); + const pad = await padManager.getPad(testPadId); + assert.equal(pad.text(), padText.toString()); + }); + + it('!authn exist -> replace', async function() { + const pad = await createTestPad('before import'); + await agent.post(`/p/${testPadIdEnc}/import`) + .attach('file', padText, {filename: '/test.txt', contentType: 'text/plain'}) + .expect(200); + assert(await padManager.doesPadExist(testPadId)); + assert.equal(pad.text(), padText.toString()); + }); + + it('authn anonymous !exist -> fail', async function() { + settings.requireAuthentication = true; + await agent.post(`/p/${testPadIdEnc}/import`) + .attach('file', padText, {filename: '/test.txt', contentType: 'text/plain'}) + .expect(401); + assert(!(await padManager.doesPadExist(testPadId))); + }); + + it('authn anonymous exist -> fail', async function() { + settings.requireAuthentication = true; + const pad = await createTestPad('before import\n'); + await agent.post(`/p/${testPadIdEnc}/import`) + .attach('file', padText, {filename: '/test.txt', contentType: 'text/plain'}) + .expect(401); + assert.equal(pad.text(), 'before import\n'); + }); + + it('authn user create !exist -> create', async function() { + settings.requireAuthentication = true; + await agent.post(`/p/${testPadIdEnc}/import`) + .auth('user', 'user-password') + .attach('file', padText, {filename: '/test.txt', contentType: 'text/plain'}) + .expect(200); + assert(await padManager.doesPadExist(testPadId)); + const pad = await padManager.getPad(testPadId); + assert.equal(pad.text(), padText.toString()); + }); + + it('authn user modify !exist -> fail', async function() { + settings.requireAuthentication = true; + authorize = () => 'modify'; + await agent.post(`/p/${testPadIdEnc}/import`) + .auth('user', 'user-password') + .attach('file', padText, {filename: '/test.txt', contentType: 'text/plain'}) + .expect(403); + assert(!(await padManager.doesPadExist(testPadId))); + }); + + it('authn user readonly !exist -> fail', async function() { + settings.requireAuthentication = true; + authorize = () => 'readOnly'; + await agent.post(`/p/${testPadIdEnc}/import`) + .auth('user', 'user-password') + .attach('file', padText, {filename: '/test.txt', contentType: 'text/plain'}) + .expect(403); + assert(!(await padManager.doesPadExist(testPadId))); + }); + + it('authn user create exist -> replace', async function() { + settings.requireAuthentication = true; + const pad = await createTestPad('before import\n'); + await agent.post(`/p/${testPadIdEnc}/import`) + .auth('user', 'user-password') + .attach('file', padText, {filename: '/test.txt', contentType: 'text/plain'}) + .expect(200); + assert.equal(pad.text(), padText.toString()); + }); + + it('authn user modify exist -> replace', async function() { + settings.requireAuthentication = true; + authorize = () => 'modify'; + const pad = await createTestPad('before import\n'); + await agent.post(`/p/${testPadIdEnc}/import`) + .auth('user', 'user-password') + .attach('file', padText, {filename: '/test.txt', contentType: 'text/plain'}) + .expect(200); + assert.equal(pad.text(), padText.toString()); + }); + + it('authn user readonly exist -> fail', async function() { + const pad = await createTestPad('before import\n'); + settings.requireAuthentication = true; + authorize = () => 'readOnly'; + await agent.post(`/p/${testPadIdEnc}/import`) + .auth('user', 'user-password') + .attach('file', padText, {filename: '/test.txt', contentType: 'text/plain'}) + .expect(403); + assert.equal(pad.text(), 'before import\n'); + }); + }); + }); // End of tests. diff --git a/tests/frontend/travis/runnerBackend.sh b/tests/frontend/travis/runnerBackend.sh index c595dce02..ff92667d3 100755 --- a/tests/frontend/travis/runnerBackend.sh +++ b/tests/frontend/travis/runnerBackend.sh @@ -12,11 +12,8 @@ cd "${MY_DIR}/../../../" # Set soffice to /usr/bin/soffice sed 's#\"soffice\": null,#\"soffice\":\"/usr/bin/soffice\",#g' settings.json.template > settings.json.soffice -# Set allowAnyoneToImport to true -sed 's/\"allowAnyoneToImport\": false,/\"allowAnyoneToImport\": true,/g' settings.json.soffice > settings.json.allowImport - # Set "max": 10 to 100 to not agressively rate limit -sed 's/\"max\": 10/\"max\": 100/g' settings.json.allowImport > settings.json.rateLimit +sed 's/\"max\": 10/\"max\": 100/g' settings.json.soffice > settings.json.rateLimit # Set "points": 10 to 1000 to not agressively rate limit commits sed 's/\"points\": 10/\"points\": 1000/g' settings.json.rateLimit > settings.json