-
Notifications
You must be signed in to change notification settings - Fork 2k
Save profile images to Amazon S3 #1857
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,9 @@ var _ = require('lodash'), | |
errorHandler = require(path.resolve('./modules/core/server/controllers/errors.server.controller')), | ||
mongoose = require('mongoose'), | ||
multer = require('multer'), | ||
multerS3 = require('multer-s3'), | ||
aws = require('aws-sdk'), | ||
amazonS3URI = require('amazon-s3-uri'), | ||
config = require(path.resolve('./config/config')), | ||
User = mongoose.model('User'), | ||
validator = require('validator'); | ||
|
@@ -57,10 +60,31 @@ exports.update = function (req, res) { | |
exports.changeProfilePicture = function (req, res) { | ||
var user = req.user; | ||
var existingImageUrl; | ||
var multerConfig; | ||
|
||
|
||
if (config.uploads.storage === 's3' && config.aws.s3) { | ||
aws.config.update({ | ||
accessKeyId: config.aws.s3.accessKeyId, | ||
secretAccessKey: config.aws.s3.secretAccessKey | ||
}); | ||
|
||
var s3 = new aws.S3(); | ||
|
||
multerConfig = { | ||
storage: multerS3({ | ||
s3: s3, | ||
bucket: config.aws.s3.bucket, | ||
acl: 'public-read' | ||
}) | ||
}; | ||
} else { | ||
multerConfig = config.uploads.profile.image; | ||
} | ||
|
||
// Filtering to upload only images | ||
var multerConfig = config.uploads.profile.image; | ||
multerConfig.fileFilter = require(path.resolve('./config/lib/multer')).imageFileFilter; | ||
|
||
var upload = multer(multerConfig).single('newProfilePicture'); | ||
|
||
if (user) { | ||
|
@@ -95,7 +119,9 @@ exports.changeProfilePicture = function (req, res) { | |
|
||
function updateUser() { | ||
return new Promise(function (resolve, reject) { | ||
user.profileImageURL = config.uploads.profile.image.dest + req.file.filename; | ||
user.profileImageURL = config.uploads.storage === 's3' && config.aws.s3 ? | ||
req.file.location : | ||
'/' + req.file.path; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is preventing the I'm on Windows, so it could be only happening in my environment. @Ghalleb Can you confirm if this is happening in your environment as well (if you're not on Windows, of course :) ) After adding a profile image, my user.profileImageUrl is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To fix this issue, you can update this line https://github.com/meanjs/mean/blob/master/modules/users/server/controllers/users/users.profile.server.controller.js#L112 to resolve the path for the
But it seems a little hacky to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I understand what to do for this one. For all the other changes, I will make them soon. Thanks for the review. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you verify that the existing file is getting deleted, and you're not getting an error logged to the console? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, I have an error in the server logs:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be changed back to what it was before: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not fan of the '/' in the front end. I prefer having the final URL in the database. Otherwise if we switch from local to s3, all old URL would be bad formed. And that add more logic to the front where it shouldn't be. |
||
user.save(function (err, theuser) { | ||
if (err) { | ||
reject(err); | ||
|
@@ -109,24 +135,54 @@ exports.changeProfilePicture = function (req, res) { | |
function deleteOldImage() { | ||
return new Promise(function (resolve, reject) { | ||
if (existingImageUrl !== User.schema.path('profileImageURL').defaultValue) { | ||
fs.unlink(existingImageUrl, function (unlinkError) { | ||
if (unlinkError) { | ||
if (config.uploads.storage === 's3' && config.aws.s3) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this check is being used in multiple places, you could move it into a local variable at the top level of this request handler. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe something like: Then here you can just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to let place for another storage provider... |
||
try { | ||
var { region, bucket, key } = amazonS3URI(existingImageUrl); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could be troublesome, if switching from "local" to "s3" storage. The first time a user updates their profile picture after switching the storage from one to another, this will create inconsistency with the data/files. This is probably more of an edge case, or migration consideration. Perhaps not a big deal? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about that as well. Normally you have to configure your app once! |
||
} catch(err) { | ||
console.warn(`${existingImageUrl} is not a valid S3 uri`); | ||
|
||
// If file didn't exist, no need to reject promise | ||
if (unlinkError.code === 'ENOENT') { | ||
console.log('Removing profile image failed because file did not exist.'); | ||
return resolve(); | ||
} | ||
return resolve(); | ||
} | ||
|
||
console.error(unlinkError); | ||
aws.config.update({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this needed? This is already happening up top. |
||
accessKeyId: config.aws.s3.accessKeyId, | ||
secretAccessKey: config.aws.s3.secretAccessKey | ||
}); | ||
|
||
var s3 = new aws.S3(); | ||
|
||
var params = { | ||
Bucket: config.aws.s3.bucket, | ||
Key: key | ||
}; | ||
s3.deleteObject(params, function (err) { | ||
if (err) { | ||
console.log('Error occurred while deleting old profile picture.'); | ||
console.log("Check if you have sufficient permissions : " + err); | ||
} | ||
|
||
reject({ | ||
message: 'Error occurred while deleting old profile picture' | ||
}); | ||
} else { | ||
resolve(); | ||
} | ||
}); | ||
}); | ||
} else { | ||
fs.unlink(path.resolve('.' + existingImageUrl), function (unlinkError) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not really a fan of this solution. See my comment about adding a setting to the Shared config. This might not be needed if we can keep the "/" on the front-end when we're using local storage. |
||
if (unlinkError) { | ||
|
||
// If file didn't exist, no need to reject promise | ||
if (unlinkError.code === 'ENOENT') { | ||
console.log('Removing profile image failed because file did not exist.'); | ||
return resolve(); | ||
} | ||
|
||
console.error(unlinkError); | ||
|
||
reject({ | ||
message: 'Error occurred while deleting old profile picture' | ||
}); | ||
} else { | ||
resolve(); | ||
} | ||
}); | ||
} | ||
} else { | ||
resolve(); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,7 +38,9 @@ | |
}, | ||
"dependencies": { | ||
"acl": "~0.4.10", | ||
"amazon-s3-uri": "0.0.3", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This library is rather new, and doesn't have many stars. However, the implementation looks fine and isn't too complicated. We'll probably have to keep an eye on this library though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even we follow the library updates there are few minor changes that need to be update to it. As i am a new bee to MEAN.js need support from others to get into and complicated issues like path, switching, controllers, pre & post updates need to be checked very carefully. |
||
"async": "~2.5.0", | ||
"aws-sdk": "^2.104.0", | ||
"body-parser": "~1.17.1", | ||
"bower": "~1.8.0", | ||
"chalk": "~2.1.0", | ||
|
@@ -59,6 +61,7 @@ | |
"mongoose": "~4.11.3", | ||
"morgan": "~1.8.1", | ||
"multer": "~1.3.0", | ||
"multer-s3": "^2.7.0", | ||
"nodemailer": "~4.0.1", | ||
"owasp-password-strength-test": "~1.3.0", | ||
"passport": "~0.3.2", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can add a config setting to the Shared config that would allow you to check whether the profile should be retrieved using local storage.
Then you wouldn't need remove the "/", thus solving path issues on the backend.