Exercise 4 #10

NicolasTsc · 2020-11-25T14:42:25Z

Unser Codereview: Systems-Development-and-Frameworks/AlElMo#7 (review)

roschaefer

I received an E-Mail from a member of your group, asking me to do a Feedback-Review until Wednesday. Please request a review from @mentors in the first week next time.

See my suggestions below.

roschaefer · 2020-12-07T19:53:11Z

.github/workflows/build-lint-test.yml

+        touch .env
+        echo JWTSECRET=$JWTSECRET >> .env


Suggested change

touch .env

echo JWTSECRET=$JWTSECRET >> .env

If you use dotenv or dotenv-flow you can overwrite those values specified in .env with your current process.env.

Have you tried deleting .env and then run your sever with:

$ JWTSECRET=whatever yarn run dev

This should work.

Thanks for the review Robert. As I cant comment your review message I have to write it here. We actually did request a mentors-review in the first week of the exercise, so maybe the notification didnt work as expected.

roschaefer · 2020-12-07T19:55:23Z

.github/workflows/build-lint-test.yml

        npm install
        npm run test
        npm run lint
      env:
-        CI: true
+        CI: true,
+        JWTSECRET: ${{ secrets.JWTSECRET }}


That's the only necessary line of code.

roschaefer · 2020-12-07T19:56:26Z

backend/README.md

@@ -1,5 +1,8 @@
 # Apollo Backend

+## Mandatory for running tests ´
+Create a .env file in the root backend folder and add a secret JWTSECRET for signing the JWT token (e.g. JWTSECRET=yoursecret)


You could also check in a .env.template with dummy secrets and then tell the user to

$ cp .env.template .env

this file.

roschaefer · 2020-12-07T20:03:24Z

backend/src/index.spec.ts

+require('dotenv').config()
+
+const testContext = (testToken?) => {
+  let token = testToken || ''
+  token = token.replace('Bearer ', '')
+  try {
+    const decodedJwt = jwt.verify(
+      token,
+      process.env.JWTSECRET
+    )
+    return { decodedJwt }
+  } catch (e) {
+    return {}
+  }
+}


Suggested change

require('dotenv').config()

const testContext = (testToken?) => {

let token = testToken || ''

token = token.replace('Bearer ', '')

try {

const decodedJwt = jwt.verify(

token,

process.env.JWTSECRET

)

return { decodedJwt }

} catch (e) {

return {}

}

}

import context from './context'

const reqMock

const testContext = () => context({ req: reqMock })

beforeEach(() => {

reqMock = {} // by default

})

// later in your test cases that simulate a logged in user

beforeEach(() => {

reqMock = { headers: { authorization: `Bearer ${testToken}` } }

})

See: apollographql/apollo-server#2277 (comment)

roschaefer · 2020-12-07T20:07:15Z

backend/src/index.spec.ts

