authenticatable_salt if statement not processing.

[mattbaumann1]

I have some odd behavior when setting the saml_session_index_key. I am not sure if this is the best place to ask about this, please feel free to close if this is inappropriate.

We are currently using the fork for PR #149 because we need to have the SP initiated logout. I have some other issues which I need to put up my own PR for, but the specific issue here is that we are trying to store the session_index as a virtual attribute instead of in the database. I have been able to add it to our user model, but if fails to recognize the signed in user even though the session is created and the sign_in was successful.

We have traced this back to the authenticatable_salt method in devise_saml_authenticatable/lib/devise_saml_authenticatable/model.rb. Here is the crazy thing, if I add a puts statement with a copy of line 25 after it, it works.

puts "#{self.send(Devise.saml_session_index_key)}"

or if I add byebug after line 25 and simply continue through, it also works.

Any it would work in these instances but not normally.

As an aside, I also tried just adding a return to the line, but that did not allow it to process.

Thanks again for any help.

[adamstegman]

What do you mean by a virtual attribute? Like an attr_accessor? I'm confused how that would be affected by puts or byebug.

You may be able to just remove it though—without persistence, there's no point in using the property, because IdP-initiated logout won't work.

[mattbaumann1]

Exactly, using a attr_accessor instead of storing it in the database.

If it gets stored into the database and you open two browsers with two separate sessions, it overwrites the first session and you lose the ability to logout of that first session at the IdP level.

Also, you understand my confusion of why I think it seems crazy that the puts or byebug make a difference. I can't explain it, but it seems to hold.

We need to be able to store the session_index, because our IdP requires it for logout.

So ultimately, I am really confused by this behavior also. Thanks for again joining me in our trip through this gem.

[adamstegman]

If it gets stored into the database and you open two browsers with two separate sessions, it overwrites the first session and you lose the ability to logout of that first session at the IdP level.

Yeah, @BSorbus recently brought this up as well. I agree, this was naïve and should really have a separate table rather than one column on the users model. Are you solving this by storing it in the session? That could also work.

It must be stored somewhere persistent besides the attr_accessor, so is it possible the value wasn't present when the gem checked for it?

[mattbaumann1]

So. This seems to fix it although I need to look at it more closely to see why. Changing the send to read_attribute works.

self.read_attribute(Devise.saml_session_index_key)

[mattbaumann1]

I believe the attr_accessor puts it into the session. Right now we are over riding the method in our user model, but I think changing the send to a read_attribute fixes our problem. So for us to be able to log out of the IdP from our SP, we needed to make this change (over ride the authenticatable_salt method and add an attr_accessor for session_index on the user), write a custom idp_entity_id_reader (#117), and be on the fork associated with PR #149. I need to do some clean up, but I am hoping to submit some pull requests for these changes because I think they will benefit other people that are trying to sign out of an IdP.

[BSorbus]

Hi,
I thing a better options:

rails g migration RemoveSesisonIndexFromUser session_index:string
rails g model UserSessionsIndex session_index:string user:references

…reconstruction a gem and work with many "session_index"

[adamstegman]

Changing the send to read_attribute works.

That's really interesting, because read_attribute pulls the value from the @attributes instance variable, seemingly ignoring your attr_accessor. I'm guessing that in this case, read_attribute is the inverse of update_attributes which the gem uses to set the value. Good catch!