Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding support for CORS rules to dotnet-monitor app #1346

Closed
SachiraChin opened this issue Jul 15, 2020 · 5 comments · Fixed by #1377
Closed

Adding support for CORS rules to dotnet-monitor app #1346

SachiraChin opened this issue Jul 15, 2020 · 5 comments · Fixed by #1377
Assignees
Labels
dotnet-monitor enhancement New feature or request

Comments

@SachiraChin
Copy link

Reasonning

I wanted to develop a simple client side web app which works with REST APIs provided by dotnet-monitor tool. My reasoning behind developing this client application was that, system admins could get rich UI experience for APIs provided by dotnet-monitor without need of other tools. The client side app would be a static site which can be configured to use one or more REST endpoint of dotnet-monitor tool and to be navigated via UI without manually providing URLs in the browser. Static app like this can be even embedded inside the tool itself to provide user experience for admins (and I agree that this scenario will not require CORS management in app itself).

Issue

When I started development, I found that, direct queries cannot be sent by browser application to the APIs provided by the tool because API does not support CORS rules.

Solution

I have worked on proof-of-concept for this which will allow user to configure CORS rules.

CORS rules for dotnet-monitor can be provided in two ways,

  • Command line parameter: dotnet monitor collect --corsAllowedOrigins {origin}. This only allows user to enter allowed origins for running instance of dotnet-monitor. User can enter one or more origins using this parameter, values provided here will be merged with origins provided in appsettings.json file.

  • Using static configuration using appsettings.json section. This allows user to configure all CorsPolicy properties defined in Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicy class. appsettings.json section can be configured as below,

{
  "CorsPolicy": {
    "AllowAnyHeader": false,
    "AllowAnyMethod": false,
    "AllowAnyOrigin": false,
    "IsOriginAllowed": null,
    "ExposedHeaders": null,
    "Headers": null,
    "Methods": null,
    "Origins": [ "http://localhost:4200/" ],
    "PreflightMaxAge": null,
    "SupportsCredentials": false
  }
}

Implementation

Proof-of-concept implementation changes can be found in this commit in my fork.

As a highlight, changes are

Program.cs

private static Option CorsRules() =>
    new Option(
        aliases: new[] { "--corsAllowedOrigins" },
        description: "CORS allowed origins for the REST api.")
    {
        Argument = new Argument<string[]>(name: "corsAllowedOrigins", defaultValue: Array.Empty<string>())
    };

Above rule is added as an argument for CommandHandler.Create

DiagnosticsMonitorCommandHandler.cs

var corsPolicySection = context.Configuration.GetSection("CorsPolicy");
if (corsPolicySection.Exists())
{
    var corsPolicy = new CorsPolicy();
    corsPolicySection.Bind(corsPolicy);
    UpdateCorsPolicy(corsAllowedOrigins, corsPolicy);
    services.AddCors(options =>
    {
        options.AddPolicy(Startup.CorsPolicyName, corsPolicy);
    });
}
else if (corsAllowedOrigins?.Length > 0)
{
    services.AddCors(options =>
    {
        var corsPolicy = new CorsPolicy();
        UpdateCorsPolicy(corsAllowedOrigins, corsPolicy);
        options.AddPolicy(Startup.CorsPolicyName, corsPolicy);
    });
}

Above logic has added inside .ConfigureServices((WebHostBuilderContext context, IServiceCollection services) => block to create the CORS policy.

Startup.cs

app.UseCors(CorsPolicyName);

Finally added UseCors to Startup class so runtime can use the configured CORS policy.

Repository with the updated code can be found here: https://github.com/SachiraChin/diagnostics/

Conclusion

I really would appreciate if you could take this request to consideration and add it to the tool. I can make pull requests to the repository to make this change available if required.

@hoyosjs
Copy link
Member

hoyosjs commented Jul 15, 2020

cc: @shirhatti @jander-msft @wiktork

@hoyosjs hoyosjs added dotnet-monitor enhancement New feature or request labels Jul 15, 2020
@wiktork wiktork self-assigned this Jul 22, 2020
@wiktork
Copy link
Member

wiktork commented Jul 22, 2020

@SachiraChin Thank you for the detailed issue! The PR for the fix is at #1377.

@SachiraChin
Copy link
Author

@wiktork Thanks a lot for the quick PR. :) I will use this for development of client UI I'm working on.

@noahfalk
Copy link
Member

@SachiraChin - A bunch of us devs at MS working on .NET and VS saw the UI you were doing and its very cool!

@SachiraChin
Copy link
Author

@noahfalk That's really really nice to hear! 😊 I'm adding few more features to the app. You will see new updates in the repo soon. Here's small glance of it,

image

If you or anyone had chance to look at the code and see any issue with how it written, please don't hesitate to ping back at me. I'm gladly take any criticism you have. 😊

@dotnet dotnet locked as resolved and limited conversation to collaborators Jun 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dotnet-monitor enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants