Skip to content

Err code 501 504#725

Open
rohe wants to merge 3 commits into
masterfrom
err_code_501_504
Open

Err code 501 504#725
rohe wants to merge 3 commits into
masterfrom
err_code_501_504

Conversation

@rohe
Copy link
Copy Markdown
Contributor

@rohe rohe commented Dec 5, 2019

  • Any changes relevant to users are recorded in the CHANGELOG.md.
  • The documentation has been updated, if necessary.
  • New code is annotated.
  • Changes are covered by tests.

According to the backchannel draft a backchannel logout request should accept error codes 501 and 504 beside the normally accepted 200.
https://openid.net/specs/openid-connect-backchannel-1_0.html#BCResponse

@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #725 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #725      +/-   ##
=========================================
- Coverage   63.83%   63.8%   -0.03%     
=========================================
  Files          64      64              
  Lines       11675   11677       +2     
  Branches     2059    2060       +1     
=========================================
- Hits         7453    7451       -2     
- Misses       3649    3653       +4     
  Partials      573     573
Impacted Files Coverage Δ
src/oic/oic/provider.py 74.64% <100%> (+0.03%) ⬆️
src/oic/utils/authn/user.py 63.41% <0%> (-0.82%) ⬇️
src/oic/utils/http_util.py 69.16% <0%> (-0.58%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9623bf4...a8fc292. Read the comment docs.

Comment thread src/oic/oic/provider.py
if res.status_code < 300:
logger.info("Logged out from {}".format(_cid))
elif res.status_code in [501, 504]:
logger.info(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I am not sure how to properly handle this. I do agree that 501 and 504 shouldn't be treated as a complete failure, but it should be at least a partial failure.

501 means that the logout on that provider failed completely
504 means that some other mechanism after the logout failed as well

The log should be higher than info, at least a warning if not error.

If I understand it correctly this shouldn't be logged to the events as a fault?

I am also not sure about not appending some of these to the failed...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry I missed this comment.
I agree it should be regarded as an error, the question is what kind of error.
I also agree that it should be logged as a warning.
It should not be logged to events as a failure as it is not a OP <-> RP communication error. The communication between the OP and the RP was OK it was just that on the RP side the procedure failed.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would think that logging as an error should be fine. Warning the user would be a responsibility of the RP, I guess.

@tpazderka
Copy link
Copy Markdown
Collaborator

@rohe Any chance on finishing this?

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