OData / AspNetCoreOData

ASP.NET Core OData: A server library built upon ODataLib and ASP.NET Core
Other
454 stars 160 forks source link

Url rewrite behind proxy: `ODataOutputFormatter.BaseAddressFactory` does not affect `@odata.nextLink` urls #1257

Open lf-novelt opened 3 months ago

lf-novelt commented 3 months ago

Assemblies affected

Describe the bug The odata app is hosted behind multiple proxies. It needs to send the publicly exposed urls back to the client rather than the app service internal urls. I've checked the solution proposed in https://github.com/OData/AspNetCoreOData/issues/876 but it does not fully resolve the problem as the whole url needs to be changed, not only the Host. I think that's the purpose of ODataOutputFormatter.BaseAddressFactory.

As stated in the doc, ODataOutputFormatter.BaseAddressFactory is supposed to "Gets or sets a method that allows consumers to provide an alternate base address for OData Uri.". It works well by altering the main @odata.context urls but when a set has too many rows and a @odata.nextlink is generated, the url is still computed using only the original request. That leads to mixed urls in the same OData response. One would expect all OData generated urls would be managed by this output formatter.

Reproduce steps Register a custom ODataOutputFormatter.BaseAddressFactory in Program.cs and set a low MaxTop to trigger the nextLink and request any endpoint with more entities than MaxTop value without $top.

        // enable odata features, especially MaxTop to enable nextlink
        builder.AddOData(options =>
        {
            options.Select().Filter().OrderBy().SetMaxTop(AppSettings.Current.OdataMaxPageSize).Count();
        });

       // Modify the OData output formatter for @odata.context
        builder.Services.Configure<MvcOptions>(options =>
        {
            string replaceHost = AppSettings.Current.FrontDoorODataBaseHost;
            foreach (var outputFormatter in options.OutputFormatters.OfType<ODataOutputFormatter>())
            {
                outputFormatter.BaseAddressFactory = (request) =>
                {
                    var std = ODataOutputFormatter.GetDefaultBaseAddress(request);
                    var customHelper = new CustomUrlHelper(request.HttpContext, replaceHost);

                    return customHelper.ReplaceUrlLinkIfPublic(std);
                };
            }
        });

Data Model Schoolsentity from the default template, does not matter here.

Request/Response In this example, the private url is https://localhost:44345/odata/ and should be rewritten to https://localhost:5001/. Please share your request Uri, head or the request body https://localhost:44345/odata/SYS_ANOTHERDATASOURCE/Schools Please share your response head, body.

**Expand response body json** ```json { "@odata.context": "https://localhost:5001/SYS_ANOTHERDATASOURCE/$metadata#Schools", "@odata.count": 12, "value": [ { "ID": 100, "CreatedDay": null }, { "ID": 101, "CreatedDay": null }, { "ID": 102, "CreatedDay": null }, { "ID": 103, "CreatedDay": null }, { "ID": 104, "CreatedDay": null }, { "ID": 105, "CreatedDay": null }, { "ID": 106, "CreatedDay": null }, { "ID": 107, "CreatedDay": null }, { "ID": 108, "CreatedDay": null }, { "ID": 109, "CreatedDay": null } ], "@odata.nextLink": "https://localhost:44345/odata/SYS_ANOTHERDATASOURCE/Schools?$skip=10" } ```

Expected behavior The @odata.nextLink url should be processed by the custom provided BaseAddressFactory

Screenshots image

lf-novelt commented 3 months ago

this is where the request.Uri is passed to the GetNextLink function, we can clearly see it does not use the BaseAddressFactory and it should not be hard to add it here (NB: this is not the only place GetNextPageLink is called, not certain this is the correct place)

https://github.com/OData/AspNetCoreOData/blob/599dc1bdca9edc2cf88935ed54fe46ad5478da5b/src/Microsoft.AspNetCore.OData/Extensions/HttpRequestExtensions.cs#L194-L204

julealgon commented 3 months ago

The inconsistency of behavior across the @odata.context and @odata.nextlink results does seem to be a clear indicator of a bug to me on this one.

habbes commented 3 months ago

@lf-novelt the next page link url is generated by the SkipTokenHandler. You can create a custom SkipTokenHandler that overwrites the default one and rewrites the URL to replace private host with the public one. Here's a guide on customizing the SkipTokenHandler: https://learn.microsoft.com/en-us/odata/webapi-8/tutorials/custom-skiptokenhandler

I do agree that it's on obvious, and it's confusing, that you would have to change the URL in multiple locations. There's a room to improve the design, but I don't think SkipTokenHandler should have a dependency on ODataOutputFormatter.BaseAddressFactory.

lf-novelt commented 3 months ago

hey @habbes , thanks for the followup. good point, indeed I was not aware of this SkipTokenHandler, however when reading the doc, it seems more suited to allow customize the $skipToken value rather than the baseUri. Indeed the baseUri is passed as parameter of the GenerateNextPageLink method, so why not pass the customized base url directly to it? image

The input baseUri seems generated here, so in the Formatter section so it does not seem irrelevant to call the ODataOutputFormatter.BaseAddressFactory here, when passing the new Uri(writeContext.Request.GetEncodedUrl() parameter, does it? https://github.com/OData/AspNetCoreOData/blob/599dc1bdca9edc2cf88935ed54fe46ad5478da5b/src/Microsoft.AspNetCore.OData/Formatter/Serialization/ODataCollectionSerializer.cs#L112