Authorization PR #39
Authorization PR #39
Conversation
Thoughts here: 1. We only need one authorized loader (for the `readOne` case). 2. We can determine the `authQuery` at initialization time, assuming that a single user will be used for all queries (this is the case). 3. We can have the user at initialization time, although this will require refactoring the way the context is initialized here: https://github.com/tobkle/create-graphql-server/blob/master/skel/server/index.js i. We need to add the models to the context *after* the authentication, i.e. here: https://github.com/tobkle/create-graphql-server/blob/master/skel/server/index.js#L44 ii. This will mean that we need a different reference to the mongo collection in the `passport.authenticate` call. But we can make that work I think, it doesn't really need the proper user collection.
Simplify authorized loader.
This reverts commit d5a4c32.
This reverts commit d5a4c32.
|
Did the change. First prototype of the adjusted generator is running. Tests are passing. Except for the yarn.lock on CircleCI. Had it run with npm instead of yarn the last time, as yarn was causing problems in my local environment. Yarn hadn't got any network connect. Haven't had the time yet to solve that. Can you give me some comments about the way the generator is implemented? Reading the AST is not so well done yet. Also the code replacement in the model file, but it was the easiest first approach. I should find some time to continue on the weekend. |
generate/authorize.js
Outdated
| roleArgument.name && | ||
| roleArgument.name.kind === NAME && | ||
| roleArgument.name.value && | ||
| roleArgument.name.value !== ''){ |
There was a problem hiding this comment.
In general do we need to be this defensive with this AST reading code? Aren't there certain guarantees about what comes out of graphql?
There was a problem hiding this comment.
It makes this code quite hard to read having all these null checks.
| const that = this; | ||
| try { | ||
| generateAuthCodeModeReadOne | ||
| } catch (err) { log.error(err.message); } |
There was a problem hiding this comment.
I thought we were going to get rid of these try/catch blocks?
generate/authorize.js
Outdated
| const LIST_VALUE = 'ListValue'; | ||
| const DIRECTIVE = 'Directive'; | ||
| const ARGUMENT = 'Argument'; | ||
| const NAME = 'Name'; |
There was a problem hiding this comment.
I'm pretty sure you can get these constants out of the graphql package.
generate/authorize.js
Outdated
| that.authorizedLoader = new DataLoader(ids => findByIds(this.collection, ids, authQuery));`; | ||
| } else { | ||
| // User has to come from this.context.User | ||
| generatedCode = `const { me, User } = context; |
There was a problem hiding this comment.
Did you try embedding the code in some templates?
I know it's a bit clunky but it seems like it's easier to modify later if it's not buried inside a big file like this.
Also, it's a bit inconsistent to do it differently here.
I'd be cool with a different way of doing templating (maybe with proper string interpolation, or using handlebars or something?) if you wanted though.
There was a problem hiding this comment.
added handlebars templating for models and resolvers, allows also to treat the user type differently then the other types. so the additional coding with bcrypt and password don't have to be entered manually any more.
|
Have now two active branches master (this) and another one, where I've integrated both, Authorizations and Query Arguments. |
|
Sorry I am lagging so much on this, crunch time for me right now. |
|
Don't mind. Whenever you have the time. Have learnt a lot on this project so far. Thanks for your work. |
|
Wow, this is looking really amazing |
There was a problem hiding this comment.
Just a couple of smaller things (well the onAuthRegisterLoader thing might be annoying, not sure).
The code generation stuff is unsurprisingly pretty complex now but I think it's an improvement on what came before.
I think you still need to run prettier/linting over the code, but it's almost ready!
| ['authorId', 'coauthorsIds'], | ||
| { User }, | ||
| onAuthRegisterLoader('tweet findOneById', 'readOne', me, this) | ||
| ); |
There was a problem hiding this comment.
I don't understand this part -- why does it need to be async (or callback-based)?
Is it because onAuthRegisterLoader "doesn't throw exception in error case"? What is the error case? Why is it legitimate?
| return this.loader.load(id); | ||
| async findOneById(id, me, resolver) { | ||
| const log = authlog(resolver, 'readOne', me); | ||
| if (!this.authorizedLoader) { |
There was a problem hiding this comment.
I don't understand why it would ever be allowed that you do not have an authorizedLoader.
| inputField = field; | ||
| // take field as it was entered | ||
| CreateInputField = field; | ||
| // on updates, on NonNullable entered fields, the user should be able to decide, if he wants to update |
There was a problem hiding this comment.
Best not to use gender specific language--make it eg. "the user should be able to decide, if they want to update"
No description provided.