[NEW] Add JWT to uploaded files urls #15297

MarcosSpessatto · 2019-09-03T19:41:42Z

No description provided.

renatobecker-zz

My suggestion is to create a method/property to get the file path from the FileUpload store, instead of testing the file system outside the FileUpload instance.

renatobecker-zz

@MarcosSpessatto, I left some comments on your PR. They're just improvements on the codebase, but in my opinion, it would uncouple the process of adding the JWT token to the URL.

renatobecker-zz · 2019-09-06T19:33:43Z

app/file-upload/server/lib/FileUpload.js

@@ -294,6 +295,7 @@ export const FileUpload = {
 		}

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


What about renaming the query parameter to token?

renatobecker-zz · 2019-09-06T19:34:38Z

app/file-upload/server/lib/FileUpload.js

@@ -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, generateJsonWebToken } from '../../../utils/server/lib/JWTHelper';


What do you think of renaming to generateJWT, just like the same pattern on the isValidJWT ?

renatobecker-zz · 2019-09-06T19:46:44Z

app/file-upload/server/lib/FileUpload.js

@@ -389,6 +392,29 @@ export const FileUpload = {

 		request.get(fileUrl, (fileRes) => fileRes.pipe(res));
 	},
+
+	addJWTToFileUrlIfNecessary(message) {


IMHO, we should check the file field outside this method. I mean, to me it looks weird returning a message from a method that deals with JWT stuff.

So, then the method will return the JWT itself, not a message object.

renatobecker-zz · 2019-09-06T19:51:07Z

app/file-upload/server/lib/FileUpload.js

+			fileId: message.file._id,
+		}, settings.get('FileUpload_json_web_token_secret_for_files'));
+
+		if (message.attachments && Array.isArray(message.attachments) && message.attachments.length) {


...and this code looks to be a good helper method which would return the message object.

renatobecker-zz · 2019-09-06T19:52:46Z

app/livechat/server/api/v1/message.js

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

-			return API.v1.success({ message });
+			return API.v1.success({ message: FileUpload.addJWTToFileUrlIfNecessary(message) });


As I commented before, IMO it would be better having a specific helper method to handle this process, and inside that method, I would call the addJWTToFileUrlIfNecessary method.
In that case, we could rename the addJWTToFileUrlIfNecessary to addJWTToFileUrl.

renatobecker-zz · 2019-09-06T19:53:36Z

app/livechat/server/hooks/externalMessage.js

@@ -33,6 +34,8 @@ callbacks.add('afterSaveMessage', function(message, room) {
 		return message;
 	}

+	message = FileUpload.addJWTToFileUrlIfNecessary(message);


As I commented before, IMO it would be better having a specific helper method to handle this process, and inside that method, I would call the addJWTToFileUrlIfNecessary method.
In that case, we could rename the addJWTToFileUrlIfNecessary to addJWTToFileUrl.

renatobecker-zz · 2019-09-06T19:53:44Z

app/livechat/server/hooks/saveAnalyticsData.js

@@ -12,6 +13,8 @@ callbacks.add('afterSaveMessage', function(message, room) {
 		return message;
 	}

+	message = FileUpload.addJWTToFileUrlIfNecessary(message);


As I commented before, IMO it would be better having a specific helper method to handle this process, and inside that method, I would call the addJWTToFileUrlIfNecessary method.
In that case, we could rename the addJWTToFileUrlIfNecessary to addJWTToFileUrl.

renatobecker-zz · 2019-09-06T19:53:51Z

app/livechat/server/hooks/sendToCRM.js

@@ -54,8 +55,7 @@ function sendToCRM(type, room, includeMessages = true) {
 			if (message.t === msgNavType) {
 				msg.navigation = message.navigation;
 			}
-
-			postData.messages.push(msg);
+			postData.messages.push(message.file ? FileUpload.addJWTToFileUrlIfNecessary(message) : msg);


As I commented before, IMO it would be better having a specific helper method to handle this process, and inside that method, I would call the addJWTToFileUrlIfNecessary method.
In that case, we could rename the addJWTToFileUrlIfNecessary to addJWTToFileUrl.

renatobecker-zz · 2019-09-06T19:54:06Z

app/livechat/server/hooks/sendToFacebook.js

@@ -16,6 +17,7 @@ callbacks.add('afterSaveMessage', function(message, room) {
 	if (!(typeof room.t !== 'undefined' && room.t === 'l' && room.facebook && room.v && room.v.token)) {
 		return message;
 	}
+	FileUpload.addJWTToFileUrlIfNecessary(message);


As I commented before, IMO it would be better having a specific helper method to handle this process, and inside that method, I would call the addJWTToFileUrlIfNecessary method.
In that case, we could rename the addJWTToFileUrlIfNecessary to addJWTToFileUrl.

renatobecker-zz · 2019-09-06T19:54:14Z

app/livechat/server/sendMessageBySMS.js

@@ -18,6 +19,8 @@ callbacks.add('afterSaveMessage', function(message, room) {
 		return message;
 	}

+	FileUpload.addJWTToFileUrlIfNecessary(message);


As I commented before, IMO it would be better having a specific helper method to handle this process, and inside that method, I would call the addJWTToFileUrlIfNecessary method.
In that case, we could rename the addJWTToFileUrlIfNecessary to addJWTToFileUrl.

renatobecker-zz · 2019-09-11T17:19:37Z

app/livechat/server/api/v1/message.js

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

-			return API.v1.success({ message });
+			return API.v1.success({ message: normalizeMessageAttachments(message) });
 		} catch (e) {
 			return API.v1.failure(e.error);


Suggested change

return API.v1.failure(e.error);

return API.v1.failure(e);

renatobecker-zz · 2019-09-11T17:19:56Z

app/livechat/server/api/v1/message.js

-			const messages = loadMessageHistory({ userId: guest._id, rid, end, limit, ls });
+			const messages = loadMessageHistory({ userId: guest._id, rid, end, limit, ls })
+				.messages
+				.map(normalizeMessageAttachments);
 			return API.v1.success(messages);
 		} catch (e) {
 			return API.v1.failure(e.error);


Suggested change

return API.v1.failure(e.error);

return API.v1.failure(e);

renatobecker-zz · 2019-09-11T18:05:43Z

app/livechat/server/api/v1/message.js

@@ -227,7 +228,9 @@ API.v1.addRoute('livechat/messages.history/:rid', {
 				limit = parseInt(this.queryParams.limit);
 			}

-			const messages = loadMessageHistory({ userId: guest._id, rid, end, limit, ls });
+			const messages = loadMessageHistory({ userId: guest._id, rid, end, limit, ls })


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.

renatobecker-zz · 2019-09-11T20:21:31Z

app/utils/server/functions/normalizeMessageAttachments.js

+
+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 });


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

Thanks.

renatobecker-zz · 2019-09-11T20:43:42Z

app/livechat/server/api/v1/message.js

@@ -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) });


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

renatobecker-zz · 2019-09-11T20:44:02Z

app/livechat/server/api/v1/message.js

@@ -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) });


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

