OData / WebApi

OData Web API: A server library built upon ODataLib and WebApi
https://docs.microsoft.com/odata
Other
855 stars 473 forks source link

Operation inside batch returns a 200 OK instead of 401 Unauthorized #1546

Open OlivierDeRivoyre opened 6 years ago

OlivierDeRivoyre commented 6 years ago

When calling a method protected by the Authorize attribute with a $batch, the returned value inside the $batch body is a 200 OK instead of 401 Unauthorized. The bug do not occurs for the first request of the batch, only for the one after.

Assemblies affected

Seen in Microsoft.AspNetCore.OData 7.0.1

Reproduce steps

Use the branch feature/netcore of WebApiOData.AspNetCore. Add the $batch ability:

public void Configure(IApplicationBuilder app, IHostingEnvironment env)
{
    if (env.IsDevelopment())
    {
        app.UseDeveloperExceptionPage();
    }
    app.UseODataBatching();
    app.UseAuthentication();
    app.UseMvc(builder =>
    {
       builder.MapODataServiceRoute("odata", "odata",
          configureAction: containerBuilder => containerBuilder
              .AddService(Microsoft.OData.ServiceLifetime.Singleton, typeof(IEdmModel),
                  sp => EdmModelBuilder.GetEdmModel())
              .AddService(Microsoft.OData.ServiceLifetime.Singleton, typeof(IEnumerable<IODataRoutingConvention>),
                  sp => ODataRoutingConventions.CreateDefaultWithAttributeRouting("odata", builder))
              .AddService(Microsoft.OData.ServiceLifetime.Singleton, typeof(ODataUriResolver),
                  sp => new StringAsEnumResolver())
              .AddService(Microsoft.OData.ServiceLifetime.Singleton, typeof(ODataBatchHandler),
                  sp => new DefaultODataBatchHandler()));

   });
}

POST http://localhost:5912/odata/$batch sould answer

Add some JWT authorization on the movies: Add the [Authorize] on MoviesController

[Microsoft.AspNetCore.Authorization.Authorize]
public class MoviesController : ODataController

Declare the JWT:

public void ConfigureServices(IServiceCollection services)
{
    services.AddDbContext<MovieContext>(opt => opt.UseInMemoryDatabase("MovieList"));
    services.AddOData();
    services.AddMvc();
    services.AddMvcCore(options =>
    {
        var authorizationPolicy = new AuthorizationPolicyBuilder(JwtBearerDefaults.AuthenticationScheme).RequireAuthenticatedUser().Build();
        options.Filters.Add(new AuthorizeFilter(authorizationPolicy));
    });
    services.AddAuthentication(options =>
    {
        options.DefaultAuthenticateScheme = JwtBearerDefaults.AuthenticationScheme;
        options.DefaultChallengeScheme = JwtBearerDefaults.AuthenticationScheme;
    })
    .AddJwtBearer(options =>
    {
        options.TokenValidationParameters = new TokenValidationParameters { };
    });
}

GET http://localhost:5912/odata/Movies should now do a 401 error

Do a $batch on Movies:

POST http://localhost:5912/odata/$batch
Content-Type: multipart/mixed; boundary=####

with raw body (and no JWT token):

--####
Content-Type: application/http
Content-Transfer-Encoding:binary

GET /odata/Movies HTTP/1.1
Accept: application/json
Content-Type: application/json

--####
Content-Type: application/http
Content-Transfer-Encoding:binary

GET /odata/Movies HTTP/1.1
Accept: application/json
Content-Type: application/json

--####
Content-Type: application/http
Content-Transfer-Encoding:binary

GET /odata/Movies HTTP/1.1
Accept: application/json
Content-Type: application/json

Note that the first request has been well 401 Unauthorized

Expected result

--batchresponse_8443f727-3053-4434-9929-580f29d3468d
Content-Type: application/http
Content-Transfer-Encoding: binary

HTTP/1.1 401 Unauthorized
WWW-Authenticate: Bearer,Bearer,Bearer

--batchresponse_8443f727-3053-4434-9929-580f29d3468d
Content-Type: application/http
Content-Transfer-Encoding: binary

HTTP/1.1 401 Unauthorized
WWW-Authenticate: Bearer,Bearer,Bearer

--batchresponse_8443f727-3053-4434-9929-580f29d3468d
Content-Type: application/http
Content-Transfer-Encoding: binary

HTTP/1.1 401 Unauthorized
WWW-Authenticate: Bearer,Bearer,Bearer

--batchresponse_8443f727-3053-4434-9929-580f29d3468d--

Actual result

--batchresponse_8443f727-3053-4434-9929-580f29d3468d
Content-Type: application/http
Content-Transfer-Encoding: binary

HTTP/1.1 401 Unauthorized
WWW-Authenticate: Bearer,Bearer,Bearer

--batchresponse_8443f727-3053-4434-9929-580f29d3468d
Content-Type: application/http
Content-Transfer-Encoding: binary

HTTP/1.1 200 OK

--batchresponse_8443f727-3053-4434-9929-580f29d3468d
Content-Type: application/http
Content-Transfer-Encoding: binary

HTTP/1.1 200 OK

--batchresponse_8443f727-3053-4434-9929-580f29d3468d--

Additional detail

Note that the controller's method is not called, it is well protected by the Authorization. It is only the error code that is lost.

biaol-odata commented 6 years ago

@OlivierDeRivoyre The batch is in multipart/mixed format. The response for the overall $batch should be 200 as per OData spec, since all sub-requests have been processed.

As for the sub requests with response code 200, for comparison, do you have information about received sub responses using Microsoft.AspNet.OData 6.x?

OlivierDeRivoyre commented 6 years ago

Hi @biaol-odata

I completly agree with you that the overall $batch response sould be (and is) 200. The problem is on the sub responses that should be 401 instead of 200.

I do not have information with the Microsoft.AspNet.OData 6.x version. I'm working in AspNetCore, and I do not know the AspNet version. Sorry for that.

Meanwhile, I have found a complementary workaround that consist to forbid the $batch request to unauthenticated client. It is done with:

public class AuthorizedDefaultODataBatchHandler : DefaultODataBatchHandler
{
    public override Task ProcessBatchAsync(HttpContext context, RequestDelegate nextHandler)
    {
        if (!context.User.Identity.IsAuthenticated)
        {
            context.Response.StatusCode = (int) HttpStatusCode.Unauthorized;
            return Task.CompletedTask;
        }
        return base.ProcessBatchAsync(context, nextHandler);
    }       
}    

I plug this customisation during config.MapODataServiceRoute().

It is not perfect since it ignores the Claims/Roles of the user, and it suppose that all methods are protected. Nonetheless, it can be usefull to simply forbid $batch to unlogged client.

biaol-odata commented 6 years ago

@OlivierDeRivoyre I don't think in your case specific response code matters, either it be 400, 401, or even 500. Additionally, batch format, either it be in multipart/mixed or in Json format, does not matter either in the part of the implementation related to individual request processing.

With that said, I tried out some batch with failed operations followed by successful operation, using Microsoft.AspNetCore.OData 7.0.1, and am able to receive expected batch response.

{
    "responses": [{
            "id": "-1",
            "status": 500,
            "headers": {}
        }, {
            "id": "0",
            "status": 500,
            "headers": {}
        }, {
            "id": "1",
            "atomicityGroup": "ac0b1e9c-4f8c-4d6c-804a-ab0ec2c86d2f",
            "status": 201,
            "headers": {
                "location": "http://localhost:9999/path/to/entitysetName('key1')",
                "content-type": "application/json; odata.metadata=minimal; odata.streaming=true; charset=utf-8"
            },
            "body": {
                "@odata.context": "http://localhost:9999/path/to/entitysetName/$metadata#ET/$entity",
                "Id": "key1",
                "Name": "CreatedByJsonBatch_11"
            }
        }, {
            "id": "2",
            "atomicityGroup": "ac0b1e9c-4f8c-4d6c-804a-ab0ec2c86d2f",
            "status": 201,
            "headers": {
                "location": "http://localhost:9999/path/to/entitysetName('key2')",
                "content-type": "application/json; odata.metadata=minimal; odata.streaming=true; charset=utf-8"
            },
            "body": {
                "@odata.context": "http://localhost:9999/path/to/$metadata#entitysetName/$entity",
                "Id": ":key2",
                "Name": "Name2"
            }
        }, {
            "id": "3",
            "status": 201,
            "headers": {
                "location": "http://localhost:9999/path/to/entitysetName('key3')",
                "content-type": "application/json; odata.metadata=minimal; odata.streaming=true; charset=utf-8"
            },
            "body": {
                "@odata.context": "http://localhost:9999/path/to/$metadata#entitysetName/$entity",
                "Id": "key3",
                "Name": "Name3"
            }
        }
    ]
}

Note that in the above response, response code 500 is set correctly for first two failed operations respectively; and the other successful operations also have 2xx response codes returned correctly.

So, it works for my experiment. Each operation response is handled inside a thread without shared state, therefore I don't think there could be any multi-thread issue causing data racing/mixed up.

Can you double check on your side? Thanks.

OlivierDeRivoyre commented 6 years ago

Hi Bill,

If I remove the Authorisation and if I change the controller method to return a 500 error, this 500 error is correctly reported (and repeated) in the $batch.

I think the 401 is important because it is not raised by the controller method but by the AuthenticationMiddleware. I guess the AuthenticationMiddleware and the ODataBatchMiddleware do not mix together as expected.

Another interresting behavior that may help: If I switch UseODataBatching() and UseAuthentication() to have this order:

