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

Exercise 4 #11

Merged
merged 13 commits into from
Dec 14, 2020
597 changes: 566 additions & 31 deletions backend/package-lock.json

Large diffs are not rendered by default.

8 changes: 7 additions & 1 deletion backend/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,13 @@
"apollo-datasource-rest": "^0.9.5",
"apollo-server": "^2.19.0",
"apollo-server-testing": "^2.19.0",
"graphql": "^15.4.0"
"bcrypt": "^5.0.0",
"dotenv": "^8.2.0",
"graphql": "^15.4.0",
"graphql-middleware": "^4.0.2",
"graphql-shield": "^7.4.2",
"jsonwebtoken": "^8.5.1",
"uuid": "^8.3.1"
},
"devDependencies": {
"eslint": "^7.13.0",
Expand Down
13 changes: 13 additions & 0 deletions backend/src/context.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
const jwt = require('jsonwebtoken');

module.exports = ({ req }) => {
let token = req.headers.authorization || '';
token = token.replace('Bearer ', '');
try {
const decodedToken = jwt.verify(token, process.env.JWT_SECRET);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

return { token: decodedToken }
}
catch (e) {
return {}
}
};
16 changes: 16 additions & 0 deletions backend/src/datasources/authenticationApi.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
const { RESTDataSource } = require('apollo-datasource-rest');
const jwt = require('jsonwebtoken');

class AuthAPI extends RESTDataSource {

constructor() {
super();
}

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

Choose a reason for hiding this comment

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

expiresIn 👍

}

}

