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

feat(swagger): handle redirects from OIDC IdP and initialize swagger #467

Merged
merged 4 commits into from
Mar 22, 2024

Conversation

andrewwylde
Copy link
Contributor

This is related to this PR to investigate enabling some of the methods available for OAuth2 Try It Out Authorization in swagger-ui using our spec renderer.

Please see https://gist.github.com/andrewwylde/d47c94c7629a81a0232ac80ef862e788 for setup.

@andrewwylde andrewwylde added the do not merge Do not merge this pull request until this label is removed. label Mar 15, 2024
@andrewwylde andrewwylde requested a review from a team as a code owner March 15, 2024 15:22
@andrewwylde andrewwylde removed the do not merge Do not merge this pull request until this label is removed. label Mar 20, 2024
@425devon 425devon force-pushed the wip/oauth2-redirect branch from 4cde51d to b5d84f5 Compare March 21, 2024 18:49
devonl-kong
devonl-kong previously approved these changes Mar 21, 2024
@andrewwylde andrewwylde changed the title refactor: utilize preview build to test proof-of-concept feat(swagger): handle redirects from OIDC IdP and initialize swagger Mar 21, 2024
const oauth2 = window.opener?.swaggerUIRedirectOauth2
const sentState = oauth2.state
const redirectUrl = oauth2.redirectUrl
let qp
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to use this.$route.query to get the params?

Comment on lines 317 to 326
const arr = qp.split('&')

arr.forEach(function (v, i, _arr) { _arr[i] = '"' + v.replace('=', '":"') + '"' })
qp = qp
? JSON.parse('{' + arr.join() + '}',
function (key, value) {
return key === '' ? value : decodeURIComponent(value)
}
)
: {}
Copy link
Contributor

Choose a reason for hiding this comment

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

This algorithm won't be able to handle params that don't have a value (e.g., query params strings like a=z&x).

I think it would be better to have an algorithm like

Suggested change
const arr = qp.split('&')
arr.forEach(function (v, i, _arr) { _arr[i] = '"' + v.replace('=', '":"') + '"' })
qp = qp
? JSON.parse('{' + arr.join() + '}',
function (key, value) {
return key === '' ? value : decodeURIComponent(value)
}
)
: {}
qp = Object.fromEntries(qp.split('&').map(param => param.split('=')))

or if possible use another way of accessing query params on the route as Nate commented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah makes sense, this was a direct lift from the helper functions in swagger-ui. I'll port it over to be more vue-friendly and hopefully catch more edge cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does this look, @Mierenga ? I think this should pretty safely account for both cases as well as empty

// called with $route.hash or $route.query
const parseRouteData = (routeData: string | LocationQuery): Record<string, string> => {
  if (!routeData) return {};

  // handle locationQuery
  if (typeof routeData === 'object') {
    // decode any URI components:
    const routeDataDecoded = Object.keys(routeData).reduce((acc, key) => {
      acc[key] = decodeURIComponent(routeData[key].toString())
      return acc
    }, {})
    return routeDataDecoded
  }

  const data = routeData.startsWith('#') ? routeData.substring(1) : routeData;
  const pairs = data.split('&');
  const result = {};

  pairs.forEach(pair => {
    const [key, value] = pair.split('=');
    result[key] = decodeURIComponent(value);
  });

  return result;
};

Meaning that:

window.open('http://localhost:5173/spec/8c655d31-0c89-4660-96d0-829dd92f9e98/oauth2-redirect.html?a=z&x')

const routeHashData = parseRouteData($route.hash);
const routeQueryData = parseRouteData($route.query);
const routeData = { ...routeHashData, ...routeQueryData };

// routeData becomes `{a:'z',x:''}`

@andrewwylde
Copy link
Contributor Author

OK, should be ready for re-review @Mierenga @nateslo @425devon

Copy link
Contributor

@Mierenga Mierenga left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for clarifying things with the refactor

@andrewwylde andrewwylde merged commit 27a830f into main Mar 22, 2024
7 checks passed
@andrewwylde andrewwylde deleted the wip/oauth2-redirect branch March 22, 2024 18:55
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