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.

Sign up for GitHub

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

Jump to bottom

fix: make rest-explorer aware of openapi server root path #2293

Closed
wants to merge 1 commit into from
Closed

fix: make rest-explorer aware of openapi server root path #2293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions packages/rest-explorer/src/rest-explorer.controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const template = fs.readFileSync(indexHtml, 'utf-8');
const templateFn = ejs.compile(template);

export class ExplorerController {
private serverRootPath: string;
private openApiSpecUrl: string;

constructor(
Expand All @@ -29,11 +30,12 @@ export class ExplorerController {
@inject(RestBindings.Http.REQUEST) private request: Request,
@inject(RestBindings.Http.RESPONSE) private response: Response,
) {
this.serverRootPath = this.getServerRootPath(restConfig);
this.openApiSpecUrl = this.getOpenApiSpecUrl(restConfig);
}

indexRedirect() {
this.response.redirect(301, this.request.url + '/');
this.response.redirect(301, this.serverRootPath + this.request.url + '/');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if inferring from openapi.servers[0].url is a feasible solution. Please note that LoopBack app is not aware how its endpoints are proxies/exposed. The same endpoint can be possibly exposed as many different urls, such as:

When it comes to redirect via Location header, my understanding is that proxy/load balancer is responsible for rewriting Location to match its clients.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The redirect could be made to depend on the proxy (though that requires config on the proxy). But the same question would then apply to the /openapi.json reference too.

I was concerned about this in my original bug report, but @bajtos had a different perspective.

A brainstorm: I think LB2 dealt with this by having the explorer be the one that serves the openapi.json, at least when "talking to itself". This could be replicated within rest-explorer with a little work I think, though it would require the RestServer to expose _serveOpenApiSpec as a public method. But I think then the explorer could be made to use exclusively relative URLs, avoiding the path rewriting issue in a more general way?

Copy link
Contributor

@raymondfeng raymondfeng Jan 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some references:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should set Location to a url based on the request - https://github.com/strongloop/loopback-next/blob/master/packages/rest/src/rest.server.ts#L429.

Copy link
Contributor

@raymondfeng raymondfeng Jan 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we set Location to a relative url, would it prevent reverse proxies from rewriting it correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping that using a relative Location url would mean the proxy doesn't need to rewrite it :)

Copy link
Member

@bajtos bajtos Jan 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that this.request.url contains only the path portion of the requested URL as seen by the LB4 server, after it has been rewritten by the reverse proxy. IMO, the current implementation is correct, it's redirecting from /explorer to /explorer/index.html.

I am not able to fully understand implications of the change proposed here. More importantly, I don't understand why this particular change is needed.

Can we revert it please?

}

index() {
Expand All @@ -48,13 +50,24 @@ export class ExplorerController {
.send(homePage);
}

private getServerRootPath(restConfig: RestServerConfig): string {
const openApiConfig = restConfig.openApiSpec || {};
const servers = openApiConfig.servers || [];
let url = servers[0] ? servers[0].url : '';
// trim trailing '/' so we can safely append rooted paths to it
if (url.endsWith('/')) {
url = url.substring(0, url.length - 1);
}
return url;
}

private getOpenApiSpecUrl(restConfig: RestServerConfig): string {
const openApiConfig = restConfig.openApiSpec || {};
const endpointMapping = openApiConfig.endpointMapping || {};
const endpoint = Object.keys(endpointMapping).find(k =>
isOpenApiV3Json(endpointMapping[k]),
);
return endpoint || '/openapi.json';
return this.serverRootPath + (endpoint || '/openapi.json');
}
}

Expand Down
52 changes: 45 additions & 7 deletions packages/rest-explorer/test/acceptance/rest-explorer.acceptance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,6 @@ describe('API Explorer (acceptance)', () => {
const body = response.body;
expect(body).to.match(/^\s*url: '\/apispec',\s*$/m);
});

async function givenAppWithCustomRestConfig(config: RestServerConfig) {
app = givenRestApplication(config);
app.component(RestExplorerComponent);
await app.start();
request = createRestAppClient(app);
}
});

context('with custom RestExplorerConfig', () => {
Expand Down Expand Up @@ -109,8 +102,53 @@ describe('API Explorer (acceptance)', () => {
}
});

context('with custom server root', async () => {
beforeEach(async () => {
await givenAppWithCustomRestConfig({
openApiSpec: {
servers: [{url: '/external/proxy/root'}],
},
});
});

it('prepends server root path to OpenAPI path', async () => {
const response = await request.get('/explorer/').expect(200);
const body = response.body;
expect(body).to.match(
/^\s*url: '\/external\/proxy\/root\/openapi.json',\s*$/m,
);
});

it('prepends server root path to /explorer redirect', async () => {
await request
.get('/explorer')
.expect(301)
.expect('location', '/external/proxy/root/explorer/');
});

it('handles server root with trailing slash', async () => {
await givenAppWithCustomRestConfig({
openApiSpec: {
servers: [{url: '/external/proxy/root/'}],
},
});
const response = await request.get('/explorer/').expect(200);
const body = response.body;
expect(body).to.match(
/^\s*url: '\/external\/proxy\/root\/openapi.json',\s*$/m,
);
});
});

function givenRestApplication(config?: RestServerConfig) {
const rest = Object.assign({}, givenHttpServerConfig(), config);
return new RestApplication({rest});
}

async function givenAppWithCustomRestConfig(config: RestServerConfig) {
app = givenRestApplication(config);
app.component(RestExplorerComponent);
await app.start();
request = createRestAppClient(app);
}
});