+  const schema = makeExecutableSchema({ typeDefs, resolvers })
+  const server = new ApolloServer({
+    schema: applyMiddleware(schema, permissions),
+    context: testContext(testToken), // not sure if this is the correct way. but we didn`t find another solution to add the token as request (see https://github.com/apollographql/apollo-server/issues/2277)


This will call testContext(testToken) when you run setupServer:

Suggested change

context: testContext(testToken), // not sure if this is the correct way. but we didn`t find another solution to add the token as request (see https://github.com/apollographql/apollo-server/issues/2277)

context: testContext(testToken),

This gives you extra flexibility, because it's called when you run query or mutate:

Suggested change

context: testContext(testToken), // not sure if this is the correct way. but we didn`t find another solution to add the token as request (see https://github.com/apollographql/apollo-server/issues/2277)

context: () => testContext(testToken),

roschaefer · 2020-12-07T20:33:24Z

backend/src/postsDataSource.ts

@@ -1,5 +1,8 @@
 import { DataSource } from 'apollo-datasource'
 import { v4 as uuidv4 } from 'uuid'
+require('dotenv').config()


Why do you require('dotenv').config() so often? I would expect only one file to require it and this file gets imported wherever needed.

roschaefer · 2020-12-07T20:35:00Z

backend/src/postsDataSource.ts

+    if (this.users.find(user => user.id === id)) {
+      return true
+    } else {
+      return false
+    }


Suggested change

if (this.users.find(user => user.id === id)) {

return true

} else {

return false

}

return this.users.find(user => user.id === id)

roschaefer · 2020-12-07T20:37:54Z

backend/src/postsDataSource.ts

+
+    if (foundUser) {
+      if (bcrypt.compareSync(password, foundUser.password)) {
+        return jwt.sign({ id: foundUser.id }, process.env.JWTSECRET, { algorithm: 'HS256' })


Suggested change

return jwt.sign({ id: foundUser.id }, process.env.JWTSECRET, { algorithm: 'HS256' })

return jwt.sign({ id: foundUser.id }, JWTSECRET, { algorithm: 'HS256' })

Your JWTSECRET could be a constant imported from src/import which is the file calling require('dotenv').config().

roschaefer · 2020-12-07T20:39:39Z

backend/src/postsDataSource.ts

+  login (email, password) {
+    const foundUser = this.getUsers().find(user => user.email === email)
+
+    if (foundUser) {


import { AuthenticationError } from 'apollo-server' // ... if (foundUser && foundUser.checkPassword(password)) return jwt.sign({ id: foundUser.id }, JWTSECRET) throw new Error('Wrong email/password combination')

roschaefer · 2020-12-07T20:40:59Z

backend/src/postsDataSource.ts

+  }
+
+  addUser (name, email, password) {
+    if (password.length >= 8) {


Prefer guard-clauses over nested if-clauses, for readability:

Suggested change

if (password.length >= 8) {

if (password.length < 8) throw new UserInputError('Password must have at least 8 characters')

NicolasTsc · 2020-12-08T08:00:00Z

backend/src/postsDataSource.ts

+    }
+    const foundUser = this.getUsers().find(user => user.email === email)
+    if (foundUser) {
+      throw new ForbiddenError('Email already taken by another user')


Why is the thrown error in the tests or the graphql playground "Not Authorized" even if we throw different errors here?

I believe you must allowExternalErrors: true in graphql-shield.

AlexHTW

Well done! Looks like you successfully implemeted all the objectives.

AlexHTW · 2020-12-09T00:24:31Z

backend/src/index.spec.ts

+  it('signup with password shorter than 8 characters return error', async () => {
+    const { mutate } = setupServerAndReturnTestClient(posts)
+    const SIGNUP = 'mutation {  signup (name: "testuser", email: "testuser@example.org", password: "short") }'
+    const res = await mutate({ mutation: SIGNUP })
+    expect(res.errors[0].message).toEqual('Not Authorised!')
+    expect(res.data.signup).toBeNull()
+  })
+
+  it('signup with a valid password returns token', async () => {
+    const { mutate } = setupServerAndReturnTestClient(posts)
+    const SIGNUP = 'mutation {  signup (name: "testuser", email: "testuser@example.org", password: "password") }'
+    const res = await mutate({ mutation: SIGNUP })
    expect(res.errors).toBeUndefined()
-    expect(res.data.write.title).toEqual('TestTitle')
-    expect(res.data.write.author.name).toEqual('user1')
-    expect(res.data.write.author.posts.length).toEqual(2)
+    expect(res.data.signup).toEqual(expect.any(String))
  })

-  it('add a post from a new user adds a new user', async () => {
-    const postData = [
-      { id: 'post1', title: 'Item 1', votes: 0, voters: [], author: {} }
-    ]
-    const userData = [
-      { name: 'user1', posts: [postData[0]] }
-    ]
-    postData[0].author = userData[0]
+  it('trying to signup the same user twice returns error the second time', async () => {
+    const { mutate } = setupServerAndReturnTestClient(posts)
+    const SIGNUP = 'mutation {  signup (name: "testuser", email: "testuser@example.org", password: "password") }'
+    await mutate({ mutation: SIGNUP })
+    const res = await mutate({ mutation: SIGNUP })
+    expect(res.errors[0].message).toEqual('Not Authorised!')
+    expect(res.data.signup).toBeNull()
+  })


Great tests!
⭐ For a software-tested signup feature.

AlexHTW · 2020-12-09T00:26:03Z

backend/src/index.spec.ts

+  it('trying to login with invalid password returns error', async () => {
+    const { mutate } = setupServerAndReturnTestClient(posts)
+    const SIGNUP = 'mutation {  signup (name: "testuser", email: "testuser@example.org", password: "password") }'
+    await mutate({ mutation: SIGNUP })
+    const LOGIN = 'mutation {  login (email: "testuser@example.org", password: "invalid") }'
+    const res = await mutate({ mutation: LOGIN })
+    expect(res.errors[0].message).toEqual('Not Authorised!')
+    expect(res.data.login).toBeNull()
+  })

-    const GET_USERS = '{ users { name }}'
-    const { query } = createTestClient(server)
-    const resQuery = await query({ query: GET_USERS })
-    expect(resQuery.errors).toBeUndefined()
-    expect(resQuery.data.users.length).toEqual(2)
+  it('trying to login with valid data returns token', async () => {
+    const { mutate } = setupServerAndReturnTestClient(posts)
+    const SIGNUP = 'mutation {  signup (name: "testuser", email: "testuser@example.org", password: "password") }'
+    await mutate({ mutation: SIGNUP })
+    const LOGIN = 'mutation {  login (email: "testuser@example.org", password: "password") }'
+    const res = await mutate({ mutation: LOGIN })
+    expect(res.errors).toBeUndefined()
+    expect(res.data.login).toEqual(expect.any(String))


One more:
⭐ For a software-tested login feature.

AlexHTW · 2020-12-09T00:38:29Z

backend/src/postsDataSource.ts

+    if (foundUser) {
+      throw new ForbiddenError('Email already taken by another user')


You earned a
⭐ For unique email address validation.

AlexHTW · 2020-12-09T00:49:05Z

backend/src/postsDataSource.ts

+  login (email, password) {
+    const foundUser = this.getUsers().find(user => user.email === email)
+
+    if (foundUser) {
+      if (compareSync(password, foundUser.password)) {
+        return sign({ id: foundUser.id }, JWTSECRET, { algorithm: 'HS256' })


Now you even earned two at once!
⭐ For an implemented login feature using JWT.
⭐ For password validation.

AlexHTW · 2020-12-09T00:53:48Z

backend/src/postsDataSource.ts

+    const foundUser = this.getUsers().find(user => user.email === email)
+    if (foundUser) {
+      throw new ForbiddenError('Email already taken by another user')
+    }
+    const user = new User(uuidv4(), name, email, hashSync(password, 10))
+    this.users.push(user)
+    return sign({ id: user.id }, JWTSECRET, { algorithm: 'HS256' })


Good job.
⭐ For an implemented signup feature.

AlexHTW · 2020-12-09T01:04:02Z

backend/src/resolvers.ts

+      return context.dataSources.posts.addPost(post, context.decodedJwt.id)
    },
    upvote: (parent, { id, voter }, context, info) => {
-      return context.dataSources.posts.upvote(id, voter)
+      return context.dataSources.posts.upvote(id, context.decodedJwt.id)


⭐ For assigning the authenticated user to a post in write and upvote mutations.

roschaefer

Hey Felni, this is a great submission! You achieved all 10/10 ⭐ in
particular:

⭐ For an implemented signup feature.

⭐ For a software-tested signup feature.

⭐ For password validation.

⭐ For unique email address validation.

⭐ For not having security issues according to instructions.

⭐ For an implemented login feature using JWT.

⭐ For a software-tested login feature.

⭐ For assigning the authenticated user to a post in write and upvote mutations.

⭐ For requesting a review and reviewing another team's PR according to the instructions.

⭐ For a successful rebase on homework/main.

Since my last review, you haven't implemented all suggestions. So please have a
look. I think you could refactor your tests in general:

Check what needs to be re-initialized. Only datasources and context needs
to be re-initialized, usually.
Prefer to use .toMatchObject expectation when checking GraphQL responses. You
can check a lot more and it's easier to read.
Move as much logic as possible from the test code into the imported code. You
loose test coverage if you implement parts of the code in your tests (e.g.
password hashing, schema setup).

roschaefer · 2020-12-20T15:30:49Z

backend/src/index.spec.ts

-    ]
+  beforeEach(() => {
+    const postData = [new Post('post1', 'Item1')]
+    const userData = [new User('userid1', 'user1', 'user1@example.org', hashSync('user1password', 10))]


Why is it necessary that you call hashSync in the test? Why don't you simply put that into the constructor?

roschaefer · 2020-12-20T15:32:38Z

backend/src/post.ts

+      this.title = title
+      this.votes = 0
+      this.voters = []
+      this.author = author || {}


A post should never have no author!

roschaefer · 2020-12-20T16:07:35Z

backend/src/postsDataSource.ts

      }
+      throw new AuthenticationError('Wrong email/password combination')


I think this should be below the outmost if statement. What if foundUser is falsy?

NicolasTsc requested review from moengers and a team November 28, 2020 07:31

NicolasTsc changed the title ~~add graph packages and update readme~~ Exercise 4 Nov 30, 2020

NicolasTsc force-pushed the main branch from 47bd2e9 to e464c32 Compare December 2, 2020 14:54

NicolasTsc and others added 13 commits December 2, 2020 15:59

add graph packages and update readme

5620160

add signup

174eeff

add login method and shields WIP

7159620

lint

ec16a02

add nodeman, refactor code and implement security check

8efd893

added and fixed tests

a38aa07

Update package-lock.json

13e7017

add console log

5d4eb9b

try to fix tests

7a40821

Update package.json

ac90d65

add dotenv

66a341b

add .env file to github workflow

141b7da

add secret

1e4434f

NicolasTsc force-pushed the exercise-4 branch from f616da5 to 1e4434f Compare December 2, 2020 15:00

Update README.md

e7a9ebb

NicolasTsc requested a review from a team December 6, 2020 08:33

roschaefer reviewed Dec 7, 2020

View reviewed changes

refactor and improve code

e0737f0

NicolasTsc commented Dec 8, 2020

View reviewed changes

NicolasTsc added 2 commits December 8, 2020 09:17

Update index.spec.ts

ab06ebd

minimize code, add user and post class,

098e1bf

AlexHTW approved these changes Dec 9, 2020

View reviewed changes

AlexHTW mentioned this pull request Dec 9, 2020

Ex4 Systems-Development-and-Frameworks/AlElMo#7

Merged

roschaefer approved these changes Dec 20, 2020

View reviewed changes

Merge branch 'main' into exercise-4

2121f2e

NicolasTsc merged commit 968c21c into main Dec 30, 2020

NicolasTsc deleted the exercise-4 branch December 30, 2020 08:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exercise 4 #10

Exercise 4 #10

NicolasTsc commented Nov 25, 2020 •

edited

roschaefer left a comment •

edited

roschaefer Dec 7, 2020

roschaefer Dec 7, 2020

NicolasTsc Dec 8, 2020

roschaefer Dec 7, 2020

roschaefer Dec 7, 2020

roschaefer Dec 7, 2020

roschaefer Dec 7, 2020

roschaefer Dec 7, 2020

roschaefer Dec 7, 2020

roschaefer Dec 7, 2020

roschaefer Dec 7, 2020

roschaefer Dec 7, 2020

roschaefer Dec 7, 2020

NicolasTsc Dec 8, 2020

roschaefer Dec 14, 2020

AlexHTW left a comment

AlexHTW Dec 9, 2020

AlexHTW Dec 9, 2020

AlexHTW Dec 9, 2020

AlexHTW Dec 9, 2020

AlexHTW Dec 9, 2020

AlexHTW Dec 9, 2020

roschaefer left a comment

roschaefer Dec 20, 2020 •

edited

roschaefer Dec 20, 2020

roschaefer Dec 20, 2020

	context: testContext(testToken), // not sure if this is the correct way. but we didn`t find another solution to add the token as request (see https://github.com/apollographql/apollo-server/issues/2277)
	context: testContext(testToken),

	context: testContext(testToken), // not sure if this is the correct way. but we didn`t find another solution to add the token as request (see https://github.com/apollographql/apollo-server/issues/2277)
	context: () => testContext(testToken),

	return jwt.sign({ id: foundUser.id }, process.env.JWTSECRET, { algorithm: 'HS256' })
	return jwt.sign({ id: foundUser.id }, JWTSECRET, { algorithm: 'HS256' })

	if (password.length >= 8) {
	if (password.length < 8) throw new UserInputError('Password must have at least 8 characters')

		if (foundUser) {
		throw new ForbiddenError('Email already taken by another user')

		}
		throw new AuthenticationError('Wrong email/password combination')

Exercise 4 #10

Exercise 4 #10

Conversation

NicolasTsc commented Nov 25, 2020 • edited

roschaefer left a comment • 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

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

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

AlexHTW 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

Choose a reason for hiding this comment

roschaefer left a comment

Choose a reason for hiding this comment

roschaefer Dec 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolasTsc commented Nov 25, 2020 •

edited

roschaefer left a comment •

edited

roschaefer Dec 20, 2020 •

edited