-
Notifications
You must be signed in to change notification settings - Fork 117
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
runtime-local auth support #4843
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.
Overall looks very good!
cli/pkg/local/app.go
Outdated
http.Error(w, "failed to exchange code for token", http.StatusInternalServerError) | ||
return | ||
} | ||
// save token and redirect to home screen or TODO url provided in initial request i.e. initiateAuthFlow |
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.
Yes it would be nice if we could support a redirect
parameter in initiateAuthFlow
like we do in the admin auth endpoints.
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.
relying on front end to send origin
as query param, if present then redirect back to this url otherwise /
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.
Can we call the parameter redirect=https://...
instead of origin
? Just to align naming with the admin's /auth/login
endpoint:
rill/admin/server/auth/handlers.go
Line 94 in e14e6fe
redirect := r.URL.Query().Get("redirect") |
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.
sure I can change the query param but in the Authenticator it needs to be called this as there is already a redirectURL field for auth flow
~/.rill/credentials.yaml