Add CORS headers for swagger/openapi endpoints so that the spec can be rendered with petstore.swagger.io #570

b-admike · 2017-09-13T11:28:44Z

Add /swagger-ui which would redirect users to the online SwaggerUI instance with the app's OpenAPI spec loaded and CORS enabled so they could access their endpoints from it.

Description

Related issues

closes Add CORS support to swagger/openapi endpoints #506

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

bajtos

I am afraid this pull request makes very little sense to me.

Even if we decide to include GET /explorer endpoint on LoopBack apps, then I think this should be contributed by an "explorer" extension, it should not be built into core/rest layer.

Maybe I misunderstood your intentions, what am I missing?

bajtos · 2017-09-13T11:35:02Z

packages/core/src/application.ts

@@ -147,6 +147,12 @@ export class Application extends Context {
      const options = OPENAPI_SPEC_MAPPING[request.url];
      return this._serveOpenApiSpec(request, response, options);
    }
+
+    if (request.method === 'GET' && request.url === '/explorer') {
+      response.setHeader('Access-Control-Allow-Origin', '*');


IMO, we need to set Access-Control headers inside _serveOpenApiSpec, not here. GET /explorer would be invoked from the browser as a regular request for an HTML page.

I think the purpose of the story was to allow CORS for all endpoints defined by an application's OpenAPI spec, so that users can test them out using the SwaggerUI instance. With that in mind, I think it should still be set in this method, otherwise it will only be allowed for the /swagger.{json,yaml} or /openapi.{json,yaml} endpoints. Two things I'm not sure of though are if there are any security implications by doing so, and if we should allow it to be set by our application developers.

bajtos · 2017-09-13T11:36:54Z

packages/core/src/application.ts

+
+    if (request.method === 'GET' && request.url === '/explorer') {
+      response.setHeader('Access-Control-Allow-Origin', '*');
+      response.setHeader('Location', 'http://editor.swagger.io');


In LoopBack 3.x, /explorer was opening SwaggerUI, not the editor.

bajtos · 2017-09-13T11:39:04Z

This is the live SwaggerUI instance that people can use to get Explorer-like view of their API: http://petstore.swagger.io/

b-admike · 2017-09-13T11:53:51Z

@bajtos Thank you. I was thinking we could use /explorer endpoint temporarily built into core, but I agree in making an extension for it. This is just an initial commit to get feedback early.

raymondfeng · 2017-09-13T16:01:33Z

packages/core/src/application.ts

    if (options.version === '3.0.0') {
      specObj = await swagger2openapi.convertObj(specObj, {direct: true});
+      appUrl.replace('swagger', 'openapi');


I don't think replace will change appUrl at all.

Good catch. I will fix it pending on discussion of #570 (review).

raymondfeng · 2017-09-13T16:03:16Z

packages/core/src/application.ts

      response.setHeader('content-type', 'application/json; charset=utf-8');
+      response.statusCode = 308;
+      response.setHeader('Location', 'http://petstore.swagger.io/?url='
+      + appUrl);


This is wrong. We should not deal with redirect here for the swagger/openapi endpoint. Our responsibility is to serve specs.

Maybe I misunderstood the story, but I thought we should do it as part of the following acceptance criteria:

Pass generated swagger.json to editor.swagger.io so that users can leverage the existing editor tools on their local application.

@kjdelisle thoughts?

That was my understanding, too. @raymondfeng How do we pass the information along to the server at editor.swagger.io without modifying the request?

There are two separate concerns:

Have endpoints from LB to serve openapi/swagger specs

A UI to render the spec as api explorer

Most UI such as editor.swagger.io has an option to import URL or set the location. If we want to use redirect to editor.swagger.io, the path should not be /swagger.json. At lease we need to have something like /swagger.json?ui=true. It should NOT be replacing 1 at all.

For the GA version, we'll have /explorer to serve the UI.

Okay it makes sense to me now that we need to preserve the /{openapi,swagger}.{json,yaml} endpoints. I will take a crack at implementing /swagger.json?ui=true.

raymondfeng · 2017-09-13T18:01:56Z

packages/core/src/application.ts

@@ -234,16 +237,28 @@ export class Application extends Context {
  ) {
    options = options || {version: '2.0', format: 'json'};
    let specObj = this.getApiSpec();
+    let appPort = await this.get('http.port');
+    // TODO make this more robust i.e. get host, protocol, etc. dynamically
+    let appUrl = 'http://127.0.0.1:' + appPort + '/swagger';


The hostname and port should be default the Host header of the http request. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Host

Unfortunately, this will not work in production(like) environments, where the app is running behind a load balancer (nginx, Amalgam8, etc.). The Host header of the http request will be showing the internal hostname not accessible from outside. We could use X-Forwarded-* family of headers to determine the real public host, but I think it's not worth the effort.

bajtos · 2017-09-14T07:44:57Z

Maybe I misunderstood the story, but I thought we should do it as part of the following acceptance criteria:

Pass generated swagger.json to editor.swagger.io so that users can leverage the existing editor tools on their local application.

Sorry for the confusion!

As I understand this CORS related story, we need to add CORS headers to /swagger* and /openapi endpoint responses so that people can open Swagger UI or Swagger Editor SPA (single-page app) running somewhere else and let it load swagger/openapi spec from their running LoopBack application by manually pasting http://localhost:3000/openapi.json (or similar) to that SPA. This is not possible right now: the browser will prevent the SPA from accessing http://localhost:3000/openapi.json, because the response from LoopBack-next does not allow cross-site requests.

b-admike · 2017-09-14T16:18:56Z

@bajtos Thank you for clarifying. Can you PTAL at the latest changes? (cc @raymondfeng @kjdelisle) If we expect the users to manually paste the swagger/openapi spec url into SwaggerUI, we can document how they can do that in our wiki.

raymondfeng · 2017-09-14T17:24:22Z

packages/core/src/application.ts

@@ -145,7 +148,7 @@ export class Application extends Context {
      // content-negotiation to support XML clients, I don't want the OpenAPI
      // spec to be converted into an XML response.
      const options = OPENAPI_SPEC_MAPPING[request.url];
-      return this._serveOpenApiSpec(request, response, options);
+        return this._serveOpenApiSpec(request, response, options);


Is this intentional? The indentation seems to be wrong.

kjdelisle · 2017-09-14T21:08:41Z

packages/core/src/application.ts

@@ -135,6 +135,9 @@ export class Application extends Context {
    request: ServerRequest,
    response: ServerResponse,
  ) {
+    // allow CORS support for all endpoints so that users
+    // can test with online SwaggerUI instance
+    response.setHeader('Access-Control-Allow-Origin', '*');


Sorry if I missed it earlier, but is there a reason why we need the header to be *?

kjdelisle

If the header needs to be set to *, then LGTM, but I imagine that it should be a bit more restrictive than that, yeah?

virkt25 · 2017-09-15T04:43:29Z

packages/core/src/application.ts

@@ -135,6 +135,9 @@ export class Application extends Context {
    request: ServerRequest,
    response: ServerResponse,
  ) {
+    // allow CORS support for all endpoints so that users
+    // can test with online SwaggerUI instance
+    response.setHeader('Access-Control-Allow-Origin', '*');


I get we need to set the Header for all endpoints for Swagger but I'm not sure if users would want that enabled in production, especially with the *. Can this be configurable / toggleable via options (ideally ENV variable)? / Can we look into (perhaps as a separate issue) look into turning this into a configurable component?

We should follow the defaults described by http://loopback.io/doc/en/lb3/middleware.json.html#cors-settings.

Access-Control-Allow-Origin: <Origin request header> Access-Control-Allow-Credentials: true Access-Control-Allow-Max-Age: 86400

Please note CORS is only enforced by browsers and it's not a security concern for the server itself.

@virkt25 I think for GA we are aiming to do that (see #488), so I'll leave it as is for now.

bajtos · 2017-09-15T11:44:49Z

packages/core/src/application.ts

@@ -135,6 +135,14 @@ export class Application extends Context {
    request: ServerRequest,
    response: ServerResponse,
  ) {
+    // allow CORS support for all endpoints so that users
+    // can test with online SwaggerUI instance
+    if (process.env.ENABLE_CORS) {


-1 for depending on environment variables.

If you want a feature flag to controller this behaviour, then use app-level configuration, e.g. app.getSync('allow-cors-for-api-spec') here. Then it's up to the applications (or loopback-boot) to wire this binding to the value of process.env.ENABLE_CORS.

IMO, CORS should be always enabled for these endpoints.

@bajtos Thank you for the insight. I think that can be done later, and I am fine for leaving them always enabled for now as well.

bajtos · 2017-09-15T11:45:30Z

packages/core/src/application.ts

@@ -147,6 +155,10 @@ export class Application extends Context {
      const options = OPENAPI_SPEC_MAPPING[request.url];
      return this._serveOpenApiSpec(request, response, options);
    }
+    if (request.method === 'GET' && request.url &&
+        request.url.match(/\/swagger.json\?ui=true/)) {


I find it weird that a query parameter is changing the response so dramatically. Can we use a plain url path like /explorer or /swagger-ui instead?

That makes sense to me. I will change it to /swagger-ui.

bajtos · 2017-09-15T11:46:10Z

packages/core/src/application.ts

+  ) {
+    response.statusCode = 308;
+    response.setHeader('Location', 'http://petstore.swagger.io/?url='
+    + 'http://' + request.headers.host + '/swagger.json');


http://loopback.io/doc/en/contrib/style-guide.html#one-argument-per-line or format the code using Prettier.

bajtos · 2017-09-15T14:47:56Z

packages/core/src/application.ts

+    // can test with online SwaggerUI instance
+      response.setHeader('Access-Control-Allow-Origin', '*');
+      response.setHeader('Access-Control-Allow-Credentials', 'true');
+      response.setHeader('Access-Control-Allow-Max-Age', '86400');


The indentation of code does not match indentation of comments, PTAL.

bajtos · 2017-09-15T14:48:32Z

packages/core/src/application.ts

@@ -147,6 +153,10 @@ export class Application extends Context {
      const options = OPENAPI_SPEC_MAPPING[request.url];
      return this._serveOpenApiSpec(request, response, options);
    }
+    if (request.method === 'GET' && request.url &&
+        request.url === '/swagger-ui') {
+      return this._renderOpenApiSpec(request, response);


nitpick: perhaps _redirectToSwaggerUI would be a better name?

Yeah I think that's a more descriptive name. 👍

Add /swagger-ui path which would redirect users to the online SwaggerUI instance with the app's OpenAPI spec loaded and enable CORS so they could access their endpoints from it.

b-admike requested review from bajtos and raymondfeng as code owners September 13, 2017 11:28

b-admike added needs-priority and removed needs-priority labels Sep 13, 2017

b-admike force-pushed the feat/add-cors branch from 42c0172 to 52d65a4 Compare September 13, 2017 11:29

b-admike changed the title ~~add explorer endpoint~~ [WIP] add explorer endpoint Sep 13, 2017

bajtos suggested changes Sep 13, 2017

View reviewed changes

b-admike requested review from loay, kjdelisle, virkt25, jannyHou and dhmlau September 13, 2017 13:43

raymondfeng reviewed Sep 13, 2017

View reviewed changes

b-admike force-pushed the feat/add-cors branch from ab9d2ff to 4d7486b Compare September 14, 2017 11:41

raymondfeng reviewed Sep 14, 2017

View reviewed changes

b-admike force-pushed the feat/add-cors branch from b89f87f to 14ff8e2 Compare September 14, 2017 19:04

b-admike self-assigned this Sep 14, 2017

kjdelisle reviewed Sep 14, 2017

View reviewed changes

kjdelisle approved these changes Sep 14, 2017

View reviewed changes

raymondfeng changed the title ~~[WIP] add explorer endpoint~~ Add CORS headers for swagger/openapi endpoints so that the spec can be rendered with editor.swagger.io Sep 14, 2017

raymondfeng approved these changes Sep 14, 2017

View reviewed changes

virkt25 reviewed Sep 15, 2017

View reviewed changes

b-admike force-pushed the feat/add-cors branch from 14ff8e2 to f0c5034 Compare September 15, 2017 06:21

b-admike force-pushed the feat/add-cors branch 3 times, most recently from 056da52 to 4882e5a Compare September 15, 2017 06:33

bajtos suggested changes Sep 15, 2017

View reviewed changes

b-admike force-pushed the feat/add-cors branch 2 times, most recently from ccb9e6d to a9d52ff Compare September 15, 2017 14:11

b-admike changed the title ~~Add CORS headers for swagger/openapi endpoints so that the spec can be rendered with editor.swagger.io~~ Add CORS headers for swagger/openapi endpoints so that the spec can be rendered with petstore.swagger.io Sep 15, 2017

virkt25 approved these changes Sep 15, 2017

View reviewed changes

b-admike force-pushed the feat/add-cors branch from a9d52ff to 79f8fbc Compare September 15, 2017 14:41

bajtos approved these changes Sep 15, 2017

View reviewed changes

b-admike force-pushed the feat/add-cors branch 4 times, most recently from ebf68f7 to b134edb Compare September 15, 2017 18:44

fix(core): Add SwaggerUI endpoint, enable CORS

e80afe6

Add /swagger-ui path which would redirect users to the online SwaggerUI instance with the app's OpenAPI spec loaded and enable CORS so they could access their endpoints from it.

b-admike force-pushed the feat/add-cors branch from b134edb to e80afe6 Compare September 15, 2017 18:52

b-admike merged commit 07b025f into master Sep 15, 2017

b-admike deleted the feat/add-cors branch September 15, 2017 20:12

b-admike removed the review label Sep 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CORS headers for swagger/openapi endpoints so that the spec can be rendered with petstore.swagger.io #570

Add CORS headers for swagger/openapi endpoints so that the spec can be rendered with petstore.swagger.io #570

b-admike commented Sep 13, 2017 •

edited

Loading

bajtos left a comment

bajtos Sep 13, 2017

b-admike Sep 13, 2017

bajtos Sep 13, 2017

bajtos commented Sep 13, 2017

b-admike commented Sep 13, 2017

raymondfeng Sep 13, 2017

b-admike Sep 13, 2017

raymondfeng Sep 13, 2017

b-admike Sep 13, 2017 •

edited

Loading

kjdelisle Sep 13, 2017

raymondfeng Sep 13, 2017 •

edited

Loading

b-admike Sep 13, 2017

raymondfeng Sep 13, 2017

bajtos Sep 14, 2017 •

edited

Loading

bajtos commented Sep 14, 2017

b-admike commented Sep 14, 2017

raymondfeng Sep 14, 2017

kjdelisle Sep 14, 2017

kjdelisle left a comment

virkt25 Sep 15, 2017

raymondfeng Sep 15, 2017

b-admike Sep 15, 2017

bajtos Sep 15, 2017

b-admike Sep 15, 2017 •

edited

Loading

bajtos Sep 15, 2017

b-admike Sep 15, 2017

bajtos Sep 15, 2017

bajtos Sep 15, 2017

bajtos Sep 15, 2017

b-admike Sep 15, 2017

Add CORS headers for swagger/openapi endpoints so that the spec can be rendered with petstore.swagger.io #570

Add CORS headers for swagger/openapi endpoints so that the spec can be rendered with petstore.swagger.io #570

Conversation

b-admike commented Sep 13, 2017 • edited Loading

Description

Related issues

Checklist

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Sep 13, 2017

b-admike commented Sep 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-admike Sep 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng Sep 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

bajtos commented Sep 14, 2017

b-admike commented Sep 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjdelisle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-admike Sep 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-admike commented Sep 13, 2017 •

edited

Loading

b-admike Sep 13, 2017 •

edited

Loading

raymondfeng Sep 13, 2017 •

edited

Loading

bajtos Sep 14, 2017 •

edited

Loading

b-admike Sep 15, 2017 •

edited

Loading