Exercise 4 #11

MaykAkifovski · 2020-12-04T16:18:05Z

We have reviewed this PR

… with uuid and userid in token conformant to jwt spec, fixed some api definitions

…i methods

MaykAkifovski · 2020-12-04T16:23:27Z

@Systems-Development-and-Frameworks/countryroads, there is a bug in our PR and we are not able to add you as a Code Reviewer. I have to manually go to our PR and make a code review.

yiimnta · 2020-12-05T17:13:34Z

@Systems-Development-and-Frameworks/countryroads, there is a bug in our PR and we are not able to add you as a Code Reviewer. I have to manually go to our PR and make a code review.

Hi...
when i can review your code, please add me.
if possible, can i review a bit while waiting for fixing your bug?

yiimnta

Review on 12.06.2020 at commit "tests fixed(1f1eb90)"

The idea of using graph-shield to check the login (passwordIsValid) and to check the user has voted or not (isPostUpvoted) is quite good 👍 . And you can create 2 classes Post and User to give the code a more cool look. 😉

✔️ ⭐ 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.

yiimnta · 2020-12-06T18:12:37Z

backend/src/resolvers.js

+        const userById = await dataSources.usersApi.getUserById(parent.author);
        return {
-          name: parent.author
+          name: userById.name
        };


If you return a name like this then it's not possible to nest your queries 'indefinitely':

{ posts { title votes author { name posts { title } } } }

as result, the author's posts will be an empty list.
Here you should return to a User instead of the author's name.

Suggested change

const userById = await dataSources.usersApi.getUserById(parent.author);

return {

name: parent.author

name: userById.name

};

return await dataSources.usersApi.getUserById(parent.author);

That's 100% correct!

yiimnta · 2020-12-06T18:43:09Z

backend/src/tests/tests.test.js

+    it("creates new post - not authenticated", async () => {
        const {
-            data: { write }
-        } = await mutate({ mutation: WRITE_POST, variables: { post: { title: "Title1", author: { name: "Max" } } } });
+            errors:[ error ]
+        } = await mutate({ mutation: WRITE_POST, variables: { post: { title: "Title1"}}});

-        expect(write).toEqual({title: "Title1", author: {name: "Max"}});
+        expect(error.message).toEqual('Not Authorised!');
    });

-    it("throws error if author creates post with already existent title", async () => {    
+    it("creates new post - authenticated", async () => {
+        reqMock.headers = {authorization: 'Bearer ' + atanas_auth_token};
+
+        const {
+            data: {write}
+        } = await mutate({ mutation: WRITE_POST, variables: { post: { title: "Title1"} } });
+
+        expect(write).toEqual({title: "Title1", author: {name: "Atanas"}});
+    });


1/2⭐ For assigning the authenticated user to a post in write mutations.

yiimnta · 2020-12-06T18:45:58Z

backend/src/tests/tests.test.js

+    it("upvotes a post - not authorized", async () => {
+        const {
+            errors:[ error ]
+        } = await mutate({ mutation: UPVOTE_POST, variables: { title: "Atanas's Post 1" } });
+
+        expect(error.message).toEqual('Not Authorised!');
+    });
+
+    it("upvotes a post - authorized", async () => {
+        reqMock.headers = {authorization: 'Bearer ' + atanas_auth_token};
+
        const {
-            data: {upvote}
-        } = await mutate({ mutation: UPVOTE_POST, variables: { title: "The Nothing", voter: { name: "Max" } } });
-        
-        expect(upvote).toMatchObject({ title: "The Nothing", votes: 1 });
+            data:{upvote},
+        } = await mutate({ mutation: UPVOTE_POST, variables: { title: "Atanas's Post 1" } });
+
+        expect(upvote).toMatchObject({ title: "Atanas's Post 1", votes: 1 });
    });


1/2⭐ For assigning the authenticated user to a post in upvote mutations.

yiimnta · 2020-12-06T18:58:48Z

backend/src/resolvers.js

+      async login(parent, args, { dataSources }) {
+        const user = await dataSources.usersApi.getUserByEmail(args.email);
+        const token = await dataSources.authApi.createToken(user.id);
+        return token;
      }


⭐ For an implemented login feature using JWT.

yiimnta · 2020-12-06T18:59:14Z

backend/src/resolvers.js

+      async signup(parent, args, { dataSources }) {
+        const createdUser = await dataSources.usersApi.createUser(args.name, args.email, args.password);
+        const token = await dataSources.authApi.createToken(createdUser.id);
+        return token;
+      },


⭐ For an implemented signup feature.

yiimnta · 2020-12-06T19:19:38Z

backend/src/security/permissions.js

+    Mutation: {
+      "*": deny,
+      signup: and(
+        not(emailIsTaken, new UserInputError("A user with this email already exists.")),


⭐ For unique email address validation.

Interesting, didn't know that not has a second error argument.

yiimnta · 2020-12-06T19:20:06Z

backend/src/security/permissions.js

+      "*": deny,
+      signup: and(
+        not(emailIsTaken, new UserInputError("A user with this email already exists.")),
+        not(passwordIsTooShort, new UserInputError("The password must be at least 8 characters long."))


⭐ For password validation.

yiimnta · 2020-12-06T19:39:25Z

backend/src/tests/tests.test.js

+const context = require('../context');
+const contextPipeline = () => context({ req: reqMock, res: resMock });


you could easily use:

const contextPipeline = () => {}

when you want to authenticate a user, just use:

server.context = () => ({token: {uId: user_id}});

instead of creating these variables:

reqMock = { headers: {} }; resMock = {}; let userIds = [uuidv4(), uuidv4()]; mike_auth_token = await authApi.createToken(userIds[0]); atanas_auth_token = await authApi.createToken(userIds[1]);

Yes, mocking the context is fine and it's how I implemented the tests in my reference solution. However, this is copied from my example code for people who really want to test the requests headers for whatever reason.

yiimnta · 2020-12-06T19:51:17Z

backend/src/tests/tests.test.js

        const GET_POSTS = gql`
            query {


try testing the following query and you will see that the author's posts list will be empty.

const GET_POSTS = gql` query { posts { title author { name posts { title author { name } } } } }`;

roschaefer

Yes, I agree with @yiimnta here. You achieved 7/10 🌠 well done!

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

What got me the most is that you have put so much logic (also data-validations) into the permissions layer.

Regarding the rebase look here:

See my suggestions below.

roschaefer · 2020-12-10T20:52:39Z

backend/src/context.js

+    let token = req.headers.authorization || '';
+    token = token.replace('Bearer ', '');
+    try {
+        const decodedToken = jwt.verify(token, process.env.JWT_SECRET);


I would suggest to import a file here wich require('dotenv') and exports a constant JWT_SECRET. You would avoid a process.env.JWT_SECRET being undefined here, regardless of the order how files are loaded.

roschaefer · 2020-12-10T20:53:35Z

backend/src/datasources/authenticationApi.js

+    }
+
+    async createToken(userId) {
+        return jwt.sign({ uId: userId }, process.env.JWT_SECRET, { algorithm: 'HS256', expiresIn: '1h' });


expiresIn 👍

roschaefer · 2020-12-10T20:57:56Z

backend/src/datasources/userApi.js

-  async getUser(name) {
-    return this.users.find(user => user.name === name);
+  async getUserById(id) {
+    return this.users.find(user => user.id == id);


Suggested change

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

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

backend/src/datasources/userApi.js

roschaefer · 2020-12-10T21:29:48Z

backend/src/tests/tests.test.js

+const context = require('../context');
+const contextPipeline = () => context({ req: reqMock, res: resMock });


Yes, mocking the context is fine and it's how I implemented the tests in my reference solution. However, this is copied from my example code for people who really want to test the requests headers for whatever reason.

backend/src/tests/tests.test.js

Co-authored-by: Robert Schäfer <git@roschaefer.de>

ubiquitousbyte and others added 11 commits December 2, 2020 17:39

added token signing and veryfing, context verification, added userids…

303dc5a

… with uuid and userid in token conformant to jwt spec, fixed some api definitions

fixed signup not generating token

d510c54

implements login

e037939

added rules and permissions, fixed project structure, added proper ap…

de45ca3

…i methods

added data seeding for tests

815f3bf

fixes resolver to return author.name by author.id

cd360a2

test fixed reslover maping

f41eba4

exports context

6f25488

adds Authorisation check for create

3cb603c

fixes Post shield

2e42ac9

tests fixed

1f1eb90

MaykAkifovski requested review from a team, roschaefer and medizinmensch and removed request for a team December 4, 2020 16:18

yiimnta reviewed Dec 6, 2020

View reviewed changes

yiimnta mentioned this pull request Dec 6, 2020

WIP: Exercise 4 Systems-Development-and-Frameworks/CountryRoads#7

Closed

aloparev mentioned this pull request Dec 9, 2020

Exercise 4 wip Systems-Development-and-Frameworks/CountryRoads#8

Merged

roschaefer requested changes Dec 10, 2020

View reviewed changes

MaykAkifovski and others added 2 commits December 14, 2020 15:50

Update backend/src/tests/tests.test.js

cdc9006

Co-authored-by: Robert Schäfer <git@roschaefer.de>

Update backend/src/datasources/userApi.js

039edb4

Co-authored-by: Robert Schäfer <git@roschaefer.de>

MaykAkifovski merged commit f01584a into main Dec 14, 2020

MaykAkifovski deleted the exercise_4 branch December 14, 2020 14:53

MaykAkifovski restored the exercise_4 branch December 14, 2020 15:16

MaykAkifovski deleted the exercise_4 branch January 29, 2021 12:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exercise 4 #11

Exercise 4 #11

MaykAkifovski commented Dec 4, 2020 •

edited

Loading

MaykAkifovski commented Dec 4, 2020

yiimnta commented Dec 5, 2020 •

edited

Loading

yiimnta left a comment

yiimnta Dec 6, 2020

roschaefer Dec 10, 2020

yiimnta Dec 6, 2020

yiimnta Dec 6, 2020

yiimnta Dec 6, 2020

yiimnta Dec 6, 2020

yiimnta Dec 6, 2020

roschaefer Dec 10, 2020

yiimnta Dec 6, 2020

yiimnta Dec 6, 2020

roschaefer Dec 10, 2020

yiimnta Dec 6, 2020

roschaefer left a comment •

edited

Loading

roschaefer Dec 10, 2020

roschaefer Dec 10, 2020

roschaefer Dec 10, 2020

roschaefer Dec 10, 2020

		const context = require('../context');
		const contextPipeline = () => context({ req: reqMock, res: resMock });

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

Exercise 4 #11

Exercise 4 #11

Conversation

MaykAkifovski commented Dec 4, 2020 • edited Loading

MaykAkifovski commented Dec 4, 2020

yiimnta commented Dec 5, 2020 • edited Loading

yiimnta 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

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 • edited Loading

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

MaykAkifovski commented Dec 4, 2020 •

edited

Loading

yiimnta commented Dec 5, 2020 •

edited

Loading

roschaefer left a comment •

edited

Loading