-
Notifications
You must be signed in to change notification settings - Fork 2k
Save profile images to Amazon S3 #1857
Changes from all 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,12 +9,27 @@ 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'); | ||
|
||
var whitelistedFields = ['firstName', 'lastName', 'email', 'username']; | ||
|
||
var useS3Storage = config.uploads.storage === 's3' && config.aws.s3; | ||
var s3; | ||
|
||
if (useS3Storage) { | ||
aws.config.update({ | ||
accessKeyId: config.aws.s3.accessKeyId, | ||
secretAccessKey: config.aws.s3.secretAccessKey | ||
}); | ||
|
||
s3 = new aws.S3(); | ||
} | ||
|
||
/** | ||
* Update user details | ||
*/ | ||
|
@@ -57,10 +72,24 @@ exports.update = function (req, res) { | |
exports.changeProfilePicture = function (req, res) { | ||
var user = req.user; | ||
var existingImageUrl; | ||
var multerConfig; | ||
|
||
|
||
if (useS3Storage) { | ||
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 +124,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 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 +140,47 @@ 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 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' | ||
if (useS3Storage) { | ||
try { | ||
var { region, bucket, key } = amazonS3URI(existingImageUrl); | ||
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); | ||
} | ||
|
||
resolve(); | ||
}); | ||
} else { | ||
resolve(); | ||
} catch (err) { | ||
console.warn(`${existingImageUrl} is not a valid S3 uri`); | ||
|
||
return resolve(); | ||
} | ||
}); | ||
} else { | ||
fs.unlink(path.resolve('.' + existingImageUrl), function (unlinkError) { | ||
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.