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

Handle relative urls for oauth authorization #5244

Closed

Conversation

agateblue
Copy link
Contributor

@agateblue agateblue commented Mar 18, 2019

Closes #5243 and #3992

The full URL is computed based on the current selected server
if a relative URL is used as authorizationUrl or tokenUrl

Description

Ensure we support relative authorizationUrl and tokenUrl (relative to the server endpoint) with OAuth.

I'm not sure if this can break backward compatibility, just let me know if you need any change :)

How Has This Been Tested?

  • Unit tests
  • Manually on my personal projects/swagger UI definition
  • With variable in servers urls

Checklist

My PR contains...

  • No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fixes (non-breaking change which fixes an issue)
  • Improvements (misc. changes to existing features)
  • Features (non-breaking change which adds functionality)

My changes...

  • are breaking changes to a public API (config options, System API, major UI change, etc).
  • are breaking changes to a private API (Redux, component props, utility functions, etc.).
  • are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
  • are not breaking changes.

Documentation

  • My changes do not require a change to the project documentation.
  • My changes require a change to the project documentation.
  • If yes to above: I have updated the documentation accordingly.

Automated tests

  • My changes can not or do not need to be tested.
  • My changes can and should be tested by unit and/or integration tests.
  • If yes to above: I have added tests to cover my changes.
  • If yes to above: I have taken care to cover edge cases in my tests.
  • All new and existing tests passed.

@shockey
Copy link
Contributor

shockey commented Mar 20, 2019

@EliotBerriot thanks for this, your implementation is well-architected 👍

Have you tested this with an OpenAPI 2.0 definition? All the oas3Selectors will return null with a 2.0 definition.

@agateblue
Copy link
Contributor Author

thanks @shockey, I'll try that and let you know :)

@agateblue
Copy link
Contributor Author

@shockey I've tested locally with a swagger 2.0 file and the full oauth flow seems to work. I've pushed an additionial safeguard just in case. :)

@agateblue
Copy link
Contributor Author

@shockey is there anything I can do to proceed here? The CI build failed but it was apparently aborted on the jenkins node, and I don't find a way to relaunch it.

@shockey
Copy link
Contributor

shockey commented Apr 10, 2019

@EliotBerriot yep, one sec, I'll trigger it again

@shockey
Copy link
Contributor

shockey commented Apr 10, 2019

please build

@agateblue
Copy link
Contributor Author

it passed 🎉

@hthetiot
Copy link

Rebase? Thank you for PR!
@EliotBerriot

The full URL is computed based on the current selected server
if a relative URL is used as authorizationUrl
or tokenUrl
@agateblue agateblue force-pushed the bug/5243-oauth-relative-url branch from e406de0 to b5b290d Compare June 14, 2019 05:10
@agateblue
Copy link
Contributor Author

@hthetiot: just did that, it should be up to date now :)

@shockey
Copy link
Contributor

shockey commented Jun 24, 2019

bumped into the lack of support for this myself today...

(FYI: we haven't forgotten about this, or any, of our open PRs! we have a big branch we've been working on internally that needs to land on master before we merge more community PRs -- should be resolved in a matter of weeks.)

@ghost
Copy link

ghost commented Dec 17, 2019

Is this going to be merged in anytime soon? Is there a workaround currently?

@FBlass
Copy link

FBlass commented Feb 26, 2020

Any updates on this?

@pszalko
Copy link

pszalko commented Mar 16, 2020

I just want to notice that it was almost a year since the original PR.
Someone put the effort to make the PR, few others are waiting for the PR to be merged, and all we need is just a click on the button from someone in the collaboration team.

Please merge this PR.

@agateblue
Copy link
Contributor Author

Can someone from the team have a look at this please?

@tim-lai
Copy link
Contributor

tim-lai commented Jun 12, 2020

@EliotBerriot Thanks for the PR and patience! Can you rebase and resolve merge conflicts? Once done, should be good to go!

@tim-lai tim-lai self-assigned this Jun 13, 2020
@agateblue
Copy link
Contributor Author

@tim-lai I'm sorry, I currently lack the time and energy to do so. Can someone else take over and finish this?

@tim-lai
Copy link
Contributor

tim-lai commented Oct 21, 2020

merged via PR #6546

@tim-lai tim-lai closed this Oct 21, 2020
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.

Handling of relative urls for Oauth2 authorizationUrl and tokenUrl
7 participants