renatobecker-zz · 2019-09-11T20:45:00Z

app/livechat/server/hooks/externalMessage.js

@@ -33,6 +34,8 @@ callbacks.add('afterSaveMessage', function(message, room) {
 		return message;
 	}

+	message = normalizeMessageAttachments(message);


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

renatobecker-zz · 2019-09-11T20:46:00Z

app/livechat/server/hooks/sendToCRM.js

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


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

renatobecker-zz · 2019-09-11T20:46:07Z

app/livechat/server/hooks/sendToFacebook.js

@@ -33,5 +34,5 @@ callbacks.add('afterSaveMessage', function(message, room) {
 		text: message.msg,
 	});

-	return message;
+	return normalizeMessageAttachments(message);


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

renatobecker-zz · 2019-09-11T20:46:17Z

app/livechat/server/sendMessageBySMS.js

@@ -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);


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

Add JWT to uploaded files urls

50f7ad3

MarcosSpessatto added the feat: livechat label Sep 3, 2019

MarcosSpessatto requested a review from renatobecker-zz September 3, 2019 19:41

MarcosSpessatto added this to the 2.1.0 milestone Sep 3, 2019

MarcosSpessatto changed the title ~~[NEW] Add JWT to uploaded files urls~~ [WIP][NEW] Add JWT to uploaded files urls Sep 3, 2019

