diff --git a/CHANGELOG.md b/CHANGELOG.md index 52de5824938..27e949c0731 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Introduced auto-stitching capabilities with the new `StitchingBuilder`. - _GraphQL_ _Voyager_. Special thanks to [@drowhunter](https://github.com/drowhunter) who contributed the middleware. +### Changed + +- The authoization directive is now more aligned how the authorize attribute in ASP.net works. + ### Fixed - Introspection default values are now serialized correctly. diff --git a/examples/AspNetClassic.StarWars/Startup.cs b/examples/AspNetClassic.StarWars/Startup.cs index 3dd84b4973f..7e060ad97d8 100644 --- a/examples/AspNetClassic.StarWars/Startup.cs +++ b/examples/AspNetClassic.StarWars/Startup.cs @@ -1,6 +1,7 @@ using System; using HotChocolate; using HotChocolate.AspNetClassic; +using HotChocolate.AspNetClassic.Voyager; using HotChocolate.Execution.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Owin; diff --git a/src/Core/Abstractions.Tests/Abstractions.Tests.csproj b/src/Core/Abstractions.Tests/Abstractions.Tests.csproj index fba33d5f83f..69f928f11c1 100644 --- a/src/Core/Abstractions.Tests/Abstractions.Tests.csproj +++ b/src/Core/Abstractions.Tests/Abstractions.Tests.csproj @@ -36,4 +36,8 @@ + + + + diff --git a/src/Core/Abstractions.Tests/Execution/QueryRequestBuilderTests.cs b/src/Core/Abstractions.Tests/Execution/QueryRequestBuilderTests.cs index 5bad5d2b1ab..3cb5394ae80 100644 --- a/src/Core/Abstractions.Tests/Execution/QueryRequestBuilderTests.cs +++ b/src/Core/Abstractions.Tests/Execution/QueryRequestBuilderTests.cs @@ -90,6 +90,39 @@ public void BuildRequest_QueryAndSetVariables_RequestIsCreated() request.MatchSnapshot(); } + [Fact] + public void BuildRequest_QueryAndSetVariable_RequestIsCreated() + { + // arrange + // act + IReadOnlyQueryRequest request = + QueryRequestBuilder.New() + .SetQuery("{ foo }") + .AddVariableValue("one", "foo") + .SetVariableValue("one", "bar") + .Create(); + + // assert + // one should be bar + request.MatchSnapshot(); + } + + [Fact] + public void BuildRequest_QueryAndSetNewVariable_RequestIsCreated() + { + // arrange + // act + IReadOnlyQueryRequest request = + QueryRequestBuilder.New() + .SetQuery("{ foo }") + .SetVariableValue("one", "bar") + .Create(); + + // assert + // one should be bar + request.MatchSnapshot(); + } + [Fact] public void BuildRequest_QueryAndResetVariables_RequestIsCreated() { @@ -141,7 +174,40 @@ public void BuildRequest_QueryAndSetProperties_RequestIsCreated() .Create(); // assert - // only three should be in the request + // only three should exist + request.MatchSnapshot(); + } + + [Fact] + public void BuildRequest_QueryAndSetProperty_RequestIsCreated() + { + // arrange + // act + IReadOnlyQueryRequest request = + QueryRequestBuilder.New() + .SetQuery("{ foo }") + .AddProperty("one", "foo") + .SetProperty("one", "bar") + .Create(); + + // assert + // one should be bar + request.MatchSnapshot(); + } + + [Fact] + public void BuildRequest_QueryAndSetNewProperty_RequestIsCreated() + { + // arrange + // act + IReadOnlyQueryRequest request = + QueryRequestBuilder.New() + .SetQuery("{ foo }") + .SetProperty("one", "bar") + .Create(); + + // assert + // one should be bar request.MatchSnapshot(); } diff --git a/src/Core/Abstractions.Tests/Execution/__snapshots__/QueryRequestBuilderTests.BuildRequest_QueryAndSetNewProperty_RequestIsCreated.snap b/src/Core/Abstractions.Tests/Execution/__snapshots__/QueryRequestBuilderTests.BuildRequest_QueryAndSetNewProperty_RequestIsCreated.snap new file mode 100644 index 00000000000..af723edc610 --- /dev/null +++ b/src/Core/Abstractions.Tests/Execution/__snapshots__/QueryRequestBuilderTests.BuildRequest_QueryAndSetNewProperty_RequestIsCreated.snap @@ -0,0 +1,10 @@ +{ + "Query": "{ foo }", + "OperationName": null, + "VariableValues": null, + "InitialValue": null, + "Properties": { + "one": "bar" + }, + "Services": null +} diff --git a/src/Core/Abstractions.Tests/Execution/__snapshots__/QueryRequestBuilderTests.BuildRequest_QueryAndSetNewVariable_RequestIsCreated.snap b/src/Core/Abstractions.Tests/Execution/__snapshots__/QueryRequestBuilderTests.BuildRequest_QueryAndSetNewVariable_RequestIsCreated.snap new file mode 100644 index 00000000000..26e7d3283fa --- /dev/null +++ b/src/Core/Abstractions.Tests/Execution/__snapshots__/QueryRequestBuilderTests.BuildRequest_QueryAndSetNewVariable_RequestIsCreated.snap @@ -0,0 +1,10 @@ +{ + "Query": "{ foo }", + "OperationName": null, + "VariableValues": { + "one": "bar" + }, + "InitialValue": null, + "Properties": null, + "Services": null +} diff --git a/src/Core/Abstractions.Tests/Execution/__snapshots__/QueryRequestBuilderTests.BuildRequest_QueryAndSetProperty_RequestIsCreated.snap b/src/Core/Abstractions.Tests/Execution/__snapshots__/QueryRequestBuilderTests.BuildRequest_QueryAndSetProperty_RequestIsCreated.snap new file mode 100644 index 00000000000..af723edc610 --- /dev/null +++ b/src/Core/Abstractions.Tests/Execution/__snapshots__/QueryRequestBuilderTests.BuildRequest_QueryAndSetProperty_RequestIsCreated.snap @@ -0,0 +1,10 @@ +{ + "Query": "{ foo }", + "OperationName": null, + "VariableValues": null, + "InitialValue": null, + "Properties": { + "one": "bar" + }, + "Services": null +} diff --git a/src/Core/Abstractions.Tests/Execution/__snapshots__/QueryRequestBuilderTests.BuildRequest_QueryAndSetVariable_RequestIsCreated.snap b/src/Core/Abstractions.Tests/Execution/__snapshots__/QueryRequestBuilderTests.BuildRequest_QueryAndSetVariable_RequestIsCreated.snap new file mode 100644 index 00000000000..26e7d3283fa --- /dev/null +++ b/src/Core/Abstractions.Tests/Execution/__snapshots__/QueryRequestBuilderTests.BuildRequest_QueryAndSetVariable_RequestIsCreated.snap @@ -0,0 +1,10 @@ +{ + "Query": "{ foo }", + "OperationName": null, + "VariableValues": { + "one": "bar" + }, + "InitialValue": null, + "Properties": null, + "Services": null +} diff --git a/src/Core/Abstractions/Execution/IQueryRequestBuilder.cs b/src/Core/Abstractions/Execution/IQueryRequestBuilder.cs index a081bb35bda..d9ff9fc8f62 100644 --- a/src/Core/Abstractions/Execution/IQueryRequestBuilder.cs +++ b/src/Core/Abstractions/Execution/IQueryRequestBuilder.cs @@ -14,12 +14,16 @@ IQueryRequestBuilder SetVariableValues( IDictionary variableValues); IQueryRequestBuilder AddVariableValue( string name, object value); + IQueryRequestBuilder SetVariableValue( + string name, object value); IQueryRequestBuilder SetInitialValue( object initialValue); IQueryRequestBuilder SetProperties( IDictionary properties); IQueryRequestBuilder AddProperty( string name, object value); + IQueryRequestBuilder SetProperty( + string name, object value); IQueryRequestBuilder SetServices( IServiceProvider services); IReadOnlyQueryRequest Create(); diff --git a/src/Core/Abstractions/Execution/QueryRequestBuilder.cs b/src/Core/Abstractions/Execution/QueryRequestBuilder.cs index 972983303b4..f17ef8664bf 100644 --- a/src/Core/Abstractions/Execution/QueryRequestBuilder.cs +++ b/src/Core/Abstractions/Execution/QueryRequestBuilder.cs @@ -53,6 +53,17 @@ public IQueryRequestBuilder SetVariableValues( return this; } + public IQueryRequestBuilder SetVariableValue(string name, object value) + { + if (_variableValues == null) + { + _variableValues = new Dictionary(); + } + + _variableValues[name] = value; + return this; + } + public IQueryRequestBuilder AddVariableValue( string name, object value) { @@ -72,6 +83,17 @@ public IQueryRequestBuilder SetProperties( return this; } + public IQueryRequestBuilder SetProperty(string name, object value) + { + if (_properties == null) + { + _properties = new Dictionary(); + } + + _properties[name] = value; + return this; + } + public IQueryRequestBuilder AddProperty( string name, object value) { diff --git a/src/Server/AspNetCore.Authorization/AspNetCore.Authorization.csproj b/src/Server/AspNetCore.Authorization/AspNetCore.Authorization.csproj index 625869fd754..a15a6b22cda 100644 --- a/src/Server/AspNetCore.Authorization/AspNetCore.Authorization.csproj +++ b/src/Server/AspNetCore.Authorization/AspNetCore.Authorization.csproj @@ -25,6 +25,7 @@ + diff --git a/src/Server/AspNetCore.Authorization/AuthorizeDirectiveType.cs b/src/Server/AspNetCore.Authorization/AuthorizeDirectiveType.cs index 8987b6e5f9f..75a8d134cd2 100644 --- a/src/Server/AspNetCore.Authorization/AuthorizeDirectiveType.cs +++ b/src/Server/AspNetCore.Authorization/AuthorizeDirectiveType.cs @@ -1,4 +1,5 @@ -using System.Collections.Generic; +using System; +using System.Collections.Generic; using System.Security.Claims; using System.Security.Principal; using System.Threading.Tasks; @@ -10,6 +11,7 @@ namespace HotChocolate.AspNetClassic.Authorization #else using Microsoft.AspNetCore.Authorization; +using Microsoft.Extensions.DependencyInjection; namespace HotChocolate.AspNetCore.Authorization #endif @@ -38,10 +40,18 @@ private static async Task AuthorizeAsync( AuthorizeDirective directive = context.Directive .ToObject(); - ClaimsPrincipal principal = context - .CustomProperty(nameof(ClaimsPrincipal)); + ClaimsPrincipal principal = null; + var allowed = false; - var allowed = IsInRoles(principal, directive.Roles); + if (context.ContextData.TryGetValue( + nameof(ClaimsPrincipal), out var o) + && o is ClaimsPrincipal p) + { + principal = p; + allowed = p.Identity.IsAuthenticated; + } + + allowed = allowed && IsInRoles(principal, directive.Roles); #if !ASPNETCLASSIC if (allowed && NeedsPolicyValidation(directive)) @@ -57,11 +67,13 @@ private static async Task AuthorizeAsync( } else if (context.Result == null) { - context.Result = QueryError.CreateFieldError( - "The current user is not authorized to " + - "access this resource.", - context.Path, - context.FieldSelection); + context.Result = ErrorBuilder.New() + .SetMessage( + "The current user is not authorized to " + + "access this resource.") + .SetPath(context.Path) + .AddLocation(context.FieldSelection) + .Build(); } } @@ -96,10 +108,16 @@ private static async Task AuthorizeWithPolicyAsync( AuthorizeDirective directive, ClaimsPrincipal principal) { - IAuthorizationService authorizeService = context - .Service(); - IAuthorizationPolicyProvider policyProvider = context - .Service(); + IServiceProvider services = context.Service(); + IAuthorizationService authorizeService = + services.GetService(); + IAuthorizationPolicyProvider policyProvider = + services.GetService(); + + if (authorizeService == null || policyProvider == null) + { + return string.IsNullOrWhiteSpace(directive.Policy); + } AuthorizationPolicy policy = null; diff --git a/src/Server/AspNetCore.Tests/AspNetCore.Tests.csproj b/src/Server/AspNetCore.Tests/AspNetCore.Tests.csproj index c8cea99a2b8..aecb9217367 100644 --- a/src/Server/AspNetCore.Tests/AspNetCore.Tests.csproj +++ b/src/Server/AspNetCore.Tests/AspNetCore.Tests.csproj @@ -36,4 +36,8 @@ + + + + diff --git a/src/Server/AspNetCore.Tests/Authorization/AuthorizationTests.cs b/src/Server/AspNetCore.Tests/Authorization/AuthorizationTests.cs index f98580d6a0e..695a9fc61fb 100644 --- a/src/Server/AspNetCore.Tests/Authorization/AuthorizationTests.cs +++ b/src/Server/AspNetCore.Tests/Authorization/AuthorizationTests.cs @@ -3,7 +3,6 @@ using System.Net.Http; using System.Security.Claims; using System.Threading.Tasks; -using ChilliCream.Testing; using HotChocolate.Execution; using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.Http; @@ -23,7 +22,6 @@ public AuthorizationTests(TestServerFactory testServerFactory) TestServerFactory = testServerFactory; } - private TestServerFactory TestServerFactory { get; } [Fact] @@ -42,7 +40,72 @@ public async Task DefaultPolicy_NotFound() }, context => { - // no user + context.User = new ClaimsPrincipal( + new ClaimsIdentity("testauth")); + }); + + var request = "{ default }"; + var contentType = "application/graphql"; + + // act + HttpResponseMessage message = + await server.SendPostRequestAsync(request, contentType, null); + + // assert + Assert.Equal(HttpStatusCode.OK, message.StatusCode); + + var json = await message.Content.ReadAsStringAsync(); + ClientQueryResult result = JsonConvert + .DeserializeObject(json); + Assert.NotNull(result.Errors); + result.MatchSnapshot(); + } + + [Fact] + public async Task NoAuthServices_Autheticated_True() + { + // arrange + TestServer server = CreateTestServer( + services => + { + services.AddGraphQL(CreateExecutor()); + }, + context => + { + context.User = new ClaimsPrincipal( + new ClaimsIdentity("testauth")); + }); + + var request = "{ default }"; + var contentType = "application/graphql"; + + // act + HttpResponseMessage message = + await server.SendPostRequestAsync(request, contentType, null); + + // assert + Assert.Equal(HttpStatusCode.OK, message.StatusCode); + + var json = await message.Content.ReadAsStringAsync(); + ClientQueryResult result = JsonConvert + .DeserializeObject(json); + Assert.Null(result.Errors); + result.MatchSnapshot(); + } + + [Fact] + public async Task NoAuthServices_Autheticated_False() + { + // arrange + TestServer server = CreateTestServer( + services => + { + services.AddGraphQL(CreateExecutor()); + }, + context => + { + context.User = new ClaimsPrincipal( + new ClaimsIdentity()); }); var request = "{ default }"; @@ -81,7 +144,8 @@ public async Task Policy_NotFound() }, context => { - // no user + context.User = new ClaimsPrincipal( + new ClaimsIdentity("testauth")); }); var request = "{ age }"; @@ -120,7 +184,8 @@ public async Task Policy_NotAuthorized() }, context => { - // no user + context.User = new ClaimsPrincipal( + new ClaimsIdentity("testauth")); }); var request = "{ age }"; @@ -159,11 +224,11 @@ public async Task Policy_Authorized() }, context => { - var identity = new ClaimsIdentity(); + var identity = new ClaimsIdentity("testauth"); identity.AddClaim(new Claim( ClaimTypes.DateOfBirth, "2013-05-30")); - context.User.AddIdentity(identity); + context.User = new ClaimsPrincipal(identity); }); var request = "{ age }"; @@ -194,7 +259,8 @@ public async Task Roles_UserHasNoRoles_NotAuthorized() }, context => { - // no user + context.User = new ClaimsPrincipal( + new ClaimsIdentity("testauth")); }); var request = "{ roles }"; @@ -225,7 +291,7 @@ public async Task Roles_UserHasDifferentRoles_NotAuthorized() }, context => { - var identity = new ClaimsIdentity(); + var identity = new ClaimsIdentity("testauth"); identity.AddClaim(new Claim( ClaimTypes.Role, "b")); @@ -260,7 +326,7 @@ public async Task Roles_UserHasOneOfTheRoles_NotAuthorized() }, context => { - var identity = new ClaimsIdentity(); + var identity = new ClaimsIdentity("testauth"); identity.AddClaim(new Claim( ClaimTypes.Role, "a")); @@ -298,14 +364,14 @@ public async Task Roles_UserHasAllOfTheRoles_Authorized() }, context => { - var identity = new ClaimsIdentity(); + var identity = new ClaimsIdentity("testauth"); identity.AddClaim(new Claim( ClaimTypes.Role, "a")); identity.AddClaim(new Claim( ClaimTypes.Role, "b")); - context.User.AddIdentity(identity); + context.User = new ClaimsPrincipal(identity); }); var request = "{ roles_ab }"; @@ -336,11 +402,11 @@ public async Task Roles_Authorized() }, context => { - var identity = new ClaimsIdentity(); + var identity = new ClaimsIdentity("testauth"); identity.AddClaim(new Claim( ClaimTypes.Role, "a")); - context.User.AddIdentity(identity); + context.User = new ClaimsPrincipal(identity); }); var request = "{ roles }"; @@ -384,14 +450,14 @@ public async Task PipedAuthorizeDirectives_Authorized() }, context => { - var identity = new ClaimsIdentity(); + var identity = new ClaimsIdentity("testAuth"); identity.AddClaim(new Claim( ClaimTypes.DateOfBirth, "2013-05-30")); identity.AddClaim(new Claim( ClaimTypes.Country, "US")); - context.User.AddIdentity(identity); + context.User = new ClaimsPrincipal(identity); }); var request = "{ piped }"; @@ -435,7 +501,7 @@ public async Task PipedAuthorizeDirectives_SecondFails_NotAuthorized() }, context => { - var identity = new ClaimsIdentity(); + var identity = new ClaimsIdentity("testauth"); identity.AddClaim(new Claim( ClaimTypes.DateOfBirth, "2013-05-30")); @@ -475,6 +541,7 @@ private TestServer CreateTestServer( OnCreateRequest = (ctx, r, ct) => { configureUser(ctx); + r.SetProperty(nameof(ClaimsPrincipal), ctx.User); return Task.CompletedTask; } }); diff --git a/src/Server/AspNetCore.Tests/Authorization/__snapshots__/AuthorizationTests.NoAuthServices_Autheticated_False.snap b/src/Server/AspNetCore.Tests/Authorization/__snapshots__/AuthorizationTests.NoAuthServices_Autheticated_False.snap new file mode 100644 index 00000000000..d6dc4d70b34 --- /dev/null +++ b/src/Server/AspNetCore.Tests/Authorization/__snapshots__/AuthorizationTests.NoAuthServices_Autheticated_False.snap @@ -0,0 +1,19 @@ +{ + "Data": { + "default": null + }, + "Errors": [ + { + "message": "The current user is not authorized to access this resource.", + "locations": [ + { + "line": 1, + "column": 3 + } + ], + "path": [ + "default" + ] + } + ] +} diff --git a/src/Server/AspNetCore.Tests/Authorization/__snapshots__/AuthorizationTests.NoAuthServices_Autheticated_True.snap b/src/Server/AspNetCore.Tests/Authorization/__snapshots__/AuthorizationTests.NoAuthServices_Autheticated_True.snap new file mode 100644 index 00000000000..f4b625827f7 --- /dev/null +++ b/src/Server/AspNetCore.Tests/Authorization/__snapshots__/AuthorizationTests.NoAuthServices_Autheticated_True.snap @@ -0,0 +1,6 @@ +{ + "Data": { + "default": "foo" + }, + "Errors": null +}