feat: authorization

[ojeytonwilliams]

Very preliminary draft of the authorization implementation.

Key point: all authorization logic is the responsibility of authorizationChecker.

This determines if a given user has a right to see or interact with a given field or resolver. To my knowledge the main weakness of this approach is that it cannot filter a resolver's output. So, if we wanted a public event feed and a private one, those would have to be implemented as separate resolvers.

We cannot (well, should not) put any authorization logic in resolvers, but we can make different queries through the client based on the information about the user (are they logged in, what role do they have etc.).

[gitpod-io]

[ghost]

👇 Click on the image for a new way to code review

• Make big changes easier — review code in small groups of related files

• Know where to start — see the whole change at a glance

• Take a code tour — explore the change with an interactive tour

• Make comments and review — all fully sync’ed with github

  Try it now!

Legend

[ojeytonwilliams]

TODO

• Refactor the tests and fixtures (I think they're correct, but they're unreadable)

[gikf]

A bit of side-question - is it planned to use this as a check if user is logged in? After passing any permission check it is implicit that user is logged in, so in theory some logged-in permission could be used for authorizing access where no more specific permission exists.

[ojeytonwilliams]

@gikf I hadn't planned to, but it's definitely possible if we want to.

What you can do is simply add an @Authorized() decorator with no arguments. The authorization logic would need tweaking slightly, but it's pretty straightforward.

Did you have something in mind where we'd want to do that? If you do, I'll add that logic in.

[ojeytonwilliams]

Also, I might need to drop the variable validation. I thought it would be useful to make sure that a request comes with, say, chapterId, but if the request is malformed it will simply be denied. As such, I don't think there are security implications. However it is quite clunky, since we'd need to add every variable we use (limit, offet and so on).

[gikf]

Nothing specific, just a general idea, that I'm now not sure if it's even needed...

[ojeytonwilliams]

No worries. If we come up with a use case for it, it's easy enough to add in, but let's leave it until we do.

[gikf]

Nitpicking inside, I've tried to unify names a bit and make them a bit more clear with not too much verbosity overhead. Feel free to ignore part or all of these.

[ojeytonwilliams]

Thanks @gikf I like these changes a lot. The only one I'd change is allowingPermissions to requiredPermissions. permissions was definitely too vague, but allowingPermissions doesn't quite sound right to me.

What I'll do is commit all your changes and rename that separately.

[ojeytonwilliams]

@gikf let me know if there's anything else that doesn't look right.

[gikf]

I considered requiredPermissions as well, I've picked allowing for a little more context, as that's how hasNecessaryPermission behaves right now - only one permission needs to match with user permissions.

Otherwise I'm not seeing anything sticking out 👍

[ojeytonwilliams]

Ah, yes, good point.

How about this? We validate that the number of permissions is 1, call it requiredPermission and adjust the logic accordingly. That way it genuinely is a required permission.

After all, the intention is that each resolver would only need a single permission. e.g. the updateEvent resolver would only need the UPDATE_EVENT permission. I'm not (yet) seeing a reason to associate more than one permission with a resolver.

[gikf]

Yeah, that sounds sensible.