public void Configure(IApplicationBuilder app, IHostingEnvironment env)
{
    app.UseAuthentication();
    app.UseODataBatching();
    ...

then, the $batch returns only 200 OK

--batchresponse_d1dcd2d7-ee5a-4cc1-94c0-c0f09f038413
Content-Type: application/http
Content-Transfer-Encoding: binary

HTTP/1.1 200 OK

--batchresponse_d1dcd2d7-ee5a-4cc1-94c0-c0f09f038413
Content-Type: application/http
Content-Transfer-Encoding: binary

HTTP/1.1 200 OK

--batchresponse_d1dcd2d7-ee5a-4cc1-94c0-c0f09f038413
Content-Type: application/http
Content-Transfer-Encoding: binary

HTTP/1.1 200 OK

--batchresponse_d1dcd2d7-ee5a-4cc1-94c0-c0f09f038413--

Instead of having 401/200/200, we now have 200/200/200 (we should have 401/401/401). The Get() method in MoviesController.cs is not called, thus the Authorisation seem to do its job.

MonkeyTennis commented 4 years ago

Is there a plan to progress this? It is not ideal that 401 errors, which could easily occur within a batch, are not returned to the caller.

HeinA commented 2 years ago

This is still an issue in OData 8.0.10...

HeinA commented 1 year ago

This is really a big issue for me.

My application uses windows authentication, and while logged into the domain, my calls randomly and silently fails because somehow the batch calls can not be authenticated.

The only way to make the OData Batch work reliably is to completely remove Anonymous Authentication from IIS. This is not ideal, since I sometimes want to allow unauthenticated calls for certain controllers. Now I have to split it into 2 separate sites. One for Authenticated calls and one for Anonymous calls.

It also really stuffs me around while debugging using Kestrel

I really believe this issue is related to HttpContextAccessor.HttpContext for a $batch request becomes unset after returning from ODataBatchMiddleware

Here is an Edge HAR file showing the issue. I get 2 HTTP 200s but no controller is hit. My client thinks the calls were successful.

Batch Request Not Working.zip

shnitze commented 1 year ago

Hopefully this helps, I had a similar issue with my own implementation of a JSON batch endpoint where the subrequests return HTTP 200 when a user is unauthorized. I was able to fix it by implementing IAuthorizationMiddlewareResultHandler to set the response StatusCode.

In my case, it was because the user is authenticated before the BatchMiddleware and the AuthorizationMiddleware calls AuthenticationHandler.ForbidAsync when a user is unauthorized. Overriding that behaviour and directly setting the Response.StatusCode worked for me.

public class UnauthorizedAuthorizationMiddlewareResultHandler : IAuthorizationMiddlewareResultHandler
    {
        private readonly AuthorizationMiddlewareResultHandler defaultHandler = new AuthorizationMiddlewareResultHandler();
        public async Task HandleAsync(RequestDelegate next,
                                HttpContext context,
                                AuthorizationPolicy policy,
                                PolicyAuthorizationResult authorizeResult)
        {
            if (context is null) throw new ArgumentNullException(nameof(context));
            if (authorizeResult is null) throw new ArgumentNullException(nameof(authorizeResult));

            if (authorizeResult.Forbidden)
            {
                context.Response.StatusCode = StatusCodes.Status403Forbidden;
                return;
            }

            //Fallback to the default implementation
            await defaultHandler.HandleAsync(next, context, policy, authorizeResult);
}

ConfigureServices:

services.AddSingleton<IAuthorizationMiddlewareResultHandler, UnauthorizedAuthorizationMiddlewareResultHandler>();

Configure:

app.UseAuthentication();
app.UseBatchEndpoint();
app.UseRouting();
app.UseAuthorization();
app.UseEndpoints();

Microsoft's Doc on IAuthorizationMiddlewareResultHandler: Customize the behavior of AuthorizationMiddleware

robertmclaws commented 1 year ago

Isn't part of the issue here that the services are registered in the wrong order? The order that you call ".UseXXX()" matters, because that that is the order the pipeline will be called.

@OlivierDeRivoyre in the original post, you should be calling it this way:

    app.UseAuthentication();
    app.UseAuthorization();
    app.UseODataBatching();
    app.UseMvc(builder =>
    {
       builder.MapODataServiceRoute("odata", "odata",
          configureAction: containerBuilder => containerBuilder
              .AddService(Microsoft.OData.ServiceLifetime.Singleton, typeof(IEdmModel),
                  sp => EdmModelBuilder.GetEdmModel())
              .AddService(Microsoft.OData.ServiceLifetime.Singleton, typeof(IEnumerable<IODataRoutingConvention>),
                  sp => ODataRoutingConventions.CreateDefaultWithAttributeRouting("odata", builder))
              .AddService(Microsoft.OData.ServiceLifetime.Singleton, typeof(ODataUriResolver),
                  sp => new StringAsEnumResolver())
              .AddService(Microsoft.OData.ServiceLifetime.Singleton, typeof(ODataBatchHandler),
                  sp => new DefaultODataBatchHandler()));
   });

@shnitze Your code has the same problem, because you've registered Authorization after you registered the routing and batch endpoint.