Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NEW] Add JWT to uploaded files urls #15297

Merged
merged 16 commits into from
Sep 17, 2019
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion app/file-upload/server/lib/FileUpload.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { roomTypes } from '../../../utils/server/lib/roomTypes';
import { hasPermission } from '../../../authorization/server/functions/hasPermission';
import { canAccessRoom } from '../../../authorization/server/functions/canAccessRoom';
import { fileUploadIsValidContentType } from '../../../utils/lib/fileUploadRestrictions';
import { isValidJWT, generateJWT } from '../../../utils/server/lib/JWTHelper';

const cookie = new Cookies();
let maxFileSize = 0;
Expand Down Expand Up @@ -294,6 +295,7 @@ export const FileUpload = {
}

let { rc_uid, rc_token, rc_rid, rc_room_type } = query;
const { token } = query;

if (!rc_uid && headers.cookie) {
rc_uid = cookie.get('rc_uid', headers.cookie);
Expand All @@ -305,7 +307,8 @@ export const FileUpload = {
const isAuthorizedByCookies = rc_uid && rc_token && Users.findOneByIdAndLoginToken(rc_uid, rc_token);
const isAuthorizedByHeaders = headers['x-user-id'] && headers['x-auth-token'] && Users.findOneByIdAndLoginToken(headers['x-user-id'], headers['x-auth-token']);
const isAuthorizedByRoom = rc_room_type && roomTypes.getConfig(rc_room_type).canAccessUploadedFile({ rc_uid, rc_rid, rc_token });
return isAuthorizedByCookies || isAuthorizedByHeaders || isAuthorizedByRoom;
const isAuthorizedByJWT = !settings.get('FileUpload_Enable_json_web_token_for_files') || (token && isValidJWT(token, settings.get('FileUpload_json_web_token_secret_for_files')));
return isAuthorizedByCookies || isAuthorizedByHeaders || isAuthorizedByRoom || isAuthorizedByJWT;
},
addExtensionTo(file) {
if (mime.lookup(file.name) === file.type) {
Expand Down Expand Up @@ -389,6 +392,17 @@ export const FileUpload = {

request.get(fileUrl, (fileRes) => fileRes.pipe(res));
},

addJWTToFileUrl({ rid, userId, fileId }) {
if (!settings.get('FileUpload_ProtectFiles') || !settings.get('FileUpload_Enable_json_web_token_for_files')) {
return;
}
return generateJWT({
rid,
userId,
fileId,
}, settings.get('FileUpload_json_web_token_secret_for_files'));
},
};

export class FileUploadClass {
Expand Down
20 changes: 20 additions & 0 deletions app/file-upload/server/startup/settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,26 @@ settings.addGroup('FileUpload', function() {
i18nDescription: 'FileUpload_ProtectFilesDescription',
});

this.add('FileUpload_Enable_json_web_token_for_files', true, {
type: 'boolean',
i18nLabel: 'FileUpload_Enable_json_web_token_for_files',
i18nDescription: 'FileUpload_Enable_json_web_token_for_files_description',
enableQuery: {
_id: 'FileUpload_ProtectFiles',
value: true,
},
});

this.add('FileUpload_json_web_token_secret_for_files', '', {
type: 'string',
i18nLabel: 'FileUpload_json_web_token_secret_for_files',
i18nDescription: 'FileUpload_json_web_token_secret_for_files_description',
enableQuery: {
_id: 'FileUpload_Enable_json_web_token_for_files',
value: true,
},
});

this.add('FileUpload_Storage_Type', 'GridFS', {
type: 'select',
values: [{
Expand Down
19 changes: 11 additions & 8 deletions app/livechat/server/api/v1/message.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { API } from '../../../../api';
import { loadMessageHistory } from '../../../../lib';
import { findGuest, findRoom, normalizeHttpHeaderData } from '../lib/livechat';
import { Livechat } from '../../lib/Livechat';
import { normalizeMessageAttachments } from '../../../../utils/server/functions/normalizeMessageAttachments';

API.v1.addRoute('livechat/message', {
post() {
Expand Down Expand Up @@ -95,9 +96,9 @@ API.v1.addRoute('livechat/message/:_id', {
throw new Meteor.Error('invalid-message');
}

return API.v1.success({ message });
return API.v1.success({ message: normalizeMessageAttachments(message) });

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind of checking the message.file before returning? I think we should always return the message instead of the normalizeMessageAttachments.

} catch (e) {
return API.v1.failure(e.error);
return API.v1.failure(e);
}
},

Expand Down Expand Up @@ -134,12 +135,12 @@ API.v1.addRoute('livechat/message/:_id', {
const result = Livechat.updateMessage({ guest, message: { _id: msg._id, msg: this.bodyParams.msg } });
if (result) {
const message = Messages.findOneById(_id);
return API.v1.success({ message });
return API.v1.success({ message: normalizeMessageAttachments(message) });

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind of checking the message.file before returning? I think we should always return the message instead of the normalizeMessageAttachments.

}

return API.v1.failure();
} catch (e) {
return API.v1.failure(e.error);
return API.v1.failure(e);
}
},
delete() {
Expand Down Expand Up @@ -183,7 +184,7 @@ API.v1.addRoute('livechat/message/:_id', {

return API.v1.failure();
} catch (e) {
return API.v1.failure(e.error);
return API.v1.failure(e);
}
},
});
Expand Down Expand Up @@ -227,10 +228,12 @@ API.v1.addRoute('livechat/messages.history/:rid', {
limit = parseInt(this.queryParams.limit);
}

const messages = loadMessageHistory({ userId: guest._id, rid, end, limit, ls });
return API.v1.success(messages);
const messages = loadMessageHistory({ userId: guest._id, rid, end, limit, ls })

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new implementation is returning an array of messages, but the original implementation needs to return an object containing a property called by messages, which will contain the array of messages.

.messages
.map(normalizeMessageAttachments);
return API.v1.success({ messages });
} catch (e) {
return API.v1.failure(e.error);
return API.v1.failure(e);
}
},
});
Expand Down
3 changes: 3 additions & 0 deletions app/livechat/server/hooks/externalMessage.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { settings } from '../../../settings';
import { callbacks } from '../../../callbacks';
import { SystemLogger } from '../../../logger';
import { LivechatExternalMessage } from '../../lib/LivechatExternalMessage';
import { normalizeMessageAttachments } from '../../../utils/server/functions/normalizeMessageAttachments';

let knowledgeEnabled = false;
let apiaiKey = '';
Expand Down Expand Up @@ -33,6 +34,8 @@ callbacks.add('afterSaveMessage', function(message, room) {
return message;
}

message = normalizeMessageAttachments(message);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind of checking the message.file before returning? I think we should always return the message instead of the normalizeMessageAttachments.


// if the message hasn't a token, it was not sent by the visitor, so ignore it
if (!message.token) {
return message;
Expand Down
3 changes: 3 additions & 0 deletions app/livechat/server/hooks/saveAnalyticsData.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { callbacks } from '../../../callbacks';
import { LivechatRooms } from '../../../models';
import { normalizeMessageAttachments } from '../../../utils/server/functions/normalizeMessageAttachments';

callbacks.add('afterSaveMessage', function(message, room) {
// skips this callback if the message was edited
Expand All @@ -12,6 +13,8 @@ callbacks.add('afterSaveMessage', function(message, room) {
return message;
}

message = normalizeMessageAttachments(message);


const now = new Date();
let analyticsData;
Expand Down
8 changes: 7 additions & 1 deletion app/livechat/server/hooks/sendToCRM.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { settings } from '../../../settings';
import { callbacks } from '../../../callbacks';
import { Messages, LivechatRooms } from '../../../models';
import { Livechat } from '../lib/Livechat';
import { normalizeMessageAttachments } from '../../../utils/server/functions/normalizeMessageAttachments';

const msgNavType = 'livechat_navigation_history';

Expand Down Expand Up @@ -55,7 +56,12 @@ function sendToCRM(type, room, includeMessages = true) {
msg.navigation = message.navigation;
}

postData.messages.push(msg);
if (message.file) {
msg.file = message.file;
msg.attachments = message.attachments;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not call the normalizer function inside this block of code?

}

postData.messages.push(normalizeMessageAttachments(msg));
});
}

Expand Down
3 changes: 2 additions & 1 deletion app/livechat/server/hooks/sendToFacebook.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { callbacks } from '../../../callbacks';
import { settings } from '../../../settings';
import OmniChannel from '../lib/OmniChannel';
import { normalizeMessageAttachments } from '../../../utils/server/functions/normalizeMessageAttachments';

callbacks.add('afterSaveMessage', function(message, room) {
// skips this callback if the message was edited
Expand Down Expand Up @@ -33,5 +34,5 @@ callbacks.add('afterSaveMessage', function(message, room) {
text: message.msg,
});

return message;
return normalizeMessageAttachments(message);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind of checking the message.file before returning? I think we should always return the message instead of the normalizeMessageAttachments.

}, callbacks.priority.LOW, 'sendMessageToFacebook');
3 changes: 2 additions & 1 deletion app/livechat/server/methods/sendMessageLivechat.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { LivechatVisitors } from '../../../models';
import { Livechat } from '../lib/Livechat';

Meteor.methods({
sendMessageLivechat({ token, _id, rid, msg, attachments }, agent) {
sendMessageLivechat({ token, _id, rid, msg, file, attachments }, agent) {
check(token, String);
check(_id, String);
check(rid, String);
Expand Down Expand Up @@ -36,6 +36,7 @@ Meteor.methods({
rid,
msg,
token,
file,
attachments,
},
agent,
Expand Down
3 changes: 2 additions & 1 deletion app/livechat/server/sendMessageBySMS.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { callbacks } from '../../callbacks';
import { settings } from '../../settings';
import { SMS } from '../../sms';
import { LivechatVisitors } from '../../models';
import { normalizeMessageAttachments } from '../../utils/server/functions/normalizeMessageAttachments';

callbacks.add('afterSaveMessage', function(message, room) {
// skips this callback if the message was edited
Expand Down Expand Up @@ -42,5 +43,5 @@ callbacks.add('afterSaveMessage', function(message, room) {

SMSService.send(room.sms.from, visitor.phone[0].phoneNumber, message.msg);

return message;
return normalizeMessageAttachments(message);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind of checking the message.file before returning? I think we should always return the message instead of the normalizeMessageAttachments.

}, callbacks.priority.LOW, 'sendMessageBySms');
16 changes: 16 additions & 0 deletions app/utils/server/functions/normalizeMessageAttachments.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { FileUpload } from '../../../file-upload/server';

export const normalizeMessageAttachments = (message) => {
if (message.file && message.attachments && Array.isArray(message.attachments) && message.attachments.length) {
const jwt = FileUpload.addJWTToFileUrl({ rid: message.rid, userId: message.u._id, fileId: message.file._id });

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the FileUpload.addJWTToFileUrl may return null, you need to check the returned value before moving forward, don't you think?

Thanks.

message.attachments.forEach((attachment) => {
if (attachment.title_link) {
attachment.title_link = `${ attachment.title_link }?token=${ jwt }`;
}
if (attachment.image_url) {
attachment.image_url = `${ attachment.image_url }?token=${ jwt }`;
}
});
}
return message;
};
28 changes: 28 additions & 0 deletions app/utils/server/lib/JWTHelper.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { jws } from 'jsrsasign';

const HEADER = {
typ: 'JWT',
alg: 'HS256',
};

export const generateJWT = (payload, secret) => {
const tokenPayload = {
iat: jws.IntDate.get('now'),
nbf: jws.IntDate.get('now'),
exp: jws.IntDate.get('now + 1hour'),
aud: 'RocketChat',
context: payload,
};

const header = JSON.stringify(HEADER);

return jws.JWS.sign(HEADER.alg, header, JSON.stringify(tokenPayload), { rstr: secret });
};

export const isValidJWT = (jwt, secret) => {
try {
return jws.JWS.verify(jwt, secret, HEADER);
} catch (error) {
return false;
}
};
4 changes: 4 additions & 0 deletions packages/rocketchat-i18n/i18n/en.i18n.json
Original file line number Diff line number Diff line change
Expand Up @@ -1391,6 +1391,10 @@
"FileUpload_Enabled": "File Uploads Enabled",
"FileUpload_Error": "File Upload Error",
"FileUpload_Enabled_Direct": "File Uploads Enabled in Direct Messages ",
"FileUpload_Enable_json_web_token_for_files": "Enable Json Web Tokens protection to file uploads",
"FileUpload_Enable_json_web_token_for_files_description": "Appends a JWT to uploaded files urls",
"FileUpload_json_web_token_secret_for_files": "File Upload Json Web Token Secret",
"FileUpload_json_web_token_secret_for_files_description": "File Upload Json Web Token Secret (Used to be able to access uploaded files without authentication)",
"FileUpload_File_Empty": "File empty",
"FileUpload_FileSystemPath": "System Path",
"FileUpload_GoogleStorage_AccessId": "Google Storage Access Id",
Expand Down
1 change: 1 addition & 0 deletions server/startup/migrations/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,4 +153,5 @@ import './v152';
import './v153';
import './v154';
import './v155';
import './v156';
import './xrun';
44 changes: 44 additions & 0 deletions server/startup/migrations/v156.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { Random } from 'meteor/random';

import { Migrations } from '../../../app/migrations/server';
import { Settings } from '../../../app/models/server';
import { settings } from '../../../app/settings/server';

Migrations.add({
version: 156,
up() {
Settings.upsert({
_id: 'FileUpload_Enable_json_web_token_for_files',
},
{
_id: 'FileUpload_Enable_json_web_token_for_files',
value: settings.get('FileUpload_ProtectFiles'),
type: 'boolean',
group: 'FileUpload',
i18nLabel: 'FileUpload_Enable_json_web_token_for_files',
i18nDescription: 'FileUpload_Enable_json_web_token_for_files_description',
enableQuery: {
_id: 'FileUpload_ProtectFiles',
value: true,
},
});
Settings.upsert({
_id: 'FileUpload_json_web_token_secret_for_files',
},
{
_id: 'FileUpload_json_web_token_secret_for_files',
value: Random.secret(),
type: 'string',
group: 'FileUpload',
i18nLabel: 'FileUpload_json_web_token_secret_for_files',
i18nDescription: 'FileUpload_json_web_token_secret_for_files_description',
enableQuery: {
_id: 'FileUpload_Enable_json_web_token_for_files',
value: true,
},
});
},
down() {
// Down migration does not apply in this case
},
});