Add some auth tests #26
Comments
|
Collecting ideas:
Was wondering, where is the best place to add the authorization to keep it simple: in the model or in the resolver? Jonas Helfer wrote on GraphQL-Authorizations this post: in ./models/User.js export default class User {
constructor(context) {
this.context = context;
this.collection = context.db.collection('user');
this.pubsub = context.pubsub;
this.loader = new DataLoader(ids => findByIds(this.collection, ids));
this.allows = {
create: (user, doc) => {
// the user must be logged in, and the document must be owned by the user
return user._id && user.role && user.role === 'admin';
},
read: (user, doc) => {
// the user must be logged in, and the document must be owned by the user
return user._id && doc._id.toString() === user._id.toString();
},
update: (user, id, doc, fields, modifier) => {
// the user must be logged in, and the document must be owned by the user
return user._id && id.toString() === user._id.toString();
},
delete: (user, id) => {
// the user must be logged in, and the document must be owned by the user
return user._id && id.toString() === user._id.toString();
}
};
}so referencing this in the resolver instead in the model!?!? const resolvers = {
User: {
id(user) {
return user._id;
},
employer(user, { lastCreatedAt, limit }, { User }) {
return User.employer(user, { lastCreatedAt, limit });
},
addresses(user, { lastCreatedAt, limit }, { User }) {
return User.addresses(user, { lastCreatedAt, limit });
},
},
Query: {
users(root, { lastCreatedAt, limit }, { User, user }) {
//return User.all({ lastCreatedAt, limit });
return User.all({ lastCreatedAt, limit, user })
// do authorization check, according to https://dev-blog.apollodata.com/auth-in-graphql-part-2-c6441bcc4302
.then( lists => {
return lists.filter((doc) => {
return User.allows.read(user, doc);
});
});
},
user(root, { id }, { User, user }) {
// return User.findOneById(id);
return User.findOneById(id)
// do authorization check, according to https://dev-blog.apollodata.com/auth-in-graphql-part-2-c6441bcc4302
.then(doc => {
return User.allows.read(user, doc) ? doc : null;
});
},
},
Mutation: {
async createUser(root, { input }, { User, user }) {
// do authorization check
if (User.allows.create(user, input)){
const id = await User.insert(input);
return User.findOneById(id);
}
return null;
},
async updateUser(root, { id, input }, { User, user }) {
// do authorization check
if (User.allows.update(user, id, input)){
await User.updateById(id, input);
return User.findOneById(id);
}
return null;
},
removeUser(root, { id }, { User, user }) {
// do authorization check
if (User.allows.delete(user, id)){
return User.removeById(id);
}
return null;
},
},
Subscription: {
userCreated: user => user,
userUpdated: user => user,
userRemoved: id => id,
},
};
export default resolvers;What do you think? |
|
Hey, A few thoughts.
Type Post @canEdit('admin', 'owner') @canDelete('admin') {
owner: User @belongsTo # <- this would be required for the `owner` auth to work.
}I can imagine a world where we would want to introduce a whole plugin model to allow different types of auth to be generated, but OTOH, I think people should probably be at least carefully reading the generated code, if not writing it themselves, so I'm inclined to say we just do the minimum to avoid people having to have to just write a lot of boilerplate. |
|
Created a prototype for the authorization. Please have a look in the README.md here: |
|
@tobkle I am pretty happy with the API, not exactly sure about the implementation. Can I make a suggestion? In your branch, edit the Some minor thoughts on what you have so far:
As for the implementation, I would probably add the I would be inclined to steer clear of making things too "library" like. We can generate the code, so we can keep things pretty simple in the implementation. class Post {
isAuthorized(post, mode, user) {
if (mode === 'create') {
return this.context.User.role(user) === 'admin';
}
}Let me know what you think |
|
opened a new branch 'authorization' based on your last master to do what you've suggested: It all starts by defining type files such as in test/input:
Using this to authorize CRUD operations for Tweet: type Tweet
@authorize(
create: ["owner"]
readOne: ["world"]
readMany: ["world"]
update: ["owner", "admin"]
delete: ["owner", "admin"]
ownerField: "author"
)Meaning:
Using this to authorize CRUD operations for User: type User
@authorize(
create: ["admin"]
readOne: ["owner", "admin"]
readMany: ["admin"]
update: ["owner", "admin"]
delete: ["owner", "admin"]
ownerField: "_id"
roleField: "role"
)Meaning:
The role 'admin' itself is just a name of a role, you can use any role name. It gains its meaning only by using this role name in the @authorize directives in the different type definitions. Later this will generate the following target code: export default class User {
constructor(context) {
this.ownerField = '_id'; // @authorize(ownerField: "_id")
this.roleField = 'role'; // @authorize(roleField: "role")
this.context = context;
this.collection = context.db.collection('user');
this.pubsub = context.pubsub;
this.loader = new DataLoader(ids => findByIds(this.collection, ids));
}
// returns the owner of the current document @authorize(ownerField)
owner(doc){
return (doc && doc[this.ownerField]) ? doc[this.ownerField] : null;
}
// returns the role of the current user @authorize(roleField)
role(user){
return (user && user[this.roleField]) ? user[this.roleField] : null;
}
// returns true, if the current user is authorized for the current mode and document
isAuthorized({doc, mode, user, debug = true}){
let authResult = false;
const owner = this.owner(doc);
const role = this.context.User.role(user);
switch (mode) {
case 'create':
// @authorize(create: ["admin"])
authResult = (!!role && role === 'admin');
break;
case 'readOne':
// @authorize(readOne: ["owner", "admin"])
authResult = (!!role && (user._id === owner || role === 'admin'));
break;
case 'readMany':
// @authorize(readMany: ["admin"])
authResult = (!!role && role === 'admin');
break;
case 'update':
// @authorize(update: ["owner", "admin"])
authResult = (!!role && (user._id === owner || role === 'admin'));
break;
case 'delete':
// @authorize(delete: ["owner", "admin"])
authResult = (!!role && (user._id === owner || role === 'admin'));
break;
}
(debug) ? console.log('User', mode, owner, role, "===>", authResult) : null;
return authResult;
}
...Target Code: Resolvers An empty array in an @authorize argument will result into e.g. not allowing delete mutations: case 'delete':
// @authorize(delete: [])
authResult = false;
break; Query: {
async users(root, { lastCreatedAt, limit }, { User, user }) {
const doc = await User.all({ lastCreatedAt, limit });
return User.authorized({doc, mode: 'readMany', user});
},
async user(root, { id }, { User, user }) {
const doc = await User.findOneById(id);
return User.authorized({doc, mode: 'readOne', user});
},
},
Mutation: {
async createUser(root, { input }, { User, user }) {
const doc = input;
const authorized = User.isAuthorized({doc, mode: 'create', user});
if (!authorized) throw new Error('User: mode: create not authorized');
const id = await User.insert(input);
return User.findOneById(id);
},
async updateUser(root, { id, input }, { User, user }) {
const doc = await User.findOneById(id);
const authorized = User.isAuthorized({doc, mode: 'update', user});
if (!authorized) throw new Error('User: mode: update not authorized');
await User.updateById(id, input);
return User.findOneById(id);
},
async removeUser(root, { id }, { User, user }) {
const doc = await User.findOneById(id);
const authorized = User.isAuthorized({doc, mode: 'delete', user});
if (!authorized) throw new Error('User: mode: delete not authorized');
return User.removeById(id);
},
},End-to-end-test: Mutations for the role
Using client authentication for the end-to-end-test:
const payload = {
userId: user._id.toString(),
};to: const payload = {
userId: user._id.$oid.toString(),
};otherwise it would have only So far the test passes. Planned extensions:
Did I get you right with this? |
|
|
Hey @tobkle, apologies for being a little slow to get to this, I will try and look at it today! |
|
@tobkle this is great! I think this is a great way to do it. I have some minor comments about the generated code but perhaps let's start with bigger picture stuff.
type Tweet
@authorize(
owner: ["create", "update", "delete"],
admin: ["update", "delete"],
world: ["read"]
)I guess in general people will have less roles than modes, so this will be briefer? I could go either way, up to you.
If we see a named So for the tweet it would We could also consider making those decorators work at the field level? One other advantage of this approach is you could defined multiple document "owner" fields and give different permissions to each.
type User @authorize(...) {
role: String @authRole;
}These are just suggestions, I'm open to whatever way you want to go with it. It's looking great! |
|
Don't mind. |
|
For verifying your points, let's use an example to check if I got you right... type tweet
@authorize(
author: ["create", "read", "update", "delete"],
coauthors: ["read", "update"],
admin: ["readOne", "readMany", "update", "delete"],
world: ["readOne", "readMany"]
)
{
body String!
author User! @belongsTo
coauthors [User!] @belongsTo
}type user
@authorize(
this: ["create", "readOne", "update", "delete"],
admin: ["create", "read", "update", "delete"]
)
{
username String!
role String! @authorize(this: ["read"], admin: ["read", "update"])
bio String
}So you suggest not to use "roleField" nor "ownerField" in the @authorize directive. Instead, if we have the "author" and "coauthors" role in the @authorize header directive, to check within the type field definitions, if there is a field with this specific name "author" or "coauthors" which has a type "User" or a list of users "[User]".
Can we use "read" to allow both "readOne" and "readMany" at the same time for keeping it short? If we want to handle "readOne" and "readMany" separately, we use them explicitely. So if either one or even both of them appear, then we know, to handle both cases separately. For the field directive, I suggest to use the same roles and modes like in the header, which gives us more control on field level authorization. |
|
...or using as you suggested @authRole on field level authorization instead of @authorize if it more robust while reading the AST. |
|
This looks great. A few quick points: First part looks perfect.
Please! Re: field directive -- is this doing auth for the field? I was talking about an |
|
Ah, now I understand that point. So instead of using in the header @authorize a "roleField" and defining a field name, you suggest to use the field directive @authRole as a "connector" between role and field. Ok, this makes sense. Then the role in the @authorize and the field hosting the role can be different if necessary. -Agreed- Field level authorization: If we don't consider some kind of field authorization, I don't know, how to secure the "role" field in the user type. If I want a default user to be allowed to change some of his fields such as "bio" or "address", I have to grant him an "update" authorization on his own "User" document. But then also the "role" field is open for updates. I don't want him to upgrade his role from "user" to "admin" by himself. This is something, only a user with higher authorization should be allowed to do. So whether we hard code it in the user model (what I have done so far), or we enable some kind of field based authorization directive with some auth logic. What was your intention on this? |
|
Good point! What do you think? We could start by hard-coding it so the Or we could just do field auth now too.. |
|
At the moment I have this logic in the User Model... // returns document without not authorized field
// generated from
// role: String! @authorize(admin: ["read", "update"])
allowedFields(input, user){
if (user.role === 'admin'){
return input;
}
if (input[this.roleField]){
delete input[this.roleField];
}
return input;
}Can answer you this evening (CET) again, have some meetings now.... |
|
We are not far away from the field authorization though, as we have already the document authorization in place. Only the not authorized fields shouldn't be in the document, while doing the create, read, update operations on a specific user and role. There is to distinguish between inbound and outbound operations: Inbound are create and update. Outbound are readOne and readMany. In inbound operations, we are receiving the "input" document in the resolver. There, I would use a function like the above mentioned "allowedFields" function, to remove the fields of the input document, which aren't allowed for the user/role and mode. Then we have already the field level authorization. I wouldn't throw an exception for missing field authorizations. In outbound operations, we are receiving the documents from the dataloader. There we have two alternatives: Alternative 1: Before we query, we construct a field selection object, which holds all requested fields without not authorized fields. Then we use that for the find operation. So we read and return only documents with authorized fields. Alternative 2: We remove the not authorized fields afterwards, meaning, removing the not authorized fields from the found documents, which requires an additional loop over the records. The easiest way is the second alternative, as it is very much like the upper one, but I assume it will have a performance impact. Alternative 1 would be the better approach, but requires a new data access interface for e.g. the dataloader findById and collection find, so the easier and faster is for the moment alternative 2. Or do you see a nice way therefore? We also need to agree upon the field authorization directive, for triggering the generator correctly: ...
username: String!
role: String! @authorize(this: ["read"], admin: ["create", "read", "update"])
...This would mean:
What do you think? |
|
My feeling is this is getting pretty complicated; I would prefer to leave the per-field stuff in a separate PR. I think I am pretty happy with the API, now, it's going to be great! I'm a bit concerned that the generated code is pretty complicated; remembering that an aim of the project is to generate simple, easy to understand code that a user can extend + modify themselves. I can see two ways forward:
One thing I was thinking is rather than doing document checks, can we just alter the queries (and potentially check the output)? For instance, if you are trying to update a tweet, and the permissions are as you have them in the example app, the query could look something like: async updateById(id, doc, _user) {
const query = { _id: id };
if (!loggedIn) {
throw new Error("Can't update");
} else if (!User.isAdmin(user) {
query.$or = { authorId: user._id, coauthorIds: user._id };
}
const success = this.collection.update(query, ...)
if (!success) {
throw new Error("Can't update!");
}
//...
}Then the only abstraction I would make to the above would be to factor out the common "generate query for roles": async updateById(id, doc, _user) {
const baseQuery = {_id: id};
// This line is generated by examining the `@authorize` directive + `@authRole`s
const authQuery = Auth.queryForRoles(user, ['admin'], ['authorId', 'coauthorIds'], { User });
const success = this.collection.update({ ...baseQuery, ...authQuery }, ...);
if (!success) {
throw new Error("Can't update!");
}And the implementation would look something like: Auth.queryForRoles = function(user, userRoles, docRoles, { User }) {
const role = User.authRole(user);
if (userRoles.include(role)) {
return {};
}
const query = {$or: {}};
docRoles.forEach(docRole => query.$or[docRole] = user._id);
return query;
}We could then alter every query we run (fetch/insert/update/delete) to factor in the role query? Or am I missing something? |
|
Thanks for your suggestions. That's much better. Enhanced it a bit. Please have a look on this readme and code. The field authorization is necessary in my opinion. Of course it can be also easily removed, if not required. Avoiding the @authorize directive on field level also skips the field authorization logic. It is not yet "prettier" and "eslint"-ed, can do it, if we stick to this approach. |
|
Thanks @tobkle -- apologies, won't have a chance to look at this properly until later in the week. |
|
Don't mind. Took me also a while to answer...
… Am 19.06.2017 um 05:29 schrieb Tom Coleman ***@***.***>:
Thanks @tobkle -- apologies, won't have a chance to look at this properly until later in the week.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Apologies again that this took so long. I still think the output here is more complicated than it needs to be. A few things:
Rather than: export default class User {
this.auth = {
....
}
getOneById(id, _user = {}, resolver = 'getOneById') {
const authQuery = queryForRoles(_user, this.auth, 'readOne', { User: this.context.User }, {_id: id}, resolver);
return this.loader.load(id, authQuery);
}
}This seems simpler and easier if anything to understand: export default class User {
getOneById(id, _user = {}, resolver = 'getOneById') {
// This means users with role 'admin', or who match the document's `_id` field
const authQuery = queryForRoles(_user, ['admin'], ['_id'], 'readOne', { User: this.context.User }, resolver);
return this.loader.load(id, authQuery);
}
}
If it helps I can send a PR to your README showing what I mean. |
New Authorization VersionSorry for the delay, new version here. 0. New with LogfileNow it writes a nice human-readable logfile for debugging purpose. Please have a look into the example logfile of a full end-to-end-test. It logs all user requests with relevant user information and its graphql query, and adds authorization information. 1. queryForRoles-done- 2. Field authorization-done-
3. DataLoader
Please feel free to add a PR to the README file. |
|
First off, love the logfile idea, that seems super useful. Looking through the readme, some questions:
const restrictedFields = (Object.keys(authQuery).length === 0) ? {} : { role: 0 };It seems to me that we would in general need to do something more complicated here, as
|
|
Oh and 5. What do you think about |
|
And 6 ;) -- why does |
While hardcoding this role and field topics, I was wondering if it makes more sense, to add the authorization logic into data. Meaning, having a few types maybe 2 or 3, dedicated to store the user, userRoles and access to different types, and instead of coding the queryForRoles, do a GraphQL query to those types, to figure out, if an access is allowed. Maybe those types are only generated, if the user does something like: create-graphql-server add-authorization-with-roles or if the @authorize directive appears. But this would require some more query filters. Or do you have security/performance concerns? |
What kind of loader are you thinking here?
My concern would just be more that it makes the whole thing more complicated. I actually think once we put the library functions like |
|
Comments:
Hmm, I am not sure what to think about this actually. What would you expect as a CGS user?
Or are you considering cases like "only the user themselves can update their profile"? I think the complexity of understanding the generated code for this is going to be a security problem, I strongly believe people need to be able to easily understand what the code is doing to ensure they are doing their authorization correctly. As a user of the tool I think I would rather just hand-code the special cases above and make sure I have them right.
I think if we just hard-code disallowed access to the |
|
Ok, I removed the field authorization completely, with this version.. Now it should be much easier and less code. Concerning your points 1 and 2 and 4 and 5:
Concerning 3.:
|
|
I think it's looking great! I don't have any more general changes/comments. To your questions above:
Is it time to make a PR? I can make some line by comments on some minor things, then we can start talking about the code generation! |
|
OK. Had many commits, so I have to merge it first before creating a PR. But first I have to finalize the role thing. As a user of CGS I would expect this field protected as soon as @authorize comes into play. So I suggest to copy the following code into the authorize.js include, to have it available on demand. protectFields(me, authorizedUserRoles, protectedFields, inputObject){
const result = Object.assign({}, inputObject);
const role = this.context.User.authRole(me);
// if user is not allowed to access specific fields...
if (!authorizedUserRoles.includes(role)){
protectedFields.every(protectedField => {
if (result[protectedField]) delete result[protectedField];
});
}
return result;
}In the end it remains only this line in mode/User.js to be added docToUpdate.$set = this.protectFields(me, ['admin'], ['role'], docToUpdate.$set);
I assume you don't want to have the field protection inside like so: type User
...
{
role: String @authRole("admin") @authorizeField(admin: ["update"])
...
} |
|
I was thinking that we'd just hard code the role field on the user model (it can use the |
|
Thinking about it a little bit, it would be quite easy for a user to make a critical mistake if they have to remember to set the |
|
Ok, I agree. So I merge it together this evening (German Time Zone) and create a PR. |
|
Just updated, with your PR comments ... Created new authorization package... Please have a look on. Now, we can start with the generator setup. Some thoughts about:
Roadmap:
(I must admit that I haven't understood https://github.com/benjamn/ast-types, which you've recommended in your code.) |
|
I agree about the auth "plugin" model that we can think about later.. The main thing I am thinking about, and not sure about yet, is that when generating types, the way it's currently implemented there is no "cross-type" communication. For instance when building the tweet type, we don't know anything about the user type. So we will need to be careful. But in theory it all seems possible.
I think maybe we should at first try without two versions (and instead inject the auth code into the base version or strip it out if easier). The reason I say that is because otherwise I am sure the code will go out of sync and bugs will appear.
Yes I have not succeeding in doing this yet either ;) |
No description provided.
The text was updated successfully, but these errors were encountered: