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

Make Distinction Between "p" and "r" Paths Clearer #84

Closed
9p4 opened this issue Sep 23, 2022 · 5 comments
Closed

Make Distinction Between "p" and "r" Paths Clearer #84

9p4 opened this issue Sep 23, 2022 · 5 comments
Labels
documentation Improvements or additions to documentation enhancement New feature or request good first issue Good for newcomers

Comments

@9p4
Copy link
Owner

9p4 commented Sep 23, 2022

Is your feature request related to a problem? Please describe.
It seems like there isn't enough of a distinction between the p and r paths for initialization and redirection respectively. Perhaps make the different clearer?

Describe the solution you'd like
Replace the p paths with start and r with redirect.

Describe alternatives you've considered
Improve the documentation perhaps?

Additional context
#49
#82
#55
#106

@9p4 9p4 added enhancement New feature or request documentation Improvements or additions to documentation good first issue Good for newcomers labels Sep 23, 2022
@9p4
Copy link
Owner Author

9p4 commented Sep 23, 2022

Whatever solution is created, it should NOT break existing configs. p and r paths should still work, but the documentation should all point to the new paths.

@esmondmissen
Copy link
Contributor

How do we want to handle the redirect url changing. Making the routes have multiple paths is easy but the redirect url needs to be configured on the authentication side.

When we get to the code below we have no way of telling if they have configured their redirect url to /r/ or /redirect/. Could we assume that if they use /start/ we will use /redirect/ and if they use /p/ we will use /r/?

var options = new OidcClientOptions
            {
                Authority = config.OidEndpoint?.Trim(),
                ClientId = config.OidClientId?.Trim(),
                ClientSecret = config.OidSecret?.Trim(),
                RedirectUri = GetRequestBase() + "/sso/OID/r/" + provider,
                Scope = string.Join(" ", config.OidScopes.Prepend("openid profile")),
            };

@esmondmissen
Copy link
Contributor

Something like this:

RedirectUri = GetRequestBase() + $"/sso/OID/{(Request.Path.Value.Contains("/start/", StringComparison.InvariantCultureIgnoreCase) ? "redirect" : "r")}/" + provider,

@9p4
Copy link
Owner Author

9p4 commented Nov 8, 2022

I guess there isn't any other option if the auth provider has a whitelist of URLs to redirect to

9p4 added a commit that referenced this issue Aug 6, 2023
@9p4
Copy link
Owner Author

9p4 commented Aug 6, 2023

Fixed in defab9c

@9p4 9p4 closed this as completed Aug 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants