Add avatar photo upload component #101

makoncline · 2020-01-23T02:14:46Z

User can upload avatar image
File is checked for type and size
File is uploaded to AWS S3 bucket using pre signed url
File is deleted with pre signed url
NEXT API route used for pre signing urls
Bucket has public read access for viewing

not many changes needed to allow upload of multiple user photos.
(don't limit the file list to a single value and keep displaying upload button)

There are MANY things that can be improved, but it's what i've got.

benjie · 2020-01-24T12:31:43Z

Thanks for this! It's going to take me a while to getting around to improving it/merging it, but in the mean time it acts as a great demonstration of how to accomplish this to other users 👍

You need to add @app/graphql as a dependency to @app/components/package.json; I believe this is why CI is failing.

benjie · 2020-01-27T17:57:42Z

$ prettier --ignore-path .eslintignore --check '**/*.{js,jsx,ts,tsx,graphql,md}'
Checking formatting...
README.md
Code style issues found in the above file(s). Forgot to run Prettier?

^ To solve this, yarn lint:fix (yes, we even format markdown! 😁)

singingwolfboy

This is a great start, but I found a few things that should probably be improved before merge. In particular, I think this is passing the AWS secret key to the browser, which is a big security problem.

singingwolfboy · 2020-02-01T11:30:13Z

@app/client/src/pages/api/s3.tsx

+      if (err) {
+        res.json({ success: false, err });
+      } else {
+        res.json({ success: true, url });


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

singingwolfboy · 2020-02-01T11:37:26Z

@app/components/src/AvatarUpload.tsx

+} from "@app/graphql";
+import { ApolloError } from "apollo-client";
+
+export function slugify(string: string) {


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.

singingwolfboy · 2020-02-01T11:42:39Z

@app/components/src/AvatarUpload.tsx

+  };
+
+  const uploadButton = (
+    <div>


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.

singingwolfboy · 2020-02-01T11:53:10Z

@app/client/src/next.config.js

+      serverRuntimeConfig: {
+        BUCKET: BUCKET,
+        AWSACCESSKEYID: AWSACCESSKEYID,
+        AWSSECRETKEY: AWSSECRETKEY,


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.

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.

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.

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.

singingwolfboy · 2020-02-01T11:54:48Z

@app/components/src/AvatarUpload.tsx

+    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!");


Why this restriction?

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.

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.

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

benjie · 2020-05-15T13:01:55Z

I tried to get this into a mergeable state in #167 but I ran out of time.

Makon Cline and others added 12 commits January 22, 2020 19:16

add avatar upload component

3721586

schema auto change

11e4796

add axios

9d0bbbd

add change avatar mutation

02135d3

add cors. This is bad...I think

156e92e

add aws env vars

3644031

add s3 api route

5190ff5

use avatarPhoto if exists

acaffb7

yarn lock change

40c4ffb

add avatar upload to settings page

02ffa19

add labels to error, success, and avatar form item

882ab58

replace readme with instructions

594364c

add @app/graphql

84e304f

singingwolfboy suggested changes Feb 1, 2020

View reviewed changes

singingwolfboy mentioned this pull request Feb 10, 2020

Add upload router for signed S3 upload URLs #107

Closed

benjie mentioned this pull request May 15, 2020

Upload avatar #167

Draft

10 tasks

benjie changed the base branch from master to main June 24, 2020 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add avatar photo upload component #101

Add avatar photo upload component #101

makoncline commented Jan 23, 2020

benjie commented Jan 24, 2020

benjie commented Jan 27, 2020

singingwolfboy left a comment

singingwolfboy Feb 1, 2020

singingwolfboy Feb 1, 2020

singingwolfboy Feb 1, 2020

singingwolfboy Feb 1, 2020

makoncline Feb 1, 2020

singingwolfboy Feb 1, 2020 •

edited

makoncline Feb 1, 2020

singingwolfboy Feb 1, 2020

makoncline Feb 1, 2020

singingwolfboy Feb 1, 2020

benjie Feb 3, 2020

benjie commented May 15, 2020

Add avatar photo upload component #101

Are you sure you want to change the base?

Add avatar photo upload component #101

Conversation

makoncline commented Jan 23, 2020

benjie commented Jan 24, 2020

benjie commented Jan 27, 2020

singingwolfboy 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

singingwolfboy Feb 1, 2020 • edited

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

benjie commented May 15, 2020

singingwolfboy Feb 1, 2020 •

edited