Hotchocolate: Nested Authorized Directive does not evaluate

[drowhunter]

Describe the bug
Give the following schema

public class RootQuery : ObjectType<Query>
{
    descriptor.Field(f => f.GetViewerAsync( default, default, default))
                .Type<UserProfileGraph>()
                .Authorize();  //Getting UserProfileGraph requires Authentication
}

public partial class UserProfileGraph : ObjectGraphType<UserProfile>
{
        protected override void Configure(IObjectTypeDescriptor<UserProfile> descriptor)
        {
            base.Configure(descriptor);

            descriptor.Field<UserProfileGraphModel>(f => f.GetUserClaims(default, default))
                .Type<NonNullType<ListType<ClaimGraph>>>()
                 .Authorize(roles:"Admin" );   // this field requires Admin Policy 
       }
}

To Reproduce
Outer field only requires Authorization
Inner field requires specific Role

Only Outer Policy gets triggered, Inner field is already authorized so it skips authorization check.

Inner field passes security check and allows me to execute it without Admin Policy being checked

Expected behavior
Authorize Policies should stack , or at the very least evaluate always.

[michaelstaib]

.Authorize(roles:"Admin" ); // this field requires Admin Policy

this would not check policy but check if the user has a role claim.

Did you check if the role claim is available in your context data?

[drowhunter]

Update: Issue is bigger than I initially thought

Back to UserProfileGraph this fails to invoke Middleware and it also fails to invoke the Authorization

  descriptor.Field<UserProfileGraphModel>(f => f.GetUserClaims(default, default))
  .Use(next => async context =>
  {
         Console.WriteLine("This Middle ware does not get hit");
         await next(context);
 })
.Authorize(Policies.ADMIN ); //<< this does not get invoked , with or without this line the middleware 
 above does not get hit.

however this works if i don't use descriptor.Field<T>()

descriptor.Field("thisWorks")
                .Argument("claim", arg => arg.Type<StringType>())
                .Resolver(ctx =>
                {
                      // break point wont get hit because Authorize Directive Stops it because i'm not an admin.  This is expected
                }).Type<ListType<ClaimGraph>>()
                .Use<MyCustomMiddleWare>(); 
                .Authorize(Policies.ADMIN);   //<< - this gets hit

Strangely Enough the second one which didnt work when inside of UserProfile
Works when placed at root query level
```public class RootQuery : ObjectType
{
descriptor.Field(f => f.GetViewerAsync( default, default, default))
.Type()
.Authorize(); //Getting UserProfileGraph requires Authentication
}

```

[drowhunter]

Oh and this also works
I added a method into my UserProfile POCO just to test

public class UserProfile 
{
        public string Works()
        {
            return "this will get hit";
        }
}

 descriptor.Field(f => f.Works())
                .Use(next => async context =>
                {
                    Console.WriteLine("This Will be not be hit because of the ADMIN policy below");
                    await next(context);

                })
                .Authorize(Policies.ADMIN);  << i can confirm that this gets hit

[drowhunter]

Conclusion
using descriptor.Field<T>()

breaks Middleware from executing
break Authorization Directives from executing

[drowhunter]

.Authorize(roles:"Admin" ); // this field requires Admin Policy

this would not check policy but check if the user has a role claim.

Did you check if the role claim is available in your context data?

Just responding to your questions. Yes anything under viewer requires Authorization in general. However i plan to have certain fields only accessible via special roles such as Admin.

I have checked the claims and the role is there. And it works in all circumstances where I don't use a ResolverType. When using a resolver type all Directives and Middlewares inline or class based do not trigger. Except when placed on the Root Query for some reason.

[michaelstaib]

I will write some tests and see where this leads to.

[michaelstaib]

So, I wrote a simple test that has three levels with middleware on each field. The field all use the generic field selector.

So, far I cannot see any issue here:

https://github.com/ChilliCream/hotchocolate/commit/75de41d2307e8cfcb1d7fdf6c3be92403e83436f

Can you create a small repro of your issue?

[michaelstaib]

Added directive middleware into the test and it still performs as expected.

https://github.com/ChilliCream/hotchocolate/commit/8ad68e5856cda441fd948863321a01aeef18cc86

I will wait for your repro.

[drowhunter]

Ok i figured out what was wrong. Including descriptor.Include<UserProfileResolverType>(); causes the ResolverFields below not to have middleware or directives applied. Is this a bug ?

I don't even know why I included that line in the first place what does descriptor.include even do?

 public partial class UserProfileGraph : ObjectType<UserProfile>
 {
        protected override void Configure(IObjectTypeDescriptor<UserProfile> descriptor)
        {
            descriptor.Include<UserProfileResolverType>(); <-- THIS IS THE PROBLEM

            descriptor.Field<UserProfileResolverType>(f => f.GetUserClaims(default, default))
                .Authorize(Policies.ADMIN )
                .Use(next => async context =>
                {
                    Console.WriteLine("This Will NOT be Hit");
                    await next(context);
                });
         }
}

[drowhunter]

yeah so in your tests add descriptor.Include<ResolverType>();
before the fields which use

Field<ResolverType>();

and you will see any directives and middleware will be ignored on the fields

[michaelstaib]

@drowhunter can you post UserProfileResolverType?

[drowhunter]

here you go

public class UserProfileResolverType
    {
        [GraphQLNonNullType]
        public async Task<List<UserClaim>> GetUserClaims([Service]IUserProfileService userProfileService, string claim)
        {
            if(claim == null)
                return await userProfileService.GetUserClaims();

            return new List<UserClaim>
            {
                await userProfileService.GetUserClaim(claim)
            };
        }


    }

[michaelstaib]

Why are you including it?

With include you essentially tell the type to infer the field ... then you are trying to declare the same field explicitly. This is why it fails... your declaration is ignored since you included it first.

[michaelstaib]

Remove the include and then it will work.

[michaelstaib]

I will still issue a fix that will remove the previous declared fields when they are declared multiple times.

[michaelstaib]

OK, I will close this issue and create a new one that deals with duplicate field declarations.

[michaelstaib]

1119

[drowhunter]

Yes i realized this when I removed the Include. But it was rather hard to track down, , the fix you are creating in #1119 will fool proof it :)

EDIT: Actually to answer your question above.

I was using include initially because I wanted to include all fields in the ResoverType

but the reason i was declaring it again was that i needed to also add authorization policies to specific fields.

I wasn't aware that by doing this you were actually duplicating the field.

This could have been solved if there was an [GraphQLAuthorizeAttribute] to place on the ResoverType's field declaration