fix: resilient social sign in #3011
Conversation
aeneasr
force-pushed
the
fix-status-codes
branch
from
7c814cc
to
79aca99
Compare
aeneasr
force-pushed
the
fix-status-codes
branch
from
79aca99
to
7ffd02c
Compare
Codecov Report
@@ Coverage Diff @@
## master #3011 +/- ##
==========================================
+ Coverage 76.58% 77.52% +0.93%
==========================================
Files 308 310 +2
Lines 19229 19216 -13
==========================================
+ Hits 14727 14897 +170
+ Misses 3401 3181 -220
- Partials 1101 1138 +37
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| @@ -17,3 +25,17 @@ var ( | |||
| ErrAPIFlowNotSupported = herodot.ErrBadRequest.WithError("API-based flows are not supported for this method"). | |||
| WithReasonf("Social Sign In and OpenID Connect are only supported for flows initiated using the Browser endpoint.") | |||
| ) | |||
|
|
|||
| func logUpstreamError(l *logrusx.Logger, resp *http.Response) error { | |||
| if resp.StatusCode == http.StatusOK { | |||
There was a problem hiding this comment.
Why not
| if resp.StatusCode == http.StatusOK { | |
| if resp.StatusCode / 100 == 2 { |
in case the status code is some other 2XX?
There was a problem hiding this comment.
Good point. I also thought of doing 200 >= x < 300 but decided against it. As far as I could tell from my research, all userinfo endpoints will return 200. Once exception might be a 3xx redirect, however that should be handled be the roundtripper. One problem with accepting all 200 is that we also accept 204 (no content) which would bring back the issue of an empty response body. 204 might be emitted when the service is having a disruption or something. So I think it makes sense to explicitly test for 200. For any service that doesn't return 200 but instead idk 201 (doesn't make sense though imo), we could adjust the logic. WDYT?
| } | ||
|
|
||
| l.WithField("response_code", resp.StatusCode).WithField("response_body", string(body)).Error("The upstream OIDC provider returned a non 200 status code.") | ||
| return errors.WithStack(herodot.ErrInternalServerError.WithReasonf("OpenID Connect provider returned a %d status code but 200 is expected.", resp.StatusCode)) |
There was a problem hiding this comment.
| return errors.WithStack(herodot.ErrInternalServerError.WithReasonf("OpenID Connect provider returned a %d status code but 200 is expected.", resp.StatusCode)) | |
| return errors.WithStack(herodot.ErrInternalServerError.WithReasonf("OpenID Connect provider returned a %d status code but 2XX is expected.", resp.StatusCode)) |
aeneasr
force-pushed
the
fix-status-codes
branch
from
a588aa1
to
fc258f8
Compare
Related issue(s)
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further Comments