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

Add avatar photo upload component #101

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions @app/client/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"apollo-link-error": "^1.1.11",
"apollo-link-http": "^1.5.15",
"apollo-link-ws": "^1.0.18",
"axios": "^0.19.1",
"graphql": "^14.4.2",
"less": "^3.9.0",
"less-vars-to-js": "^1.3.0",
Expand Down
9 changes: 9 additions & 0 deletions @app/client/src/graphql/ChangeAvatar.graphql
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
}
}
}
11 changes: 8 additions & 3 deletions @app/client/src/layout/SharedLayout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -132,9 +132,14 @@ function SharedLayout({ title, noPad = false, children }: SharedLayoutProps) {
data-cy="layout-dropdown-user"
style={{ whiteSpace: "nowrap" }}
>
<Avatar>
{(data.currentUser.name && data.currentUser.name[0]) || "?"}
</Avatar>
{data.currentUser.avatarUrl ? (
<Avatar src={`${data.currentUser.avatarUrl}`} />
) : (
<Avatar>
{(data.currentUser.name && data.currentUser.name[0]) ||
"?"}
</Avatar>
)}
<Warn okay={data.currentUser.isVerified}>
<span style={{ marginLeft: 8, marginRight: 8 }}>
{data.currentUser.name}
Expand Down
15 changes: 14 additions & 1 deletion @app/client/src/next.config.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,14 @@
require("@app/config");
const compose = require("lodash/flowRight");

const { ROOT_URL, T_AND_C_URL } = process.env;
const {
ROOT_URL,
T_AND_C_URL,
BUCKET,
AWSACCESSKEYID,
AWSSECRETKEY,
AWS_REGION,
} = process.env;
if (!ROOT_URL) {
throw new Error("ROOT_URL is a required envvar");
}
Expand Down Expand Up @@ -31,6 +38,12 @@ if (!ROOT_URL) {
withCss,
withLess
)({
serverRuntimeConfig: {
BUCKET: BUCKET,
AWSACCESSKEYID: AWSACCESSKEYID,
AWSSECRETKEY: AWSSECRETKEY,
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

image

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.

Copy link
Collaborator

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 that serverRuntimeConfig 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.

Copy link
Collaborator Author

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.

AWS_REGION: AWS_REGION,
},
poweredByHeader: false,
distDir: `../.next`,
exportTrailingSlash: true,
Expand Down
67 changes: 67 additions & 0 deletions @app/client/src/pages/api/s3.tsx
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 });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than using a success key in the JSON payload, it's better to use the HTTP status code to communicate an error. In this case, if the error is coming from S3, that indicates the status "502 Bad Gateway" because our server is acting as a gateway (or proxy) to S3.

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 });
}
});
}
};
13 changes: 10 additions & 3 deletions @app/client/src/pages/settings/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { ApolloError } from "apollo-client";
import { FormComponentProps, ValidateFieldsOptions } from "antd/lib/form/Form";
import { getCodeFromError, extractError } from "../../errors";
import { formItemLayout, tailFormItemLayout } from "../../forms";
import { Redirect, ErrorAlert, H3 } from "@app/components";
import { Redirect, ErrorAlert, H3, AvatarUpload } from "@app/components";

const Settings_Profile: NextPage = () => {
const [formError, setFormError] = useState<Error | ApolloError | null>(null);
Expand Down Expand Up @@ -114,6 +114,13 @@ function ProfileSettingsForm({
<div>
<H3>Edit Profile</H3>
<Form {...formItemLayout} onSubmit={handleSubmit}>
<Form.Item label="Avatar">
<AvatarUpload
user={user}
setError={setError}
setSuccess={setSuccess}
/>
</Form.Item>
<Form.Item label="Name">
{getFieldDecorator("name", {
initialValue: user.name,
Expand All @@ -137,7 +144,7 @@ function ProfileSettingsForm({
})(<Input />)}
</Form.Item>
{error ? (
<Form.Item>
<Form.Item label="Error">
<Alert
type="error"
message={`Updating username`}
Expand All @@ -155,7 +162,7 @@ function ProfileSettingsForm({
/>
</Form.Item>
) : success ? (
<Form.Item>
<Form.Item label="success">
<Alert type="success" message={`Profile updated`} />
</Form.Item>
) : null}
Expand Down
3 changes: 2 additions & 1 deletion @app/components/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"apollo-client": "^2.6.8",
"next": "^9.2.0",
"react": "^16.9.0",
"tslib": "^1.10.0"
"tslib": "^1.10.0",
"@app/graphql": "0.0.0"
}
}
208 changes: 208 additions & 0 deletions @app/components/src/AvatarUpload.tsx
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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 yarn.lock, and it looks like we already depends on a library called unique-slug, which would probably suit your needs just fine.

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!");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this restriction?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.
You can give it a content type, but it will still accept any.
Same with file size. So, thats a problem if the user want to be malicious.
I think a different method would need to be used to enforce file size and type on the S3 end.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 image/. See the MDN article on MIME types to see some examples.

Copy link
Member

Choose a reason for hiding this comment

The 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>
Copy link
Collaborator

Choose a reason for hiding this comment

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

For accessibility purposes, this should use a <button> element. It should probably also have aria-label set to something like "Upload avatar". You can use the Button component in antd.

<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>
);
}
1 change: 1 addition & 0 deletions @app/components/src/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ export * from "./StandardWidth";
export * from "./Text";
export * from "./Warn";
export * from "./PasswordStrength";
export * from "./AvatarUpload";