MarcosSpessatto added 2 commits September 3, 2019 20:53

Add JWT to files inside messages on REST endpoints

749bcbb

Improve code

bfa9413

MarcosSpessatto changed the title ~~[WIP][NEW] Add JWT to uploaded files urls~~ [NEW] Add JWT to uploaded files urls Sep 4, 2019

RocketChat deleted a comment from lgtm-com bot Sep 4, 2019

Move File Helper to utils folder

6f33df3

renatobecker-zz reviewed Sep 4, 2019

View reviewed changes

Move JWT to File Upload class and improve code

5608041

renatobecker-zz reviewed Sep 6, 2019

View reviewed changes

renatobecker and others added 4 commits September 6, 2019 16:57

Merge branch 'develop' into add-jwt-to-file-upload-urls

57c4466

Code improve

e68da32

Rename function

1ac20e3

Merge branch 'develop' into add-jwt-to-file-upload-urls

9f50a05

renatobecker-zz added this to In progress in Omnichannel Roadmap via automation Sep 10, 2019

Merge branch 'develop' into add-jwt-to-file-upload-urls

cfdf556

Omnichannel Roadmap automation moved this from In progress to Needs review Sep 11, 2019

renatobecker-zz suggested changes Sep 11, 2019

View reviewed changes

Improve code

96a6c67

renatobecker-zz suggested changes Sep 11, 2019

View reviewed changes

Fix undefined tokens and rename function

84938a8

renatobecker-zz suggested changes Sep 11, 2019

View reviewed changes

MarcosSpessatto and others added 3 commits September 11, 2019 18:11

Apply suggestions

2c26139

Final version.

26a2825

Merge branch 'develop' into add-jwt-to-file-upload-urls

c31d52a

renatobecker-zz approved these changes Sep 12, 2019

View reviewed changes

Omnichannel Roadmap automation moved this from Needs review to Reviewer approved Sep 12, 2019

Merge branch 'develop' into add-jwt-to-file-upload-urls

a0466bf

renatobecker-zz merged commit bb61341 into develop Sep 17, 2019

Omnichannel Roadmap automation moved this from Reviewer approved to Done Sep 17, 2019

renatobecker-zz deleted the add-jwt-to-file-upload-urls branch September 17, 2019 13:33

sampaiodiego mentioned this pull request Sep 27, 2019

Release 2.1.0 #15459

Merged

antkaz mentioned this pull request Aug 13, 2020

[FIX] Information missing from livechat/messages.history/:rid endpoint #18545

Closed

12 tasks

tassoevan added subj: livechat squad: omnichannel and removed feat: livechat labels Oct 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NEW] Add JWT to uploaded files urls #15297

[NEW] Add JWT to uploaded files urls #15297

MarcosSpessatto commented Sep 3, 2019

renatobecker-zz left a comment

renatobecker-zz left a comment

renatobecker-zz Sep 6, 2019

renatobecker-zz Sep 6, 2019

renatobecker-zz Sep 6, 2019

renatobecker-zz Sep 6, 2019

renatobecker-zz Sep 6, 2019

renatobecker-zz Sep 6, 2019

renatobecker-zz Sep 6, 2019

renatobecker-zz Sep 6, 2019

renatobecker-zz Sep 6, 2019

renatobecker-zz Sep 6, 2019

renatobecker-zz Sep 11, 2019

renatobecker-zz Sep 11, 2019

renatobecker-zz Sep 11, 2019

renatobecker-zz Sep 11, 2019

renatobecker-zz Sep 11, 2019

renatobecker-zz Sep 11, 2019

renatobecker-zz Sep 11, 2019

renatobecker-zz Sep 11, 2019

renatobecker-zz Sep 11, 2019

renatobecker-zz Sep 11, 2019

[NEW] Add JWT to uploaded files urls #15297

[NEW] Add JWT to uploaded files urls #15297

Conversation

MarcosSpessatto commented Sep 3, 2019

renatobecker-zz left a comment

Choose a reason for hiding this comment

renatobecker-zz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment