Auth issue
Shane32 opened this issue · 27 comments
I'm not sure how to reproduce the problem. If I change AuthorizationSample/Schema/Query.cs
public class MySchema : GraphQL.Types.Schema
{
public MySchema(IServiceProvider services) : base(services)
{
Query = services.GetRequiredService<QueryType>();
}
}
public class QueryType : ObjectGraphType
{
public QueryType()
{
this.AuthorizeWithRoles("User");
Field(typeof(StringGraphType), "Hello", resolve: context => "Testing");
}
}and the builder method to
.AddSchema<MySchema>()
.AddGraphTypes()then my sample seems to work....... can you reproduce the problem with the sample app? I'm sure there's a logical answer, if we can only find it.
My guess is that all of this is because my graph types are only created when I first query them. That's why it works for subsequent requests (when there is no cache) and doesn't work at all for cached auth. Can you debug when your QueryType gets created?
It works for me if I put it in a Query. It doesn't in nested types
Can you explain a little more? There shouldn't be any cached authentication (??). The only cache my authorization rule does is cache authorization results, and only on a per-request basis, and only for policy checks. So if it checks for policy "MyPolicy", then the next time it encounters the "MyPolicy" requirement, it knows that auth has already passed. Would that be a problem for you?
I'll write a quick sample with nested types...
I'll try to create a small repo if it still works for you
This seems to work...
using GraphQL;
using GraphQL.Types;
namespace AuthorizationSample.Schema;
public class MySchema : GraphQL.Types.Schema
{
public MySchema(IServiceProvider services) : base(services)
{
Query = services.GetRequiredService<QueryType>();
}
}
public class QueryType : ObjectGraphType
{
public QueryType()
{
Field(typeof(PersonType), "parent", resolve: context => new object());
}
}
public class PersonType : ObjectGraphType
{
public PersonType()
{
this.AuthorizeWithPolicy("MyPolicy");
Field(typeof(StringGraphType), "child", resolve: context => "Hello!");
}
}ok, here is a repo
using System;
using GraphQL;
using GraphQL.AspNetCore3;
using GraphQL.MicrosoftDI;
using GraphQL.SystemTextJson;
using GraphQL.Types;
using Microsoft.AspNetCore.Builder;
using Microsoft.Extensions.DependencyInjection;
namespace Test;
public class TestSchema : Schema
{
public TestSchema(IServiceProvider provider, TestQuery query)
: base(provider)
{
Query = query;
}
}
public class TestQuery : ObjectGraphType
{
public TestQuery()
{
Field<NonNullGraphType<TestRootType>>("test", resolve: context => new { });
}
}
public class TestRootType : ObjectGraphType
{
public TestRootType()
{
this.AuthorizeWithRoles("asd");
Field<StringGraphType>("test", resolve: context => "test");
}
}
public class Program
{
public static void Main(string[] args)
{
var builder = WebApplication.CreateBuilder(args);
builder.Services.AddAuthentication();
builder.Services.AddAuthorization();
builder.Services.AddHttpContextAccessor();
builder.Services.AddGraphQL(gqlBuilder => gqlBuilder
.AddSystemTextJson()
.AddAuthorization()
.AddSchema<TestSchema>()
.AddGraphTypes(typeof(TestSchema).Assembly)
);
var app = builder.Build();
app.UseAuthentication();
app.UseAuthorization();
app.UseGraphQLPlayground("/graphql/playground");
app.UseGraphQL<TestSchema>();
app.Run();
}
}
Ugh it better not be because it's a non-null graph type.... lol
And ... it is....
Well at least I can easily fix it, and write tests for it.
98% test coverage is worth nothing if you don't test the right thing...
Also, you mentioned potential issues using .AuthorizeWithRoles() in a field resolver. What's a safe way of doing this? I can simply wrap it in lock(something) { ... } but maybe there is a better way?
I don't quite understand the scenario in which you would need this, so it's hard to explain correctly. For example, if you use a scoped schema with serial execution, there's nothing to worry about. The point though is that the metadata dictionary that stores authorization information is not thread safe. So if you have a singleton schema and two requests come in at the same time, in theory, they could both write to the dictionary simultaneously. (Or a scoped schema with a parallel execution strategy.)
A lock would be best. I might just write an extension method and then do search & replace - e.g. .AuthorizeWithRolesLocked() which would put a lock on the graph type before adding the roles. This is of course assuming there's no other simultaneous metadata access
something like this
Field<StringGraphType>("myMutationA", resolve: context =>
{
this.AuthorizeWithRoles("roleA");
// mutate
});
Field<StringGraphType>("myMutationB", resolve: context =>
{
this.AuthorizeWithRoles("roleB");
// mutate
});
Full example
using System;
using GraphQL;
using GraphQL.AspNetCore3;
using GraphQL.MicrosoftDI;
using GraphQL.SystemTextJson;
using GraphQL.Types;
using Microsoft.AspNetCore.Builder;
using Microsoft.Extensions.DependencyInjection;
namespace Test;
public class TestSchema : Schema
{
public TestSchema(IServiceProvider provider, TestQuery query)
: base(provider)
{
Query = query;
}
}
public class TestQuery : ObjectGraphType
{
public TestQuery()
{
Field<TestRootType>("test", resolve: context => new { });
}
}
public class TestRootType : ObjectGraphType
{
public TestRootType()
{
// this.AuthorizeWithRoles("roleA");
Field<StringGraphType>("testA", resolve: context => "A");
Field<StringGraphType>("testB", resolve: context =>
{
this.AuthorizeWithRoles("roleB");
return "B";
});
Field<StringGraphType>("testC", resolve: context =>
{
context.FieldDefinition.AuthorizeWithRoles("roleC");
return "C";
});
}
}
public class Program
{
public static void Main(string[] args)
{
var builder = WebApplication.CreateBuilder(args);
builder.Services.AddAuthentication();
builder.Services.AddAuthorization();
builder.Services.AddHttpContextAccessor();
builder.Services.AddGraphQL(gqlBuilder => gqlBuilder
.AddSystemTextJson()
.AddAuthorization()
.AddSchema<TestSchema>()
.AddGraphTypes(typeof(TestSchema).Assembly)
);
var app = builder.Build();
app.UseAuthentication();
app.UseAuthorization();
app.UseGraphQLPlayground("/graphql/playground");
app.UseGraphQL<TestSchema>();
app.Run();
}
}
Here both { test { testB } } and { test { testC } } don't fail for the first request but fail for subsequent requests
If I query testB fist, then testC. testC fails.
If I query testC fist, then testB. Both work (subsequent to both fails).
Based on this it's definitely wrong to use this.AuthorizeWithRoles() in a resolver. context.FieldDefinition.AuthorizeWithRoles() almost works but not for the first request.
In server this.AuthorizeWithRoles() worked as expected in all these cases.
why don't you set the authorization on the field to begin with?
public class TestRootType : ObjectGraphType
{
public TestRootType()
{
this.AuthorizeWithRoles("roleA"); //any requests for TestRootType at all, no matter what field or how it got here, will require roleA
Field<StringGraphType>("testA", resolve: context => "A");
Field<StringGraphType>("testB", resolve: context =>
{
return "B";
}).AuthorizeWithRoles("roleB"); //any requests for field testB will require roleB
Field<StringGraphType>("testC", resolve: context =>
{
return "C";
}).AuthorizeWithRoles("roleC"); // any requests for field testC will require roleC
}
}Didn't check but I think that's right...
First let me fix the bug; I'll publish 2.1.0 in a few minutes; I want to fix something else first. Maybe all your old code will work correctly.
why don't you set the authorization on the field to begin with?
Because I wasn't aware I could do that, lol. That's exactly what I was missing. This works as expected, thanks!
No problem! I released v2.1.0. Let me know if you have any other issues.
Yep, I'll try it after I refactor my fields this.AuthorizeWithPolicy() calls
You will not need to use locks or anything if you have all the authorization rules within the constructors, as I showed above, rather than inside the resolvers.
Most likely my code iterates through a request differently than the server project did, causing a difference in the way it gave a validation response when the rules were being changed during the request. But if they are set ahead of time, both projects should validate the same.
Thanks for bearing with me and helping me test this new project.
I think everything works now. Thank you!
Hey @Shane32 sorry to bother you with unrelated stuff, can you please take a look at graphql-dotnet/graphql-dotnet#3088 and tell me if I'm missing something obvious that already exists?
No problem. Closing this.