Resolving NameID

[astoltz]

I was having the same issues as https://bitbucket.org/lgs/djangosaml2/issues/42/attributeerror-nonetype-object-has-no on the latest version of this plugin. I'm testing on Django 1.9.4.

With the proposed pull request, I got both SAML Login and Logout to work properly.

[knaperek]

Thanks for the work, but let's investigate this a bit further.

First, everything was (or at least seemed to be) working until v4.0.3 of pysaml2, which introduced regression in djangosaml2 code. @mheijink noticed it and pushed a fix into pysaml2 that fixes this (it's just not yet released).

But, anyway, this might not be over.
Apparently, the issue was caused by inconsistency in the content of session data under the key name_id. Both djangosaml2 and saml2 use this key for storing some value. The question is, if this is a pure coincidence (just a name clash), or djangosaml2 purposely accesses that name_id session variable.
To me, the first option seems more probable, but I'm inviting you to prove me wrong :-)
If it really happens to be a name clash, then we should use different key in djangosaml2, e.g. authenticated_name_id instead of name_id.

[mheijink]

Hi Jozef,

I don’t think that renaming the key will help you because pysaml2 then aleady has changed the data session_info[‘name_id’]

A solution would be if pysaml2 would store the coded data in another key, so the source isn’t changed.
For example:

session_info['coded_name_id'] = code(name_id)

instead of

session_info['name_id'] = code(name_id)
Marcel

Van: Jozef [mailto:[email protected]]
Verzonden: donderdag 24 maart 2016 09:48
Aan: knaperek/djangosaml2 [email protected]
CC: Marcel Heijink [email protected]
Onderwerp: Re: [djangosaml2] Resolving NameID (#4)

Thanks for the work, but let's investigate this a bit further.

First, everything was (or at least seemed to be) working until v4.0.3 of pysaml2, which introduced regression in djangosaml2 code. @mheijinkhttps://github.com/mheijink noticed it and pushed a fixhttps://github.com/rohe/pysaml2/commit/1f16f68c5bb9da40b522ff2ad9b175d512890bb4 into pysaml2 that fixes this (it's just not yet released).

But, anyway, this might not be over.
Apparently, the issue was caused by inconsistency in the content of session data under the key name_id. Both djangosaml2 and saml2 use this key for storing some value. The question is, if this is a pure coincidence (just a name clash), or djangosaml2 purposely accesses that name_id session variable.
To me, the first option seems more probable, but I'm inviting you to prove me wrong :-)
If it really happens to be a name clash, then we should use different key in djangosaml2, e.g. authenticated_name_id instead of name_id.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//pull/4#issuecomment-200738696

[knaperek]

I think you might have missed my point, so please let me rephrase.

The first thing I dislike here, just by looking at it from the highest level and not investigating the actual internals at this point, is the mere fact that the two libraries (djangosaml2 and pysaml2) use the same key in session to both store and retrieve some data. Now, in an ideal situation, I'd imagine to have a single routine that would be handling such cases, that is reading and writing this particular information into the correct location. Unless...

Unless the design is actually OK in this case, and it is just a bad luck and a terrible coincidence that both libraries use the same key to store their data in the session. And, by even bigger coincidence, this has been (or seemed to be) working so far because those data were actually of the same (or similar/compatible) meaning and in the same encoding.

To be specific, it seems that both libraries were storing Name-ID to the session at some point, apparently after successful authentication. I don't have time to review it right now, but it's my gut feeling.

It only broke now that pysaml2 changed the encoding (in some cases, which btw. seems quite strange and inconsistent to me).

I don't find that switching-by-isinstance approach very Pythonic.
We should not be "guessing" the contents format, that's a rather dangerous path that quickly leads to slippery slope. I think it should be obvious what goes there - be it an object, or a string - but consistently!

And, actually, what you have proposed is technically equivalent with my proposition. I just said we should perhaps use different key in the session, you suggested pysaml2 should use a different key - both solution aiming for the same thing - avoiding the key name clash between the libraries.

So, in my opinion, what's left is to investigate if this really was just a mistake and the original djangosaml2 author didn't realize this session key is already used by pysaml2, or it was his intention to use and/or even somehow override pysaml2 behavior by changing it on the fly. (I'm saying this without any deeper analysis of the code, just purely from the formal standpoint).

[astoltz]

If it helps, the isinstance logic was taken from the parent function, so it's not really inconsistent. The behavior I was seeing was a double encoding, so simply making sure that only happened once worked for me.

[knaperek]

@astoltz Storing different data under the same session key is inconsistent, no matter how many people or libraries do it, period. In our case I was not referring to inconsistency with the pysaml2 library, but rather an inner inconsistency (which obviously pysaml2 suffers from also). But this issue is not about that - let's not discuss the formal correctness of an external library now unless it becomes non-functioning, which has not been proved yet.

At this point I'm more concerned about the mere fact that we set and retrieve data to/from pysaml2 internal cache variable. If it wasn't for this, this whole thing would have never happened, no matter how crazy encoding patterns pysaml2 starts to employ.

[jimr]

It looks like the break was introduced here: IdentityPython/pysaml2@0515de9#diff-77a71a585af5a3cfaefa180fd9b8341f with the hugely unhelpful commit title of "language correction".

Basically, it seems that get and set on IdentityCache aren't necessary any more, since the name_id caching is taken care of in pysaml2.

(edited for correction)

[jimr]

I've opened #6 as an alternative fix for this issue. We've tested it in our environment and it seems to work fine.

[knaperek]

Thanks @jimr, I've manually merged your PR.