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

signinRedirectCallback should accept a relative path #534

Closed
ydarma opened this issue May 18, 2022 · 3 comments · Fixed by #535
Closed

signinRedirectCallback should accept a relative path #534

ydarma opened this issue May 18, 2022 · 3 comments · Fixed by #535
Assignees
Labels
bug Something isn't working help wanted Extra attention is needed
Milestone

Comments

@ydarma
Copy link
Contributor

ydarma commented May 18, 2022

Hi,

first THANK YOU for this port. I would like to report a small inconvenience.

    signinRedirectCallback(url?: string): Promise<User>;

only accepts full URL, including scheme and host.

Vue router guards (i.e. middleware) parameters only gives the relative path (cf. https://router.vuejs.org/api/#fullpath). Scheme and host are never provided by Vue router in order to enforce portability across environments.

I had to change from :

function endLogin(to, from, next, userManager) {
  return userManager
    .signinRedirectCallback(to.fullPath)
    .then(user => ...)
}

To something like

function endLogin(to, from, next, userManager) {
  const origin = new URL(process.env.VUE_APP_CALLBACK_ROOT_URL).origin;
  const fullUrl = origin + to.fullPath;
  return userManager
    .signinRedirectCallback(fullUrl)
    .then(user => ...)
}

where VUE_APP_CALLBACK_ROOT_URL is an environment variable also used for UserManager({ redirect_uri }) setting. I found the former more readable.

Thanks again

@ydarma
Copy link
Contributor Author

ydarma commented May 18, 2022

for information, this comes from https://github.com/authts/oidc-client-ts/blob/main/src/utils/UrlUtils.ts, line 9 :

        const parsedUrl = new URL(url);

@pamapa
Copy link
Member

pamapa commented May 18, 2022

I am using hard-coded (per environment) full paths.

According to documentation new URL(url) should handle relative paths (https://developer.mozilla.org/en-US/docs/Web/API/URL/URL), when we would provide a base.

Probably: const baseUrl = window.location.origin; ... new URL(url, baseUrl); does the trick? Can you provide a working/tested MR?

@pamapa pamapa added bug Something isn't working help wanted Extra attention is needed labels May 18, 2022
@ydarma
Copy link
Contributor Author

ydarma commented May 18, 2022

In AbstractChildWindow.ts it may happen that an undefined url is passed to readParams which is catched and rethrown as "Invalid response from windows". When a base is passed to URL constructor an undefined url becomes valid as first parameter. Thus I suggest :

        if (!url) throw "Invalid URL";
        const parsedUrl = new URL(url, window.location.origin);

All tests pass. I'll push a MR.

ydarma pushed a commit to ydarma/oidc-client-ts that referenced this issue May 18, 2022
@ydarma ydarma mentioned this issue May 18, 2022
2 tasks
@pamapa pamapa added this to the 2.0.5 milestone May 18, 2022
pamapa pushed a commit that referenced this issue May 19, 2022
* origin based URL fix #534
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants