Skip to content
This repository has been archived by the owner on Jul 28, 2021. It is now read-only.

Multiple value fix #6

Closed
wants to merge 6 commits into from
Closed

Conversation

dayer4b
Copy link
Contributor

@dayer4b dayer4b commented May 28, 2014

This pull request contains a fix for the problem raised in Issue #5

@pencil
Copy link
Member

pencil commented May 28, 2014

Not so sure about this one, as it breaks backward compatibility. Your thoughts on this, @luxflux? Maybe something for the upcoming major release?

@luxflux
Copy link
Member

luxflux commented Jul 21, 2014

Sorry for the long delay. I really missed my mention here...

What's the reason to use a string as return value? Wouldn't it be better to always return an array with one or more elements?

CASino will dump the array into the response as yaml.

@dayer4b
Copy link
Contributor Author

dayer4b commented Jul 23, 2014

I suppose an array would work. I didn't test that. You're right, it would
be better to return such data as an array.

On Mon, Jul 21, 2014 at 3:27 AM, Raffael Schmid notifications@github.com
wrote:

Sorry for the long delay. I really missed my mention here...

What's the reason to use a string as return value
https://github.com/rbCAS/casino-ldap_authenticator/pull/6/files#diff-3db56e44a637a7b46d9d4e9d9b0fd974L75?
Wouldn't it be better to always return an array with one or more elements?

CASino will dump the array into the response as yaml
https://github.com/rbCAS/CASino/blob/master/app/builders/casino/ticket_validation_response_builder.rb#L37-L46
.


Reply to this email directly or view it on GitHub
#6 (comment)
.

Sunil

@adamcrown
Copy link

Glad to see there's a fix being worked on. We'd like to switch form rubycas-server to casino but this is a breaking bug for us.

I'd also like to bring up Jasig CAS extra attributes format compatibility. Just because a LDAP attribute only has one value doesn't mean it should be single value. I believe rubycas-server just spits out everything as an array. Which works, but if everything is an array then everything gets YAML encoded and that's especially bad for Jasig compatibility.

I'm thinking the best path forward could be a configuration option for extra attributes so anybody can configure certain extra attributes an being multi-value, and other as not, according to their needs. The default could be single-value to keep backwards compatibility.

Another, thing to consider might be different compatibility modes for extra attributes: Jasig, rubycas-server and possibly others I'm not aware of.

@pencil
Copy link
Member

pencil commented Sep 5, 2014

I'm thinking about just including all the values like this:

<name>John Doe</name>
<phone>123456789</phone>
<phone>321456789</phone>
<phone>213456789</phone>

Would this work? Probably would have to check compatibility with CAS clients out there.

Another, thing to consider might be different compatibility modes for extra attributes: Jasig, rubycas-server and possibly others I'm not aware of.

Actually it is kind of hard to get information on how Jasig CAS does it. There is a schema documented but I'm not sure it is actually used since I didn't find a single CAS client that does support it. Probably would have to look through their source code.

@pencil
Copy link
Member

pencil commented May 15, 2015

CASino 4.0 supports multiple-values for a single field. Basically we can change this line to:

result[index_result] = value

I currently don't have the infrastructure to test it but if you are still interested, you might want to give it a try.

@twyfordr
Copy link

We ran into this bug today. Will this patch (or a similiar one) be included in any upcoming release?

@pencil
Copy link
Member

pencil commented Jul 23, 2015

I didn't have the chance to test this change. I'm pretty sure most CAS client implementations can't handle multiple values for the same key but that would have to be tested as well.

@pencil
Copy link
Member

pencil commented Jul 23, 2015

Closing this and continuing in #5

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants