Comment 22 for bug 1175367

In response to Brant Knudson:

> file keystoneclient/middleware/auth_token.py:
> line 848: suggest use "not ..." rather than "is None"
> line 895: suggest use "not ..." rather than "is None"

I'm deliberately using None rather than accepting 0, [], '', etc. because the documentation specifies the way to skip the security strategy is to omit the key. I don't think that an empty string is an acceptable way to specify "no security", and I want this code to fail loudly if it is improperly configured.

> line 860: Not sure what "This" is referring to, is it the memcache_crypt.unprotect_data function or the _cache_get function?
Good point, I'll clarify the point that this is a reminder to the reader that unprotect_data can return None and is not always a json value.

> line 864: no need to include exception string because LOG.exception will print it out.
Good point, I'll fix that.

> line 870: suggest use "not serialized rather than "is None"
This was an abundance of caution about parsing unexpected data. There should be no paths where serialized is not one of None or a valid parsable json, and I'd prefered to have a json decode failure (which makes it obvious that data like 0, '', (), etc. is unexpected) rather than silently returning None. That said, I'm not particularly tied to this use, if you don't buy the argument.

> line 875: comment doesn't make sense since would have returned at 871 if serialized is None.
On the contrary, this is entirely the point. At this point in the code, we have returned if serialized was None (indicating a cache miss or other error), and we know we should be holding a json decodable string. What we know is that the string is NOT the value 'null' which deserializes to None. This comment is asserting that we have not made the common cache/serialization mistake of not differentiating between a stored serialized value of None and a cache miss. This is why we can safely do the "data, expires = cached" expansion a couple lines down.

(note that I'm not wild about the way this works, but I'm fixing the crypto here, not rewriting the caching middleware)

> file keystoneclient/middleware/memcache_crypt.py:
> line 146: Should be "except Exception:", see HACKING.rst
Yep, that was a holdover from the original code. I'll fix it.

> tests/test_memcache_crypt.py:
> line 17: would like to see a test for 2 empty strings and 1 empty one not.
Added.

> line 23: looks like the error message is the length of the MAC? Was the intent to check all 3 are equal?
Good catch, fixed that.

I've attached an updated patch.