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

added support for argument fields #17

Merged
merged 2 commits into from
Aug 24, 2018
Merged

added support for argument fields #17

merged 2 commits into from
Aug 24, 2018

Conversation

cotzo
Copy link
Contributor

@cotzo cotzo commented Aug 23, 2018

No description provided.

Copy link
Member

@joemcbride joemcbride left a comment

Choose a reason for hiding this comment

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

Would you also mind adding a test for this? I believe this scenario will not currently work with "Schema First" syntax, so the test will have to be written with the "Graph type first" approach.

return;

var fieldType = argumentType.Fields.First(p => p.Name == objectFieldAst.Name);
CheckAuth(objectFieldAst, fieldType, userContext, context, OperationType.Mutation);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of a hard-coded OperationType.Mutation this should be the operationType variable that was matched and assigned previously in the node stack. You can have an input object on queries and subscriptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree. will update once we decide on the validation part

if (argumentType == null)
return;

var fieldType = argumentType.Fields.First(p => p.Name == objectFieldAst.Name);
Copy link
Member

Choose a reason for hiding this comment

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

This should be able to be simplified to:

var fieldType = context.TypeInfo.GetInputType();
CheckAuth(...);

The TypeInfo class does that lookup for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Authorization Metadata is not set on the input type but on the EventStreamFieldType so that's why we need to search for the type of the field

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately in my setup GetInputType() returns EnumerationGraphType instead of EventStreamFieldType

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Posting here my current setup

mutation {
  account_update(account: {
    id: "9b6bb020-0892-44f8-89b9-067000f57d80"
    type: ADMIN
  }) {
    id
  }
}
 FieldAsync<AccountType>(
                "account_update",
                arguments: new QueryArguments(
                    new QueryArgument<NonNullGraphType<AccountInputTypeUpdate>> {Name = "account"}
                ),
                resolve: async context => 
                {
                    return await UpdateObjectAsync(context, accountsRepository, "account");
                });
public class AccountInputTypeUpdate : InputObjectGraphType<Account>
    {
        public AccountInputTypeUpdate()
        {
            Field(x => x.Id);
            Field(x => x.Type, true, typeof(EnumerationGraphType<AccountType>))
                .AuthorizeWith("Admin");
        }
    }

Copy link
Member

Choose a reason for hiding this comment

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

You're right! It is passing that ResolvedType and not the FieldType. Sorry about that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no problem ;) happy that it's solved

@joemcbride joemcbride merged commit 4db05bb into graphql-dotnet:master Aug 24, 2018
@joemcbride
Copy link
Member

Have this published, will take a while for Nuget.org to index it.

https://www.nuget.org/packages/GraphQL.Authorization/1.1.26

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.

2 participants