Skip to content
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.

Sign up for GitHub

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

Err code 501 504 #725

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Err code 501 504 #725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7268276
A backchannel logout request shall according to the spec except 501 a…
rohe Dec 5, 2019
4019a87
isort, blacken, mypy and pylama changes.
rohe Dec 5, 2019
a8fc292
Added PR to CHANGELOG.md
rohe Dec 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@ The format is based on the [KeepAChangeLog] project.

### Added
- [#719] Add support for JWT registration tokens
- [#725] Accepting 501 and 504 responses on backchannel logout request

[#719]: https://github.com/OpenIDC/pyoidc/pull/719
[#725]: https://github.com/OpenIDC/pyoidc/pull/725

## 1.1.2 [2019-11-23]

Expand Down
5 changes: 5 additions & 0 deletions src/oic/oic/provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2298,6 +2298,11 @@ def do_verified_logout(

if res.status_code < 300:
logger.info("Logged out from {}".format(_cid))
elif res.status_code in [501, 504]:
logger.info(
Copy link
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
Collaborator 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
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.

"Received a %s error, which is OK according to the spec",
res.status_code,
)
else:
_errstr = "failed to logout from {}".format(_cid)
if self.events:
Expand Down
50 changes: 50 additions & 0 deletions tests/test_oic_provider_logout.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1085,3 +1085,53 @@ def test_logout_from_clients_one_without_logout_info(self):
assert set(res.keys()) == {"back_channel", "front_channel"}
assert set(res["back_channel"].keys()) == {"number5"}
assert set(res["front_channel"].keys()) == {"number5"}

def test_do_bc_logout_501_response(self):
self._code_auth()

# client0
self.provider.cdb["number5"][
"backchannel_logout_uri"
] = "https://example.com/bc_logout"
self.provider.cdb["number5"]["client_id"] = "number5"

try:
del self.provider.cdb["number5"]["frontchannel_logout_uri"]
except KeyError:
pass

# Get a session ID, anyone will do.
# I know the session backend DB is a DictSessionBackend so I can use that
_sid = list(self.provider.sdb._db.storage.keys())[0]

with responses.RequestsMock() as rsps:
rsps.add(rsps.POST, "https://example.com/bc_logout", status=501)
res = self.provider.do_verified_logout(_sid, "number5", alla=True)

# Accepted the 501
assert set(res.keys()) == {"cookie"}

def test_do_bc_logout_504_response(self):
self._code_auth()

# client0
self.provider.cdb["number5"][
"backchannel_logout_uri"
] = "https://example.com/bc_logout"
self.provider.cdb["number5"]["client_id"] = "number5"

try:
del self.provider.cdb["number5"]["frontchannel_logout_uri"]
except KeyError:
pass

# Get a session ID, anyone will do.
# I know the session backend DB is a DictSessionBackend so I can use that
_sid = list(self.provider.sdb._db.storage.keys())[0]

with responses.RequestsMock() as rsps:
rsps.add(rsps.POST, "https://example.com/bc_logout", status=504)
res = self.provider.do_verified_logout(_sid, "number5", alla=True)

# Accepted the 504
assert set(res.keys()) == {"cookie"}