-
Notifications
You must be signed in to change notification settings - Fork 45
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
backend: Autogenerate default oidc-valid-redirect-urls #541
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good to be clarified, yet the string can be improved as it's more suggestive than it should, IMO. How about:
OIDC valid Redirect URLs; accepts comma separated values and supports wildcards (*), for example http://nebraska.example.io/*
This patch improves the help string of oidc-valid-redirect-urls by adding explanation for wildcard support. Signed-off-by: Santhosh Nagaraj S <[email protected]>
64a9c73
to
38e6249
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a small comment which I think will improve the code.
backend/cmd/nebraska/nebraska.go
Outdated
@@ -62,15 +62,15 @@ var ( | |||
oidcClientID = flag.String("oidc-client-id", "", "OIDC client ID used for authentication") | |||
oidcClientSecret = flag.String("oidc-client-secret", "", fmt.Sprintf("OIDC client Secret used for authentication; can be taken from %s env var too", oidcClientIDEnvName)) | |||
oidcIssuerURL = flag.String("oidc-issuer-url", "", fmt.Sprintf("OIDC issuer URL used for authentication;can be taken from %s env var too", oidcClientSecretEnvName)) | |||
oidcValidRedirectURLs = flag.String("oidc-valid-redirect-urls", "http://localhost:8000/*", "OIDC valid Redirect URLs accepts comma separated values with wildcard *, for example if nebraska is hosted at http://nebraska.kinvolk.io the value should be http://nebraska.kinvolk.io/*") | |||
oidcValidRedirectURLs = flag.String("oidc-valid-redirect-urls", fmt.Sprintf("%s/*", *nebraskaURL), "OIDC valid Redirect URLs; accepts comma separated values and supports wildcards (*), for example http://nebraska.example.io/*. If not set defaults to <nebraska-url>/*") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's easier if you set the oidcValidRedirectURLs as ""
because this way you know already that it's not been set.
The docs make it explicit what it does if the user doesn't override it.
backend/cmd/nebraska/nebraska.go
Outdated
@@ -165,6 +165,16 @@ func mainWithError() error { | |||
|
|||
url.Path = "/login/cb" | |||
|
|||
if (*nebraskaURL != "http://localhost:8000") && (*oidcValidRedirectURLs == "http://localhost:8000/*") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the oidcValidRedirectURLs
as an empty string by default, this check becomes much simpler: if *oidcValidRedirectURLs == ""
This patch auto generates the default OIDC valid redirect URLs from the nebraska-url arg if the oidc-valid-redirect-urls is not provided``. Signed-off-by: Santhosh Nagaraj S <[email protected]>
38e6249
to
b2d08f5
Compare
This patch improves the help string of
oidc-valid-redirect-urls by adding explanation
for wildcard support.
Signed-off-by: Santhosh Nagaraj S [email protected]