Skip to content

Commit

Permalink
Regression: File upload via apps not working in some scenarios (#18995)
Browse files Browse the repository at this point in the history
* [FIX] [Apps] Fix app user doesn't has permission to upload files

* Fix errorClass [Error]: Forbidden [forbidden]

In `app/file-upload/lib/FileUploadBase.js`, we configured `UploadFS.config.defaultStorePermissions`, which validates `insert(userId, doc)` etc. But the parameter userId cann't be always obtained by this validation method correctly (sometimes it's undefined). Meteor use the Meteor.userId() as its fallback option. So we can wrap the original call with `Meteor.runAsUser` to solve issue.

* Add a new validator into canAccessRoom

canAccessRoom (`app/authorization/server/functions/canAccessRoom.js`) is an essential validator for Rocket.Chat to check whether some user has permissions to access some room. In this PR, we added a new validator that allows app users to access any room on a Rocket.Chat server even if it is not a member of the room.

*  An attempt to fix Meteor code must always run within a Fiber Error

Original Error: "Error: Meteor code must always run within a Fiber. Try wrapping callbacks that you pass to non-Meteor libraries with Meteor.bindEnvironment."

* Add support for uploading files by a livecaht visitor

* Support upload files with livechat visitors

* Reduce an unnecessary DB query - Users.findOneById

* Move the "bypass" out of canAccessRoom

Adding a bypass inside canAccessRoom can potentially allow apps to do stuff we're not prepared (yet)

* Update Apps-Engine version

* Some refactoring

* Fix a rateada

Co-authored-by: Douglas Gubert <[email protected]>
  • Loading branch information
Shiqi Mei and d-gubert authored Sep 29, 2020
1 parent 77f2b51 commit 5f6f002
Show file tree
Hide file tree
Showing 5 changed files with 39 additions and 17 deletions.
38 changes: 29 additions & 9 deletions app/apps/server/bridges/uploads.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,40 @@ export class AppUploadBridge {
});
}

createUpload(details, buffer, appId) {
async createUpload(details, buffer, appId) {
this.orch.debugLog(`The App ${ appId } is creating an upload "${ details.name }"`);

details.type = determineFileType(buffer, details);
if (!details.userId && !details.visitorToken) {
throw new Error('Missing user to perform the upload operation');
}

if (details.visitorToken) {
delete details.userId;
}

const fileStore = FileUpload.getStore('Uploads');
const uploadedFile = fileStore.insertSync(details, buffer);
const insertSync = details.userId
? (...args) => Meteor.runAsUser(details.userId, () => fileStore.insertSync(...args))
: Meteor.wrapAsync(fileStore.insert.bind(fileStore));

if (details.userId) {
Meteor.runAsUser(details.userId, () => {
Meteor.call('sendFileMessage', details.rid, null, uploadedFile);
});
}
details.type = determineFileType(buffer, details);

return new Promise(Meteor.bindEnvironment((resolve, reject) => {
try {
const uploadedFile = insertSync(details, buffer);

if (details.visitorToken) {
Meteor.call('sendFileLivechatMessage', details.rid, details.visitorToken, uploadedFile);
} else {
Meteor.runAsUser(details.userId, () => {
Meteor.call('sendFileMessage', details.rid, null, uploadedFile);
});
}

return this.orch.getConverters().get('uploads').convertToApp(uploadedFile);
resolve(this.orch.getConverters().get('uploads').convertToApp(uploadedFile));
} catch (err) {
reject(err);
}
}));
}
}
2 changes: 1 addition & 1 deletion app/file-upload/server/lib/FileUpload.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export const FileUpload = {
const room = Rooms.findOneById(file.rid);
const directMessageAllowed = settings.get('FileUpload_Enabled_Direct');
const fileUploadAllowed = settings.get('FileUpload_Enabled');
if (user.type !== 'app' && canAccessRoom(room, user, file) !== true) {
if (user?.type !== 'app' && canAccessRoom(room, user, file) !== true) {
return false;
}
const language = user ? user.language : 'en';
Expand Down
8 changes: 5 additions & 3 deletions app/file-upload/server/methods/sendFileMessage.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,21 @@ import { Random } from 'meteor/random';
import _ from 'underscore';

import { Uploads } from '../../../models';
import { Rooms } from '../../../models/server/raw';
import { callbacks } from '../../../callbacks';
import { FileUpload } from '../lib/FileUpload';
import { canAccessRoom } from '../../../authorization/server/functions/canAccessRoom';

Meteor.methods({
async sendFileMessage(roomId, store, file, msgData = {}) {
if (!Meteor.userId()) {
throw new Meteor.Error('error-invalid-user', 'Invalid user', { method: 'sendFileMessage' });
}

const room = Meteor.call('canAccessRoom', roomId, Meteor.userId());
const room = await Rooms.findOneById(roomId);
const user = Meteor.user();

if (!room) {
if (user?.type !== 'app' && canAccessRoom(room, user) !== true) {
return false;
}

Expand Down Expand Up @@ -65,7 +68,6 @@ Meteor.methods({
attachment.video_size = file.size;
}

const user = Meteor.user();
let msg = Object.assign({
_id: Random.id(),
rid: roomId,
Expand Down
6 changes: 3 additions & 3 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@
"@nivo/heatmap": "^0.61.0",
"@nivo/line": "^0.61.1",
"@nivo/pie": "^0.61.1",
"@rocket.chat/apps-engine": "1.18.0-beta.3861",
"@rocket.chat/apps-engine": "1.18.0-beta.3873",
"@rocket.chat/css-in-js": "^0.14.1",
"@rocket.chat/fuselage": "^0.14.1",
"@rocket.chat/fuselage-hooks": "^0.14.1",
Expand Down

0 comments on commit 5f6f002

Please sign in to comment.