Configure a default failure handler for GitHub OAuth #22852
Conversation
Currently we don't have a failure handler registered for GitHub OAuth. That results in customers being dropped off at a plaintext page for actions like canceling their OAuth integration midway[0]. This change fixes a number of paths, but there are others that can lead to the dreaded plaintext that are out of scope for this immediate change. [0] https://sourcegraph.atlassian.net/browse/COREAPP-25
|
Is it ready for review yet? |
|
@unknwon kind of 😄 I'm trying to see how much surgery needs doing from a product perspective, but fundamentally yeah give it a try and see if it avoids the dreaded plaintext final state. |
| @@ -66,7 +66,9 @@ func parseProvider(p *schema.GitHubAuthProvider, db dbutil.DB, sourceCfg schema. | |||
| allowSignup: p.AllowSignup, | |||
| allowOrgs: p.AllowOrgs, | |||
| }, sessionKey), | |||
| nil, | |||
| http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { | |||
| http.Redirect(w, req, "/", http.StatusTemporaryRedirect) | |||
There was a problem hiding this comment.
If we naively redirect... how would we debug/see failures like we have right now (i.e. the ugly plaintext)?
There was a problem hiding this comment.
I found that a number of paths drop the user off in plaintext-land, so your question above is part of why I'm not really sure about this. It may be the case that the ticket needs to be re-scoped and addressed differently.
There was a problem hiding this comment.
Yeah, I just made a comment on JIRA: https://sourcegraph.atlassian.net/browse/COREAPP-25?focusedCommentId=12563, may help you a bit!
|
superseded by #23083 |
Currently we don't have a failure handler registered for GitHub OAuth. That results in customers being dropped off at a plaintext page for actions like canceling their OAuth integration midway[0].
This change fixes a number of paths, but there are others that can lead to the dreaded plaintext that are out of scope for this immediate change.
[0] https://sourcegraph.atlassian.net/browse/COREAPP-25