Restored support for NIL as well as empty AttributeValues

[borgand]

@sebastienluquet commented on 3e7a9a9:

I have an issue with this patch. attributes method does not parse this xml any more:

       <ns2:Attribute Name="ATTRIBUTE_NAME">
           <ns2:AttributeValue/>
       </ns2:Attribute>

I expect attributes including

{ "ATTRIBUTE_NAME" => nil }

What I read from SAMLCore:

1. if the SAML attribute exists but has no values, then the <AttributeValue> element MUST be omitted
2. If a SAML attribute includes an empty value, such as the empty string, the corresponding <AttributeValue> element MUST be empty (generally this is serialized as <AttributeValue/>).
3. if a SAML attribute includes a "null" value, the corresponding <AttributeValue> element MUST be empty and MUST contain the reserved xsi:nil XML attribute with a value of "true" or "1".

This pull request restores parsing empty <AttributeValue/> into nil or "" as per specs. Note that (2) above does not define empty string, but currently ruby-saml does not support xsi:type and regards all values as strings, thus empty value essentially means empty string in ruby-saml current implementation.

NB! this does not fully resolve @sebastienluquet's issue, if his XML does not contain the xsi:nil attribute, but it would be against specs to parse that into nil.

[sebastienluquet]

Great work ! Thank you.

I think I will be able to deal with some attributes like { "ATTRIBUTE_NAME" => "" }

[sebastienluquet]

@Lordnibbler Before leaving, I will also need this one :)

[Lordnibbler]

@sebastienluquet @borgand please confirm you want this PR merged

[ridiculous]

I'd vote against the use of extend to decorate the attr_value object because it could be nil, which then effectively defines values and values= on nil. This can cause unwanted side effects throughout the application, since there is only one nil object.

[sebastienluquet]

I'll prefer this one but ridiculous comment suggest a little refactoring

[borgand]

@ridiculous - I hadn't thought about that.

I'll look into it and see if I come up with a better solution. SimpleDelegate comes to mind.

PS. Can I update this pull-request somehow with new code?

[sebastienluquet]

@borgand : Yes you can, Pull Request is auto-magically updated :).

[borgand]

OK, I got this far with SimpleDelegator:

# attribute_value.rb
require 'delegate'
module OneLogin
  module RubySaml

    # Wrapper for AttributeValue with multiple values
    # For most duck typing purposes AttributeValue behaves as the first value type (String, nil, etc)
    # unless specifically asked for (i.e AttributeValue#class would not lie)
    # Use AttributeValue#values to get all values as an array
    class AttributeValue < SimpleDelegator
      def initialize(*args)
        super
        @values = []
      end
      attr_accessor :values

      # Redefine nil? so that we properly respond to actual nil delegate
      def nil?
        __getobj__.send(:nil?)
      end
    end
  end
end

and then in the response.rb

 # Retain first value's type while transporting all values
attr_value = AttributeValue.new values.first
attr_value.values = values.reverse # retain XML order

This passes all tests, including not polluting nil with values and values= and acting like a String or nil when needed, but it breaks badly at the basic truthiness test:

value = AttributeValue.new(nil)
# passes inverted truthiness:
puts !v ? 'falsey' : 'truthy'             # => falsey


# but fails all direct truthiness tests:
v || 1                                   # => nil
v && 1                                   # => 1
puts v ? 'truthy' : 'falsey'             # => truthy
puts "should not" if v                   # => should not

As you can see, we can not do much about the C-level RTEST(obj) that returns true for any kind of object other than nil of false and I believe this is the worst kind of unexpected behavior we could provide.

Anybody have any better ideas?

[borgand]

Seems that this monkey-patching attribute values was not that great idea after all. So I'm reverting back to my initial idea that I did not realize at first -- making response.attributes return an attributes collection that is itself aware of multiple values.

As I understand my previous multi-value pull-request has not been released as gem yet so there should not be too many users whom this will hurt.

response.attributes['mail']
# => '[email protected]'
response.attributes.multi('mail')
# => ['[email protected]', '[email protected]']

As a bonus I added a switch that downstream users can use to turn the backwards compatibility off:

OneLogin::RubySaml::Attributes.single_value_compatibility = false
response.attributes['mail']
# =>  ['[email protected]', '[email protected]']
response.attributes.single('mail')
# => '[email protected]'

This default could be changed at some later time (say 1.0)

[sebastienluquet]

I'm leaving tomorrow :(

[borgand]

@sebastienluquet, would this implementation work for you?

[sebastienluquet]

Actually it create some regression since I expect .attributes to be an Hash and some of my .attributes.with_indifferent_access do not work anymore :(

[borgand]

@sebastienluquet - what do you do with the HashWithIndifferentAccess afterwards? Did you notice this method in the Attributes class:

      # stringifies all names so both 'email' and :email return the same result
      def canonize_name(name)
        name.to_s
      end

This makes Attributes behave almost the same - these would return the same result:

response.attributes['mail’]
response.attributes[:mail]

Does this fit your needs?

I kept from making class Attributes < Hash because Attributes overwrites some of the Hash methods (e.g. [] and []=) and could thus create some subtle bugs.

Actually it create some regression since I expect .attributes to be and Hash and some of my .attributes.with_indifferent_access do not work anymore :(

—
Reply to this email directly or view it on GitHub.

[sebastienluquet]

You're right I did not really need with_indifferent_access. Refactoring ok for me.

Ready to merge :)

[ridiculous]

Seems like this one #137 achieves the same thing with a lot less code changes and no regression worries.

[borgand]

SAML does allow specific nil values so I think maintainers should chip in here which they prefer - simplest fix for (admittedly my introduced) current situation or move for better SAML support with risk for regression.

Of course, with a bit more look into this matter, maybe a Hash subclass would not break things.

[ridiculous]

Maybe an IllegalAttribute error should be raised if that's the case?

[borgand]

Sorry, I don't catch where to raise the error. Can you elaborate?

[ridiculous]

If any empty attribute is given without an appropriate value set for xsi:nil

[borgand]

Then it is to be interpreted as empty string as my implementation should do.

If any empty attribute is given without an appropriate value set for xsi:nil

—
Reply to this email directly or view it on GitHub.

[Lordnibbler]

@borgand @ridiculous please confirm you want this PR reviewed and merged

[Lordnibbler]

@pitbulk @inakidelamadrid @luisvm can you guys review? lgtm 👍

[borgand]

👍 I prefer this over #137 as this follows spec more closely.

[luisvm]

Agree with @borgand as this approach closely follows standards:

    <saml:AttributeStatement>
      <saml:Attribute Name="novalue" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic">
      </saml:Attribute>
      <saml:Attribute Name="blankvalue" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic">
        <saml:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:type="xs:string"/>
      </saml:Attribute>
      <saml:Attribute Name="multiplevalue" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic">
        <saml:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:type="xs:string">test1</saml:AttributeValue>
        <saml:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:type="xs:string">test2</saml:AttributeValue>
      </saml:Attribute>
      <saml:Attribute Name="nilvalue" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic">
        <saml:AttributeValue xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:type="xs:string" xsi:nil="true"/>
      </saml:Attribute>
    </saml:AttributeStatement>

 => #<OneLogin::RubySaml::Attributes:0x007f989a997ed0 @attributes={"novalue"=>[], "blankvalue"=>[""], "multiplevalue"=>["test1", "test2"], "nilvalue"=>[nil]}> 

👍

[Lordnibbler]

@inakidelamadrid @pitbulk @pwnetrationguru please review

[pitbulk]

👍

[luisvm]

We're going to merge this one in and close 137