Skip to content

Conversation

peppelinux
Copy link
Member

This PR fixes this frequent error messages in the logs:

[2019-07-09 12:53:30] [ERROR]: No cookie named SATOSA_STATE in 
Traceback (most recent call last):
  File "/opt/django-saml2-idp.env/lib/python3.7/site-packages/satosa/state.py", line 74, in cookie_to_state
    state = State(cookie[name].value, encryption_key)
KeyError: 'SATOSA_STATE'

Also related to #236

All Submissions:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what problem you are trying to solve with this PR?
  • Have you added information on what your changes do and why you chose this as your solution?
  • Have you written new tests for your changes?
  • Does your submission pass tests?
  • This project follows PEP8 style guide. Have you run your code against the 'flake8' linter?

@peppelinux peppelinux force-pushed the no_cookie_name_SATOSA_STATE branch from 81b9ce4 to 59d8828 Compare July 12, 2019 14:09
@peppelinux
Copy link
Member Author

peppelinux commented Jul 12, 2019

Three test failed, like:

TestStateAsCookie.test_cookie_to_state_handle_broken_cookies[Set-Cookie: state_cookie="_Td6WFoAAATm1rRGAgAhARYAAAB0L-WjAQCXYWt4NU9ZLWF5amdVVDdSUjhWdnkyUHE5MFhJV0J4Uzg5di1EVW1nNTR0WHZKakFsaWJmN2JMOUtlNEltMkJ0dmxOakRyUDJXZE53d0dwSGNqYnBzVng5YjVVeUYyUzkwcWVSMU42U2VNNHZDQTktUXdCQWx0WUh6LVBPX1pBYnZ1M1RsV09Qc2lKS3VpelB5a0FsMG93PT0AmlSCX0Pk2WoAAbABmAEAAGRNyZ2xxGf7AgAAAAAEWVo="; Max-Age=600; Path=/; Secure-state_cookie-wrong_encrypt_key-Exception]

because with this fix the unexistent cookies or which do not have the name SATOSA_STATE (or whatever name configured in proxy_conf) will be simply ignored.

Decide if change the unit tests to handle the null cookie as valuable or remove these unit tests.

@c00kiemon5ter
Copy link
Member

c00kiemon5ter commented Aug 27, 2019

I opted to remove logging, as it doesn't seem to be of much value.
Instead just log what was finally loaded.
Fixed by fcd61a2 and dcec6e5

The cookie_to_state(cookie_str, name, encryption_key) function is expected to load data from a cookie given a name. If that is not found, it should error out, as it already does. Defaulting to another value is not correct; it is unexpected. The call should be notified that what they asked is not available and then they should be the ones to handle what should happen.
In our case the caller is _load_state and for that caller not finding the given name is fine, as it can fallback to an empty State(). For other callers this is not guaranteed and we should not make such a decision for everyone in cookie_to_state.

@peppelinux
Copy link
Member Author

Ok, I'd preferred a commit on top of mine Just to track my contributions but you're the boss and everything Is allright. Thank you

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.

2 participants