make relay_state in IDP response optional #264
Conversation
Required for redirecturl microservice; the redirect handler does not have an opportunity to set the backend relay_state
| 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]: |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
|
Hi guys, I think that we should implement our ideas following the goals related to these. Code style and patterns are very important but they shouldn't block the availability of a feature in a usefull time. We could do refactor and coding style purposes in a second time, we Just have to keep track of them in mailinglist or github. |
|
Travis is fine with py3.6, 3.7 and 3.8. Only py3.5 is failing - it should be removed. |
Important decision. Ubnt 16.04 and debian9 still have py35 as default |
Can you explain why this error exists only in py3.5 and why py3.5 should be removed? |
See travis log, it seems that is something related to socket management through urllib3. Probably the code refactor touched some feature that's not available in py35. I should dug the code, but I think that this the problem |
|
Bytheway removing py35 could be done in a future release, I think that 2021 should be the year to abadon py35 |
There isn't any obvious python 3.5 breaking change in the PR and master branch currently is green for python 3.5. |
|
I agree. I forgot the debian dependency on py3.5 |
|
May be that is a karmic hint that I should appreciate @ioparaskev 's style sugestion ;-) |
|
It seems there is an issue with travis and py3.5 with the requests package (worth investigating why these random failures happen). @c00kiemon5ter re-triggered the build and now everything passes 😉 |
Required for redirecturl microservice because the redirect url handler does not have an opportunity to set the backend relay_state.
(Remark: IIUC satosa only sets a backend relaystate in the AuthnRequest to compare it with the Response, but does nothing else. In the context of SAMl I see no use for the relaystate in the backend)