-
-
Notifications
You must be signed in to change notification settings - Fork 221
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
Add avatar photo upload component #101
base: main
Are you sure you want to change the base?
Changes from all commits
3721586
11e4796
9d0bbbd
02135d3
156e92e
3644031
5190ff5
acaffb7
40c4ffb
02ffa19
882ab58
594364c
84e304f
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 |
---|---|---|
@@ -0,0 +1,9 @@ | ||
mutation ChangeAvatar($id: Int!, $patch: UserPatch!) { | ||
updateUser(input: { id: $id, patch: $patch }) { | ||
clientMutationId | ||
user { | ||
id | ||
avatarUrl | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
import { NextApiRequest, NextApiResponse } from "next"; | ||
import AWS from "aws-sdk"; | ||
import getConfig from "next/config"; | ||
const { serverRuntimeConfig } = getConfig(); | ||
|
||
export default async (req: NextApiRequest, res: NextApiResponse) => { | ||
const bucket = serverRuntimeConfig.BUCKET; | ||
const key = req.query.key; | ||
const params: AWS.S3.PutObjectRequest = { | ||
Bucket: bucket, | ||
Key: key as string, | ||
}; | ||
const client = getClient(); | ||
const operation = req.query.operation; | ||
if (operation === "put") { | ||
put(client, params); | ||
} else if (operation === "delete") { | ||
del(client, params); | ||
} | ||
|
||
function getClient() { | ||
const region = serverRuntimeConfig.AWS_REGION; | ||
const accessKey = serverRuntimeConfig.AWSACCESSKEYID; | ||
const secretKey = serverRuntimeConfig.AWSSECRETKEY; | ||
AWS.config.update({ | ||
accessKeyId: accessKey, | ||
secretAccessKey: secretKey, | ||
signatureVersion: "v4", | ||
region: region, | ||
}); | ||
const options = { | ||
signatureVersion: "v4", | ||
region: region, | ||
// uncomment to use accelerated endpoint | ||
// accelerated endpoint must be turned on in your s3 bucket first | ||
// endpoint: new AWS.Endpoint( | ||
// "bucket.s3-accelerate.amazonaws.com" | ||
// ), | ||
// useAccelerateEndpoint: true, | ||
}; | ||
const client = new AWS.S3(options); | ||
return client; | ||
} | ||
function put(client: AWS.S3, params: AWS.S3.PutObjectRequest) { | ||
const putParams = { | ||
...params, | ||
Expires: 5 * 60, | ||
}; | ||
|
||
client.getSignedUrl("putObject", putParams, (err, url) => { | ||
if (err) { | ||
res.json({ success: false, err }); | ||
} else { | ||
res.json({ success: true, url }); | ||
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. Rather than using a if (err) {
res.status(504).json({ err });
} else {
res.json({ url });
} |
||
} | ||
}); | ||
} | ||
function del(client: AWS.S3, params: AWS.S3.DeleteObjectRequest) { | ||
client.deleteObject(params, err => { | ||
if (err) { | ||
res.json({ success: false, err }); | ||
} else { | ||
res.json({ success: true }); | ||
} | ||
}); | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,208 @@ | ||
import React, { useState, useEffect } from "react"; | ||
import { Upload, Icon, message } from "antd"; | ||
import { UploadChangeParam } from "antd/lib/upload"; | ||
import { UploadFile, RcCustomRequestOptions } from "antd/lib/upload/interface"; | ||
import axios from "axios"; | ||
import { | ||
useChangeAvatarMutation, | ||
ProfileSettingsForm_UserFragment, | ||
} from "@app/graphql"; | ||
import { ApolloError } from "apollo-client"; | ||
|
||
export function slugify(string: string) { | ||
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. Instead of defining your own function to make a slug, it's probably better to use an existing package. I just searched through |
||
const a = | ||
"àáâäæãåāăąçćčđďèéêëēėęěğǵḧîïíīįìłḿñńǹňôöòóœøōõőṕŕřßśšşșťțûüùúūǘůűųẃẍÿýžźż·/_,:;"; | ||
const b = | ||
"aaaaaaaaaacccddeeeeeeeegghiiiiiilmnnnnoooooooooprrsssssttuuuuuuuuuwxyyzzz------"; | ||
const p = new RegExp(a.split("").join("|"), "g"); | ||
|
||
return string | ||
.toString() | ||
.toLowerCase() | ||
.replace(/\s+/g, "-") // Replace spaces with - | ||
.replace(p, c => b.charAt(a.indexOf(c))) // Replace special characters | ||
.replace(/&/g, "-and-") // Replace & with 'and' | ||
.replace(/[^\w\-]+/g, "") // Remove all non-word characters | ||
.replace(/\-\-+/g, "-") // Replace multiple - with single - | ||
.replace(/^-+/, "") // Trim - from start of text | ||
.replace(/-+$/, ""); // Trim - from end of text | ||
} | ||
|
||
export function getUid(name: string) { | ||
const randomHex = () => Math.floor(Math.random() * 16777215).toString(16); | ||
const fileNameSlug = slugify(name); | ||
return randomHex() + "-" + fileNameSlug; | ||
} | ||
|
||
export function AvatarUpload({ | ||
user, | ||
setSuccess, | ||
setError, | ||
}: { | ||
user: ProfileSettingsForm_UserFragment; | ||
setSuccess: React.Dispatch<React.SetStateAction<boolean>>; | ||
setError: (error: Error | ApolloError | null) => void; | ||
}) { | ||
const [changeAvatar] = useChangeAvatarMutation(); | ||
const [fileList, setFileList] = useState<any>( | ||
user && user.avatarUrl | ||
? [ | ||
{ | ||
uid: "-1", | ||
name: "avatar", | ||
type: "image", | ||
size: 1, | ||
url: user.avatarUrl, | ||
}, | ||
] | ||
: null | ||
); | ||
|
||
useEffect(() => { | ||
if (user) { | ||
const avatar = user.avatarUrl; | ||
if (avatar) { | ||
setFileList([ | ||
{ | ||
uid: "-1", | ||
name: "avatar", | ||
type: "image", | ||
size: 1, | ||
url: avatar, | ||
}, | ||
]); | ||
} else { | ||
setFileList(null); | ||
} | ||
} | ||
}, [user, user.avatarUrl]); | ||
|
||
// const onChange = (info: UploadChangeParam) => { | ||
// console.log(info); | ||
// setFileList([...fileList]); | ||
// }; | ||
|
||
const beforeUpload = (file: any) => { | ||
const fileName = file.name.split(".")[0]; | ||
const fileType = file.name.split(".")[1]; | ||
file.uid = getUid(fileName) + "." + fileType; | ||
const isJpgOrPng = file.type === "image/jpeg" || file.type === "image/png"; | ||
if (!isJpgOrPng) { | ||
message.error("You can only upload JPG or PNG images!"); | ||
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. Why this restriction? 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. Just to restrict the file type to an image format. What other image formats should be supported? As far as I can tell, you can't restrict what type of file is uploaded with a pre signed url. 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 don't see anything wrong with accepting all image types: that is, all MIME types that begin with 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. It should probably be an image format you can render in most browsers unless you're using an avatar resizing helper. You might also want to restrict things like people using GIF avatars because it can be pretty irritating... |
||
file.status = "error"; | ||
} | ||
const isLt3M = file.size / 1024 / 1024 < 3; | ||
if (!isLt3M) { | ||
message.error("Image must smaller than 3MB!"); | ||
file.status = "error"; | ||
} | ||
return isJpgOrPng && isLt3M; | ||
}; | ||
|
||
const changeUserAvatar = async (avatarUrl: string | null) => { | ||
setSuccess(false); | ||
setError(null); | ||
try { | ||
await changeAvatar({ | ||
variables: { | ||
id: user.id, | ||
patch: { | ||
avatarUrl, | ||
}, | ||
}, | ||
}); | ||
setError(null); | ||
setSuccess(true); | ||
} catch (e) { | ||
setError(e); | ||
} | ||
}; | ||
|
||
const customRequest = (option: RcCustomRequestOptions) => { | ||
const { onSuccess, onError, file, onProgress } = option; | ||
axios | ||
.get(`${process.env.ROOT_URL}/api/s3`, { | ||
params: { | ||
key: file.uid, | ||
operation: "put", | ||
}, | ||
}) | ||
.then(response => { | ||
const preSignedUrl = response.data.url; | ||
axios | ||
.put(preSignedUrl, file, { | ||
onUploadProgress: e => { | ||
const progress = Math.round((e.loaded / e.total) * 100); | ||
onProgress({ percent: progress }, file); | ||
}, | ||
}) | ||
.then(response => { | ||
if (response.config.url) { | ||
changeUserAvatar(response.config.url.split("?")[0]); | ||
onSuccess(response.config, file); | ||
} | ||
}) | ||
.catch(error => { | ||
console.log(error); | ||
onError(error); | ||
}); | ||
}) | ||
.catch(error => { | ||
console.log(error); | ||
onError(error); | ||
}); | ||
}; | ||
|
||
const deleteUserAvatarFromBucket = async () => { | ||
if (user && user.avatarUrl) { | ||
const key = user.avatarUrl.substring(user.avatarUrl.lastIndexOf("/") + 1); | ||
await axios | ||
.get(`${process.env.ROOT_URL}/api/s3`, { | ||
params: { | ||
key: `${key}`, | ||
operation: "delete", | ||
}, | ||
}) | ||
.then(() => { | ||
// this isn't confirmation that the item was deleted | ||
// only confimation that there wasnt an error.. | ||
changeUserAvatar(null); | ||
return true; | ||
}) | ||
.catch(error => { | ||
console.log(JSON.stringify(error)); | ||
return false; | ||
}); | ||
} | ||
return true; | ||
}; | ||
|
||
const onRemove = async () => { | ||
if (await deleteUserAvatarFromBucket()) { | ||
setFileList(null); | ||
} | ||
}; | ||
|
||
const uploadButton = ( | ||
<div> | ||
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. For accessibility purposes, this should use a |
||
<Icon type="plus" /> | ||
<div className="ant-upload-text">Avatar</div> | ||
</div> | ||
); | ||
|
||
return ( | ||
<div> | ||
<Upload | ||
name="avatar" | ||
listType="picture-card" | ||
fileList={fileList} | ||
beforeUpload={beforeUpload} | ||
customRequest={customRequest} | ||
onRemove={onRemove} | ||
showUploadList={{ showPreviewIcon: false, showDownloadIcon: false }} | ||
> | ||
{fileList && fileList.length >= 0 ? null : uploadButton} | ||
</Upload> | ||
</div> | ||
); | ||
} |
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.
This is extremely insecure: you are passing the AWS secret key to the browser! That means any user of your application can find your secret key, and make AWS API requests on your behalf! 😱
A better, more secure way of doing this is to generate a signed upload URL on the server, and only pass that signed URL to the client. That way, the client has enough information to do what it needs to do (upload files to S3), but nothing more than that. Fortunately, there is a library called
react-s3-uploader
that is designed to work in exactly this way. I highly suggest using this library.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.
Thanks for the code review! I've been programming solo so far, so its good to get feedback.
This is the suggested way to pass server only runtime configuration to Next.
I think..... that these are only available on the server side, since it's used only in a next api route.
The intent is that this does exactly what you said, "...generate a signed upload URL on the server, and only pass that signed URL to the client".
I'll look into using the
react-s3-uploader
library.Can you confirm if this is actually sending the serverRuntimeConfig: credentials to the client??
If so, I have some other things I need to change quickly 😳...
At least I'm using limited IAM roles.
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.
Oh dear, I'm sorry for the panic. After reading the Next.js docs more carefully, I think you're right: you've done it correctly, and the credentials should stay on the server.
I saw that you put the credentials in the config, and then imported and used them in a file under the
/pages
directory -- and in my limited experience with Next.js, I thought that everything under/pages
is run on both the server and the client. But I didn't know thatserverRuntimeConfig
is special and/pages/api
is special. (Seems weird to me that an API is a subset of a page...)I tried to run the project locally, just to check for sure. Unfortunately, I ran into a bug with graphql-codegen that prevented me from running it. However, you should be able to check yourself by searching all of your client-side javascript in Chrome DevTools. If you search for your secret key (or even just for the string "AWSSECRETKEY", since you used that in the config), then you should get no results.
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.
No result in the search when searching for the key or any of the other server side vars.
So, at least that part seems to be secure.
I still agree that the code quality could be better.