Skip to content

make relay_state in IDP response optional #264

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

make relay_state in IDP response optional #264

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 6 additions & 5 deletions src/satosa/backends/saml2.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,16 +327,17 @@ def authn_response(self, context, binding):
del self.outstanding_queries[req_id]

# check if the relay_state matches the cookie state
if context.state[self.name]["relay_state"] != context.request["RelayState"]:
satosa_logging(logger, logging.DEBUG,
"State did not match relay state for state", context.state)
raise SATOSAAuthenticationError(context.state, "State did not match relay state")
if self.name in context.state and "relay_state" in context.state[self.name]:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest context.state.get(self.name, {}).get("relay_state") != context.request["RelayState"] to avoid nesting if statements if possible (without hurting readability). It also avoids having to lookup 4 times (vs 2) in the context.state (2 in your first if and another 2 in the second if)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not follow the thinking that nested expressions are better than nested if statements.

I agree in general that using get() is simpler that a verbose 2-part condition. But in this case I think that separating the test for the existence of attribute&key from the comparison of the value does make the intention of this pice of code very explicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the context.state.pop(self.name, None) works outside of the if statement and there is no other action on the else part, you could also have this written as one big if statement
The point of nested expressions vs multiple if statements is that they reduce the cyclomatic complexity by having fewer control flows to follow and to remember while walking down the if tree

My comment was wrong though in that it made it required (while your diff made it optional). So the correct proposal would be if context.state.get(self.name, {}).get("relay_state") not in (None, context.request["RelayState"]):

I think that since there is a check about relay_state existence and a comparison with context.request["RelayState"] in addition to the actions below (the exception) it is as explicit as it gets in either case

Copy link
Contributor Author

@rhoerbe rhoerbe Aug 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Please feel free to modify the PR.

However, I am still not persuaded that concatenated functions (the Caravan Pattern according to Robert C. Martin in his book "Clean Code") has better readability that nested ifs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not going to modify anything since we don't agree

sidenote: I haven't heard of the Caravan Pattern even though I own and have read the book of uncle bob. Tried to look it up but end up with no results. Is it on a newer version maybe?

if context.state[self.name]["relay_state"] != context.request["RelayState"]:
satosa_logging(logger, logging.DEBUG,
"State did not match relay state for state", context.state)
raise SATOSAAuthenticationError(context.state, "State did not match relay state")
context.state.pop(self.name, None)

context.decorate(Context.KEY_BACKEND_METADATA_STORE, self.sp.metadata)
if self.config.get(SAMLBackend.KEY_MEMORIZE_IDP):
issuer = authn_response.response.issuer.text.strip()
context.state[Context.KEY_MEMORIZED_IDP] = issuer
context.state.pop(self.name, None)
context.state.pop(Context.KEY_FORCE_AUTHN, None)
return self.auth_callback_func(context, self._translate_response(authn_response, context.state))

Expand Down