module.exports = AuthAPI;
15 changes: 1 addition & 14 deletions backend/src/datasources/postApi.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,7 @@ class PostsAPI extends RESTDataSource {

constructor() {
super();
this.posts = [
{
title: 'The Thing',
votes: 0,
author: 'Peter',
upvoters:[]
},
{
title: 'The Nothing',
votes: 0,
author: 'Peter',
upvoters:[]
},
];
this.posts = [];
}

async getPost(title) {
Expand Down
33 changes: 21 additions & 12 deletions backend/src/datasources/userApi.js
Original file line number Diff line number Diff line change
@@ -1,27 +1,36 @@
const { RESTDataSource } = require('apollo-datasource-rest');
const { v4: uuidv4 } = require('uuid');
const bcrypt = require('bcrypt');

class UsersAPI extends RESTDataSource {
constructor() {
super();
this.users = [
{
name: 'Peter',
posts: [],
},
{
name: 'Max',
posts: [],
},
];
this.users = [];
}

async getUser(name) {
return this.users.find(user => user.name === name);
async getUserById(id) {
return this.users.find(user => user.id == id);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

async getUsers() {
return this.users;
}

async getUserByEmail(email) {
return this.users.find(user => user.email == email);
MaykAkifovski marked this conversation as resolved.
Show resolved Hide resolved
}

async createUser(name, email, password) {
let obj = {id: uuidv4(), name: name, email: email, posts: [], password: await this.hashPassword(password)};
MaykAkifovski marked this conversation as resolved.
Show resolved Hide resolved
this.users.push(obj);
return obj;
}

async hashPassword(password) {
const saltRounds = parseInt(process.env.SALT_ROUNDS);
Copy link
Contributor

Choose a reason for hiding this comment

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

Make that a constant

return bcrypt.hash(password, saltRounds);
}

}

module.exports = UsersAPI;
31 changes: 23 additions & 8 deletions backend/src/index.js
Original file line number Diff line number Diff line change
@@ -1,26 +1,41 @@
const { ApolloServer } = require('apollo-server');
const typeDefs = require('./typeDefs');
const resolvers = require('./resolvers');
const permissions = require('./security/permissions');
const UsersAPI = require('./datasources/userApi');
const PostsAPI = require('./datasources/postApi');
const AuthAPI = require('./datasources/authenticationApi');
require('dotenv').config();
const { applyMiddleware } = require('graphql-middleware');
const { makeExecutableSchema } = require('graphql-tools');


// DO NOT initialize the endpoint inside the dataSources function!
// See https://github.com/apollographql/apollo-server/issues/3150
// Alternatively, set schema.polling.enable to false. (Haven't tested if this works tho)
const usersApi = new UsersAPI();
const postsApi = new PostsAPI();
const authApi = new AuthAPI();

const context = require('./context');

const schema = applyMiddleware(
makeExecutableSchema({
typeDefs,
resolvers,
}),
permissions,
);

const server = new ApolloServer({
typeDefs,
resolvers,
schema,
context: ({req}) => context({req}),
dataSources: () => {
return {
usersApi: usersApi,
postsApi: postsApi
postsApi: postsApi,
authApi: authApi,
}
}
});


server.listen().then(({url}) => {
console.log(`Server ready at ${url}`);
})
});
63 changes: 21 additions & 42 deletions backend/src/resolvers.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
const { UserInputError } = require('apollo-server');

module.exports = {
Query: {
async posts(parent, args, { dataSources }) {
Expand All @@ -11,57 +9,38 @@ module.exports = {
},
User: {
async posts(parent, args , { dataSources }) {
const posts = await dataSources.postsApi.getPosts();
return posts.filter(post => post.author === parent.name);
let posts = await dataSources.postsApi.getPosts();
return posts.filter(post => post.author === parent.id);
}
},
Post: {
author(parent) {
async author(parent, args , { dataSources }) {
const userById = await dataSources.usersApi.getUserById(parent.author);
return {
name: parent.author
name: userById.name
};
Comment on lines +18 to 21
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

That's 100% correct!

}
},
Mutation:{
async write(parent, args, { dataSources }) {
Mutation: {
async write(parent, args, { token, dataSources }) {
let {
title,
author: {name}
} = args.post;

// Check if a post with the title already exists.
let persistedPost = await dataSources.postsApi.getPost(title);
if (persistedPost !== undefined) {
throw new UserInputError("Post with this title already exists.", {invalidArgs: [title]});
}

// Check if the author exists
let author = await dataSources.usersApi.getUser(name);
if (author === undefined) {
throw new UserInputError("No such author.", {invalidArgs: [author]});
}

return await dataSources.postsApi.createPost({title, author: name});
} = args.post;
return await dataSources.postsApi.createPost({ title, author: token.uId });
},
async upvote(parent, args, { dataSources }) {
// Why must we mock the current user? We have him right here, in the voter field?

async upvote(parent, args, { token, dataSources }) {
let postToUpvote = await dataSources.postsApi.getPost(args.title);
if (postToUpvote === undefined) {
throw new UserInputError("Post with this title doesn't exist", {invalidArgs: [args.title]});
}

let upvoter = await dataSources.usersApi.getUser(args.voter.name);
if (upvoter === undefined) {
throw new UserInputError("No such voter.", {invalidArgs: [args.voter]});
}

let alreadyVoted = postToUpvote.upvoters.includes(args.voter.name);
if (alreadyVoted) {
throw new UserInputError("This voter has already upvoted this article", {invalidArgs: [args.title, args.voter]});
}

return await dataSources.postsApi.upvotePost(postToUpvote, args.voter.name);
return await dataSources.postsApi.upvotePost(postToUpvote, token.uId);
},
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;
},
Comment on lines +35 to +39
Copy link

Choose a reason for hiding this comment

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

⭐ For an implemented signup feature.

async login(parent, args, { dataSources }) {
const user = await dataSources.usersApi.getUserByEmail(args.email);
const token = await dataSources.authApi.createToken(user.id);
return token;
}
Comment on lines +40 to 44
Copy link

Choose a reason for hiding this comment

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

⭐ For an implemented login feature using JWT.

}
};
43 changes: 43 additions & 0 deletions backend/src/security/permissions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
const { isAuthenticated, isPostUpvoted, isPostWithTitlePresent, passwordIsValid, passwordIsTooShort, emailIsTaken, canSeeEmail} = require("./rules");
const { shield, and, not, deny, allow, chain} = require('graphql-shield');
const { UserInputError } = require('apollo-server');

const permissions = shield({
Query: {
"*": allow,
},
User: {
"*": deny,
name: allow,
posts: allow,
email: and(isAuthenticated, canSeeEmail),
Copy link
Contributor

Choose a reason for hiding this comment

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

You got a 🚀

🚀 For authorizing read access on User.email:
Allows access only if email belongs to authenticated user.

},
Post: {
"*": allow,
},
Mutation: {
"*": deny,
signup: and(
not(emailIsTaken, new UserInputError("A user with this email already exists.")),
Copy link

Choose a reason for hiding this comment

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

⭐ For unique email address validation.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

not(passwordIsTooShort, new UserInputError("The password must be at least 8 characters long."))
Copy link

Choose a reason for hiding this comment

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

⭐ For password validation.

),
login: chain(
// This may look strange, but if an already authenticated user authenticates (logs in) a second time, she'll have two tokens! This is not good practice.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this a problem? 🤔

// There are two options: either implement a cache layer for the tokens, or just disallow authenticated users to login a second time.
not(isAuthenticated, new Error("Already logged in. Redirect to home page.")),
Copy link
Contributor

Choose a reason for hiding this comment

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

You would implement a redirect in the frontend, not the backend.

not(passwordIsTooShort, new UserInputError("The password must be at least 8 characters long.")),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
not(passwordIsTooShort, new UserInputError("The password must be at least 8 characters long.")),

That shouldn't be necessary on login 🤔

passwordIsValid,
),
write: and(
isAuthenticated,
not(isPostWithTitlePresent, new UserInputError("Post with this title already exists."))
Copy link
Contributor

Choose a reason for hiding this comment

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

From my point of view, data validations like this shouldn't be part of your permission matrix.

),
upvote: and(
isAuthenticated,
isPostWithTitlePresent,
not(isPostUpvoted, new UserInputError("You've already upvoted this post"))
)
}
});

module.exports = permissions;
67 changes: 67 additions & 0 deletions backend/src/security/rules.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
const { rule } = require('graphql-shield');
const bcrypt = require('bcrypt');


const isAuthenticated = rule({ cache: 'contextual' })(
async (parent, args, { token, dataSources }) => {
const user = await dataSources.usersApi.getUserById(token.uId);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

  • Specific to our use-case: Make sure a JWT is only valid if a corresponding user in the database exists. The user might have been deleted or disabled since the JWT was issued.

return user !== undefined;
}
);

const canSeeEmail = rule({ cache: 'strict' })(
Copy link
Contributor

Choose a reason for hiding this comment

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

You could call this isMe or isAuthenticatedUser and re-use that for fields other than email.

async (parent, args, { token }) => {
return token.uId === parent.id;
}
);

const emailIsTaken = rule({ cache: 'strict' })(
async(parent, args, { token, dataSources }) => {
const user = await dataSources.usersApi.getUserByEmail(args.email);
return user !== undefined;
}
);

const passwordIsTooShort = rule({ cache: 'strict' })(
Copy link
Contributor

Choose a reason for hiding this comment

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

I consider this as data-validation, so it shouldn't be part of permission middleware.

async(parent, args, { token, dataSources }) => {
return args.password.length < 8;
}
);

const passwordIsValid = rule({ cache: 'strict' })(
Copy link
Contributor

Choose a reason for hiding this comment

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

Also this logic should go into the login resolver itself.

async (parent, args, { dataSources }) => {
const user = await dataSources.usersApi.getUserByEmail(args.email);
return user !== undefined && bcrypt.compareSync(args.password, user.password);
}
);

const isPostWithTitlePresent = rule({ cache: 'strict'})(
async (parent, args, { dataSources }) => {
let title;
if (args.post === undefined) {
title = args.title;
}
else {
title = args.post.title
}
Comment on lines +41 to +46
Copy link
Contributor

Choose a reason for hiding this comment

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

You have this if-clause because you re-use this rule for different mutations with different args. I think this is sub-optimal, better move this logic into the resolver.


const post = await dataSources.postsApi.getPost(title);
return post !== undefined;
}
);

const isPostUpvoted = rule({ cache: 'strict'})(
Copy link
Contributor

Choose a reason for hiding this comment

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

This code should be as close to the Post class as possible, so this gets checked not only during a GraphQL request execution

async (parent, args, { dataSources, token }) => {
const title = args.title;
const post = await dataSources.postsApi.getPost(title);
return post.upvoters.includes(token.uId);
}
);

exports.isAuthenticated = isAuthenticated;
exports.isPostUpvoted = isPostUpvoted;
exports.isPostWithTitlePresent = isPostWithTitlePresent;
exports.passwordIsValid = passwordIsValid;
exports.passwordIsTooShort = passwordIsTooShort;
exports.emailIsTaken = emailIsTaken;
exports.canSeeEmail = canSeeEmail;
Comment on lines +61 to +67
Copy link
Contributor

Choose a reason for hiding this comment

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

Use ES6 imports or exports = { isAuthenticated, isPostUpvoted, isPostWithTitlePresent, /* ... */ }