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

feat(graphql,auth,#1026): Pass operation name to authorizer #1043

Merged
merged 9 commits into from
Apr 9, 2021

Conversation

mwoelk
Copy link
Contributor

@mwoelk mwoelk commented Apr 2, 2021

Hey Doug,
here's a pull request that adds the primary feature requested in #1026.
I'm looking forward to your feedback.

Contents

  • Updated Authorizer interface to also receive an operationName (optional Argument to keep backwards compatibility)
  • Updated DefaultCrudAuthorizer and AuthorizerOptions to also pass along the operationName argument.
  • Updated AuthorizerFilter, RelationAuthorizerFilter and ModifyRelationAuthorizerFilter to pass the name of the annotated method as operationName to the Authorizer. This name can also be overriden by a new optional argument operationName that was added to those decorators.
  • Used this override on queryMany of the ReadResolver to change the passed name to query to better align with the names of the query service.
  • Updated the auth-example to show new possibilities using the nestjs-query-3 - user. Modified the example so that this user can read and aggregate all todos of all users but only update/delete his own todos
  • Added two new unit tests that check if the operationName parameter gets passed correctly
  • Extended documentation on authorization based on operation names

Unimplemented parts of the feature request

  • Does not pass the user input/args to the authorizer because this would require deeper changes and might overcomplicate things. Might be better to perform such checks using custom validators instead.
  • Authorization on create* methods. This should be the scope of a new issue/PR. Main use case would be to throw an Unauthorized here if a user isn't authorized. The filters returned by the authorizers may not be needed for creation. (But maybe could be checked against the input but this would be more related to validation again).

@coveralls
Copy link

coveralls commented Apr 2, 2021

Pull Request Test Coverage Report for Build 729270224

  • 48 of 48 (100.0%) changed or added relevant lines in 13 files are covered.
  • 17 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-0.04%) to 96.454%

Files with Coverage Reduction New Missed Lines %
packages/query-mongoose/src/query/comparison.builder.ts 1 96.0%
packages/query-mongoose/src/query/filter-query.builder.ts 1 97.78%
packages/query-typegoose/src/query/comparison.builder.ts 1 96.0%
packages/query-typegoose/src/query/filter-query.builder.ts 1 97.78%
packages/query-typegoose/src/services/reference-query.service.ts 6 92.13%
packages/query-typegoose/src/services/typegoose-query-service.ts 7 78.57%
Totals Coverage Status
Change from base Build 712681240: -0.04%
Covered Lines: 4778
Relevant Lines: 4864

💛 - Coveralls

@doug-martin
Copy link
Owner

@mwoelk thank you for the PR!

This makes sense to me, the only thing I question is if the we should be passing the name directly or instead create a new type (maybe AuthorizerContext) that has a field operationName.I think this makes for a more flexible implementation that won't be tied to passing additional params to make the change passive, instead we can just add new fields to the AuthorizerContext as use cases are discovered.

@mwoelk
Copy link
Contributor Author

mwoelk commented Apr 5, 2021

@doug-martin sounds great. Thought about it in the beginning but wasn't sure of any additional parameters but it's definitely the better approach to keep it flexible. I'm going to update the PR today.

@mwoelk
Copy link
Contributor Author

mwoelk commented Apr 5, 2021

@doug-martin Here's a summary of the changes made in the three added commits:

  1. As you proposed, I've refactored the argument into a new type (I've went with AuthorizationContext because AuthorizerContext is already defined for the AuthorizerInterceptor and AuthorizationContext also seems more fitting because it varies for every single authorization). (refactored and updated docs in 81ea148)
  2. I didn't like the way one had to query for each method name if one were to implement e.g. a different logic for read/write methods so I added a few more fields to the AuthorizationContext in 13a4d73. More explanation below.
  3. With the changes made in 2, it only seemed logical to me to add the @AuthorizationFilter in 11be43e to the create methods as well to at least give the user the possibility to throw an UnauthorizedException if we don't want someone to create an entity. Right now any returned filters are unused but it would be illogical to have all the nice Authorizer logic setup and having to write additional logic only to authorize the create methods (e.g. using custom guards / decorators).

AuthorizationContext structure

interface AuthorizationContext {
  /** The name of the method that uses the @AuthorizeFilter decorator */
  operationName: string;

  /** The group this operation belongs to */
  operationGroup: 'read' | 'aggregate' | 'create' | 'update' | 'delete';

  /** If the operation does not modify any entities */
  readonly: boolean;

  /** If the operation can affect multiple entities */
  many: boolean;
}

This way in a custom Authorizer we can easily check different requirements instead of having to repeat a lot of conditions. (see auth example)

e.g. we can perform a simple

if(authContext.readonly) {
...
}

instead of comparing the operation name to all possible read operations such as query, aggregate, find, etc.

AuthorizationContext inference

This context is either passed as an argument to the @AuthorizationFilter or inferred by the methodName / name passed to the @AuthorizationFilter. Inference is done in a way that fits the naming conventions of the auto generated resolvers:

  • if the method name starts with query, find or aggregate, readonly is set to true
  • if the name starts with query or aggregate or ends with many, many is set to true
  • if the method name starts with create, delete, update, aggregate, the operationGroup is set accordingly. Otherwise it's set to read

AuthorizationFilter

The authorization filter can be used in three ways:

  • Without arguments @AuthorizationFilter(), infers the context from the name of the decorated method
  • With a name as argument @AuthorizationFilter('queryCompletedTodoItems'), infers the context using the provided name
  • With an AuthorizationContext as argument:
    @AuthorizerFilter({
      operationName: 'completedTodoItems',
      operationGroup: 'read',
      readonly: true,
      many: true
    })

Copy link
Owner

@doug-martin doug-martin left a comment

Choose a reason for hiding this comment

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

Wow! Great job diving in and figuring this out so quickly. I had a few comments let me know what you think.

Thank you again for your contribution, this is awesome!

packages/query-graphql/src/auth/authorizer.ts Outdated Show resolved Hide resolved
packages/query-graphql/src/auth/authorizer.ts Outdated Show resolved Hide resolved
packages/query-graphql/src/resolvers/read.resolver.ts Outdated Show resolved Hide resolved
@doug-martin
Copy link
Owner

@mwoelk the proxy branch looks really good. The two things that stuck out to me after a first pass is the many option in the DeleteResolver#deleteMany and adding an example of using the authorizationContext.

Once you make those changes I'll do one final pass merge and release! Thanks again!

@mwoelk
Copy link
Contributor Author

mwoelk commented Apr 8, 2021

@doug-martin Good catch with the wrong attribute on the deleteMany context (fixed in 9306525). Regarding the examples what kind of example do you mean? I added a whole section to the docs and added some example usage to the auth-example in the SubTaskAuthorizer and in the TodoItemDTO

Should I create a whole new example project based on the auth example that covers different use cases of the authContext?

@doug-martin doug-martin changed the base branch from master to v0.26.0-rc April 9, 2021 00:46
@doug-martin doug-martin merged commit a695287 into doug-martin:v0.26.0-rc Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants