Include all common names when common_names is in extra attributes #11
Conversation
|
Looks like CI is failing to install some gems |
| @@ -77,17 +77,16 @@ def user_filter(username) | |||
| end | |||
|
|
|||
| def extra_attributes(user_plain) | |||
| if @options[:extra_attributes] | |||
There was a problem hiding this comment.
Is it safe to remove this conditional?
Alternatively, you could do (@options[:extra_attributes] || []).each_with_object({})...
There was a problem hiding this comment.
I was able to remove it without any specs failing, but that doesn't mean it's safe to remove. It turns out that line 52 already requires extra_attributes to be present. @options[:extra_attributes].values will give you NoMethodError: undefined method `values' for nil:NilClass if extra_attributes isn't included in the config.
There was a problem hiding this comment.
Actually, there's an issue about this exact thing #9
lib/casino/ldap_authenticator.rb
Outdated
| @options[:extra_attributes].each_with_object({}) do |(index_result, index_ldap), result| | ||
| result.merge!(index_result => user_plain[index_ldap].first.to_s) | ||
| end.tap do |results| | ||
| if @options[:extra_attributes].keys.include? :common_names |
There was a problem hiding this comment.
Maybe add parenthesis here to keep consistency?
The current implementation will only return the first group the user is in when memberof is supplied in extra_attributes. This will allow all group common names to be returned. Returning all full group names led to ActionDispatch::Cookies::CookieOverflow errors.
ryanzbone
force-pushed
the
common-name-list
branch
from
ffd3c3b
to
c1a0256
Compare
When memberof is supplied in extra_attributes, the current implementation will only return the user's first group membership. This change introduces a new option for extra attributes that will return all group common names. Solutions similar to the ones proposed in #5 led to ActionDispatch::Cookies::CookieOverflow errors from rails due to the number of groups our users are in. This solution will run into the same issue at a certain level and maybe needs some kind of limiter on what is returned beyond just using the common names.
Would the maintainers be interested in something like this?