-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
server: add error HTML templates with error description. #742
Conversation
@@ -683,8 +683,10 @@ func (s *Server) writeAccessToken(w http.ResponseWriter, idToken, refreshToken s | |||
w.Write(data) | |||
} | |||
|
|||
func (s *Server) renderError(w http.ResponseWriter, status int, err, description string) { | |||
http.Error(w, fmt.Sprintf("%s: %s", err, description), status) | |||
func (s *Server) renderError(w http.ResponseWriter, status int, errType string, description string) { |
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.
nit: use inferred type for errType
5a421e2
to
cc218d8
Compare
@@ -294,14 +294,14 @@ func (s *Server) handleConnectorCallback(w http.ResponseWriter, r *http.Request) | |||
identity, err := callbackConnector.HandleCallback(parseScopes(authReq.Scopes), r) | |||
if err != nil { | |||
s.logger.Errorf("Failed to authenticate: %v", err) | |||
s.renderError(w, http.StatusInternalServerError, errServerError, "") | |||
s.renderError(w, http.StatusInternalServerError, errServerError, "Connector callback error") |
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.
Are users going to be expected to know what Connectors are? It seems like we should make this more general.
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.
changed it to "Login error"
cc218d8
to
e70da73
Compare
@@ -138,13 +138,13 @@ func (s *Server) discoveryHandler() (http.HandlerFunc, error) { | |||
func (s *Server) handleAuthorization(w http.ResponseWriter, r *http.Request) { | |||
authReq, err := s.parseAuthorizationRequest(s.supportedResponseTypes, r) | |||
if err != nil { | |||
s.renderError(w, http.StatusInternalServerError, err.Type, err.Description) | |||
s.renderError(w, http.StatusInternalServerError, errServerError, "Failed to parse authorization request") |
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.
Users wont understand what an "authorization request" is. :/
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.
A lot of the existing http errors actually mentioned "authorization request". I'll clean those up too then.
11d1075
to
3809517
Compare
3809517
to
75aa1c6
Compare
lgtm. we can continue improving the exact error messages later. |
Tested the change with the example-app, it successfully displays an error template with error type and error description.
closes #657
@ericchiang