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

Fix the OIDC login button when using a RoutePrefix. #4022

Merged
merged 1 commit into from
Sep 19, 2023
Merged

Conversation

bigkevmcd
Copy link
Contributor

@bigkevmcd bigkevmcd commented Sep 19, 2023

Closes #4016

What changed?
Make the OIDC login button relative so that it uses the route prefix from the HTML base element.

Why was this change made?
To address a bug when logging in with OIDC and a Route Prefix.

How was this change implemented?
Tests.

How did you validate the change?
Setup a login via Dex and it now goes to the correct URL.

Release notes
Allow use of the route prefix mechanism with OIDC authentication.

Documentation Changes

@bigkevmcd bigkevmcd requested a review from foot September 19, 2023 09:02
@bigkevmcd bigkevmcd force-pushed the oidc-sub-path branch 2 times, most recently from 231cc8f to a68b109 Compare September 19, 2023 09:45
@@ -92,7 +92,7 @@ function SignIn({ darkModeEnabled = true }: Props) {

const handleOIDCSubmit = () => {
const CURRENT_URL = window.origin;
return (window.location.href = `/oauth2?return_url=${encodeURIComponent(
return (window.location.href = `./oauth2?return_url=${encodeURIComponent(
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to work. Which makes sense, but I thought I read something to the contrary recently -- that while a, img, script etc tags respect base tags, the JS world (e.g. setting location.href) does not.

However after doing some more testing it seems that it does, and this works fine, even if you end up at localhost:9001/wego/signin/ (with a trailing slash), which still routes correctly, and then logs into OIDC correctly too..

Copy link
Contributor

Choose a reason for hiding this comment

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

JSDOM etc at least don't support navigation so this is a trickier case to test w/out more involved acceptance tests..

Copy link
Contributor

@foot foot left a comment

Choose a reason for hiding this comment

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

💯

Make the OIDC login button relative so that it uses the route Prefix
from the base.
@bigkevmcd bigkevmcd merged commit a738dd2 into main Sep 19, 2023
15 checks passed
@bigkevmcd bigkevmcd deleted the oidc-sub-path branch September 19, 2023 12: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.

Sub-Paths not working with OAuth2
2 participants