-
Notifications
You must be signed in to change notification settings - Fork 602
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
Add CORS support for MiniProfiler #502
Comments
To get this working for Angular, I also need to implement a custom HttpInterceptor. I tried setting Here's the custom HttpInterceptor: import { HttpEvent, HttpHandler, HttpInterceptor, HttpRequest, HttpResponse } from '@angular/common/http';
import { Injectable } from '@angular/core';
import { NavigationStart, Router } from '@angular/router';
import { Observable } from 'rxjs';
import { filter, tap } from 'rxjs/operators';
declare const MiniProfiler:
| {
container?: HTMLElement;
fetchResults(ids: string[]): void;
}
| undefined;
@Injectable()
export class HttpProfilerInterceptor implements HttpInterceptor {
constructor(router: Router) {
// Clear profiler results on navigation
router.events.pipe(filter(ev => ev instanceof NavigationStart)).subscribe(() => {
if (typeof MiniProfiler !== 'undefined' && MiniProfiler.container) {
const clearButton = MiniProfiler.container.querySelector<HTMLSpanElement>(
'.mp-controls .mp-clear',
);
if (clearButton) {
clearButton.click();
}
}
});
}
intercept(req: HttpRequest<any>, next: HttpHandler): Observable<HttpEvent<any>> {
return next.handle(req).pipe(
tap((event: HttpEvent<any>) => {
// if the event is for http request
if (event instanceof HttpResponse) {
if (typeof MiniProfiler !== 'undefined') {
try {
const miniProfilerIds = event.headers.get('X-MiniProfiler-Ids');
if (miniProfilerIds) {
MiniProfiler.fetchResults(JSON.parse(miniProfilerIds));
}
} catch (err) {}
}
}
}),
);
}
} |
I'm not opposed to adding this - sounds like a useful feature if we can make it neat! I'm mostly unsure about the CORS header bits (need to poke more). For |
These issues are because our UI and API have different origins - so all requests to the API are cross-origin. That means they need to handle the pre-flight OPTIONS request and set the necessary CORS headers for the requests to work at all.
It would be useful to document this so anyone looking to do a similar setup can easily solve it - I think it's probably enough to give an example UI snippet. I'm not sure what to do about Angular - that really seems to be an issue with As I said, I'm happy to open a PR if that makes it easier to continue the discussion. |
I need to fire this up local when time allows to better understand it. Is a CORS policy always in effect? We're not using that functionality atm, so not familiar with it. MiniProfiler middleware handles all requests to its routes though, so it should be able to respond to I'm not sure what the cleanest API is there, but definitely not for another top level method, it should be an option or on a specific builder (imagine if we have a |
A CORS policy isn't always in effect, it's only required and used when you serve cross-origin requests. Here's the MDN article on exposed headers - the take away is this (for cross-origin requests):
The reason I would use the existing CORS middleware is so you can specify which origins are allowed access and which aren't. I agree with your point with using an option rather than a new top level method. |
Gotcha - I propose: a new boolean on This setup keeps the API super simple and we'll take care of the rest. I'm curious if that works though, or if it needs to be not a bool but a list of origins or some such. Thoughts on that API surface area? |
It would be worth a try like that. My example code opens MiniProfiler to all origins - are you concerned about doing that? |
@pmccloghrylaing That's the part I mean about "maybe it's not a bool" - maybe it's something other type (e.g. list) to support multiple origins (and it not being there/null is "off")...whatever format we need to add a policy correctly (not in front of a comp to check APIs atm) - that make sense? |
I've added a draft PR that still needs testing. @NickCraver I've added a single property to options and the rest is taken care of by the middleware. I didn't find that I could do much with |
Related to #332
MiniProfiler doesn't work out of the box when the UI application is on a different origin to the API application due to missing CORS support.
In order to get MiniProfiler working for an Angular application running from a different origin I had to set up the following:
OPTIONS
requests so the MiniProfiler endpoints would succeed:.WithExposedHeaders("X-MiniProfiler-Ids")
, otherwiseX-MiniProfiler-Ids
isn't available to the MiniProfiler UI:data-path
needed to include the host):Suggestions:
UseMiniProfiler
for setting a CORS policy for MiniProfilerX-MiniProfiler-Ids
headerI'd be happy to open up a PR for this issue if the suggestions are OK.
The text was updated successfully, but these errors were encountered: