Skip to content

Use relative protocol rather than fixed for basePath - #95

Closed
billinghamj wants to merge 1 commit into
strongloop:masterfrom
billinghamj:patch-1
Closed

Use relative protocol rather than fixed for basePath#95
billinghamj wants to merge 1 commit into
strongloop:masterfrom
billinghamj:patch-1

Conversation

@billinghamj

Copy link
Copy Markdown

No description provided.

@slnode

slnode commented Apr 13, 2015

Copy link
Copy Markdown

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@bajtos bajtos self-assigned this Apr 16, 2015
@bajtos

bajtos commented Apr 16, 2015

Copy link
Copy Markdown
Member

@slnode ok to test

@bajtos

bajtos commented Apr 16, 2015

Copy link
Copy Markdown
Member

@billinghamj Thank you for the pull request. Could you please explain what problem/use case are you trying to solve? I am not sure if a protocol-relative URL is valid according to Swagger specification, have you checked the spec?

@shelbys the hardcoded protocol was added to support your use case via #61. Will the implementation proposed here work for you too?

@bajtos bajtos added the waiting label Apr 16, 2015
@shelbys

shelbys commented Apr 21, 2015

Copy link
Copy Markdown
Contributor

@bajtos - We've long-since been using x-forwarded-proto in our fork, so this change makes sense to me. See: https://github.com/shelbys/loopback-explorer/blob/master/lib/swagger.js#L119

@billinghamj

Copy link
Copy Markdown
Author

@bajtos The problem for us is when doing reverse proxying, via a system we don't have control over, the x-forwarded-proto header is not correctly set. When we browse to the explorer, the URLs are not correctly generated.

Relative protocols will work in all cases - based on the current browser context.

@bajtos

bajtos commented May 29, 2015

Copy link
Copy Markdown
Member

@billinghamj I see, it makes sense. I am still reluctant to land this patch as it is, as I am concerned that it may break backwards compatibility.

I am proposing the following solution that should allow you to implement what you need, while preserving current behaviour for existing users:

var proto = opts.omitProtocolInBaseUrl  ?  '//' :
  (headers['x-forwarded-proto'] || opts.protocol || req.protocol) + '://';
var host = headers['x-forwarded-host'] || headers.host;
doc.basePath = proto + host + initialPath;

Usage

app.use('/explorer', explorer(app, { omitProtocolInBaseUrl: true }));

Thoughts?

I'll also need you to add a unit-test to verify the new behaviour, see the existing tests for inspiration: https://github.com/strongloop/loopback-explorer/blob/ccfd9c42b08e30e8ea6564735827940c519db308/test/swagger.test.js#L73-L107

@slnode

slnode commented May 29, 2015

Copy link
Copy Markdown

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@bajtos

bajtos commented May 29, 2015

Copy link
Copy Markdown
Member

@slnode test please

@billinghamj

Copy link
Copy Markdown
Author

Yes that works for me.

@bajtos

bajtos commented May 29, 2015

Copy link
Copy Markdown
Member

Yes that works for me.

@billinghamj great! could you then update the pull request please?

@bajtos

bajtos commented Jun 23, 2015

Copy link
Copy Markdown
Member

Closing in favour of #102.

@slnode

slnode commented Jun 23, 2015

Copy link
Copy Markdown

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@billinghamj
billinghamj deleted the patch-1 branch June 23, 2015 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants