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

authn: add support for custom callback url #7

Closed
wants to merge 1 commit into from

Conversation

simaotwx
Copy link

@simaotwx simaotwx commented Mar 1, 2022

@simaotwx simaotwx marked this pull request as ready for review March 2, 2022 15:01
Copy link
Owner

@greenpau greenpau left a comment

Choose a reason for hiding this comment

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

@simaotwx , thank you! 👍 Please add b.Config.JsCallbackURL too and handle the logic there too.

Additionally, how do you deal with

if strings.Contains(r.URL.Path, "-js-callback") {
// Intercept callback with Javascript.
return p.handleJavascriptCallbackIntercept(ctx, w, r)
}

if (i < 0) {
redirectURL = redirectURL.replace('authorization-code-js-callback', 'authorization-code-callback');
window.location = redirectURL;
} else {
ridirectURI = redirectURL.slice(0, i).replace('authorization-code-js-callback', 'authorization-code-callback');
window.location = redirectURI + "?" + redirectURL.slice(i+1);
}

@@ -96,7 +96,16 @@ func (b *Backend) Authenticate(r *requests.Request) error {
zap.String("code", reqParamsCode),
)

reqRedirectURI := reqPath + "/authorization-code-callback"
var reqRedirectURI string
Copy link
Owner

Choose a reason for hiding this comment

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

@simaotwx , please use switch instead of if/else.

@@ -96,7 +96,16 @@ func (b *Backend) Authenticate(r *requests.Request) error {
zap.String("code", reqParamsCode),
)

reqRedirectURI := reqPath + "/authorization-code-callback"
var reqRedirectURI string
if len(b.Config.CallbackUrl) > 0 {
Copy link
Owner

Choose a reason for hiding this comment

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

@simaotwx , please replace CallbackUrl with CallbackURL.

@@ -193,6 +202,12 @@ func (b *Backend) Authenticate(r *requests.Request) error {

if b.Config.JsCallbackEnabled {
Copy link
Owner

Choose a reason for hiding this comment

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

@simaotwx , please use switch instead of if/else.

@greenpau
Copy link
Owner

greenpau commented Mar 5, 2022

@simaotwx , also, please add your CLA consent to this commit.

@greenpau
Copy link
Owner

greenpau commented Mar 5, 2022

@simaotwx , additionally, please add tests.

@greenpau greenpau closed this Jul 18, 2022
@steverweber
Copy link

was this dropped or merged?

@simaotwx
Copy link
Author

@steverweber dropped. Haven't found the time to do all the things mentioned. Nevertheless, the commit works for my use case so you can pick it if you feel like you need it, too.

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.

3 participants