fix redirect url param handling #10
Conversation
|
Requesting review from @JoelSpeed Thanks for pushing this forward! |
|
See the corresponding issue I had opened on the old, dead repo before I realized it was dead for some extra context. This PR not only fixes the user-supplied redirect URL not being honoured, but it also actually uses it for both forming the request to the auth server and for listening for the callback on the appropriate path. In OAuth, "redirect URL/path" and "callback URL/path" are not distinct - the former is how the spec calls it, and the latter is what it colloquially goes by for some people. |
|
Hi @dt-rush, thanks for submitting this PR, I've had a quick look over the PR and initially I agree with the proposed change, I'm at Kubecon this week and would like to have a further look into the code before accepting the PR. I also plan to get #7 merged before accepting any additional PRs so please bear with me on that |
|
@JoelSpeed no sweat! I've used the git diff as a patch for my deployment. |
There was a problem hiding this comment.
I think this is the correct behaviour, looks like the flag's value is just being blatantly ignored at the moment and it's been this way for a very long time looking at the blame. When the proxy-prefix flag was introduced this flag was overwritten and it's been broken since. Thanks for the fix!
|
You'll want to rebase onto master to allow pick up the linting changes that have been merged to fix the CI |
|
Would you please also add a note in the changelog file to mention that you've fixed the behaviour of this flag |
|
@dt-rush Thanks for putting together this PR. This bug is affecting me as well and I'd love to see it merged. @JoelSpeed As a motivated user, is there anything I can do to help get this PR merged? |
clobbered Change-type: patch
dt-rush
force-pushed
the
9-fix-redirect-url-param
branch
from
616d7ca to
d6c239b
Compare
|
@Ghazgkull I just rebased on master. Until this is merged, I've been simply using a patch which applies the changes to the source before building the binary. @JoelSpeed, could we try to merge this now? |
|
(Note: lint is failing in the travis build, but it complains about so many files that I suspect this is a goal for the repo, not a standard which we're currently meeting) |
|
you should change the merge-target branch from "migration" to "master" |
|
One final thing before this gets merged, could you please add a note to the Changelog describing the change you have made (Please see existing entries for the format) |
Change-type: patch
dt-rush
force-pushed
the
9-fix-redirect-url-param
branch
from
d6c239b to
4faab59
Compare
|
@JoelSpeed Looks like @dt-rush submitted new changes which include the Changelog updates. Thanks, folks! |
* Added conditional to prevent user-supplied redirect URL getting clobbered Change-type: patch * use redirectURL as OAuthCallbackURL (as it should be!) Change-type: patch
Add kubecfgapi
* Exported the cookie-name in the values * Add the new field cookieName in the values file * Removed the default value linked to helm release * Update values.yaml Fixing lint error.
See issue #9