On 10 January 2018 at 16:27, Craig Russell <[email protected]> wrote:
> Thanks for the explanation. I now know what these two lines are supposed to
> do, but apparently they don't work.
>
> I removed the (redundant) check from modify_pmcchairs for testing and:
>
> [MacBook-Pro-9:/srv/whimsy/tools] clr% ./modify_pmcchairs.rb --add clr
> Password for clr:
> [MacBook-Pro-9:/srv/whimsy/tools] clr% ./modify_pmcchairs.rb --add clr
> Password for clr:
> _WARN [#<LDAP::Mod:0x7f966a1238a8 LDAP_MOD_ADD
> {"member"=>[]}>]
>
> IIUC this means that the
> _WARN cn=pmc-chairs,ou=groups,ou=services,dc=apache,dc=org
> /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:260:in `modify': Protocol
> error (LDAP::ResultError)
> from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:260:in
> `modify'
> from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:1377:in `add'
> from ./modify_pmcchairs.rb:36:in `block in <main>'
> from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:145:in `bind'
> from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:145:in `bind'
> from ./modify_pmcchairs.rb:36:in `<main>'
> [MacBook-Pro-9:/srv/whimsy/tools] clr% ./modify_pmcchairs.rb --rm clr
> Password for clr:
> [MacBook-Pro-9:/srv/whimsy/tools] clr% ./modify_pmcchairs.rb --rm clr
> Password for clr:
> _WARN [#<LDAP::Mod:0x7fc03380b6f0 LDAP_MOD_DELETE
> {"member"=>[]}>]
> _WARN cn=pmc-chairs,ou=groups,ou=services,dc=apache,dc=org
> /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:260:in `modify': Object
> class violation (LDAP::ResultError)
> from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:260:in
> `modify'
> from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:1368:in
> `remove'
> from ./modify_pmcchairs.rb:38:in `block in <main>'
> from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:145:in `bind'
> from /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:145:in `bind'
> from ./modify_pmcchairs.rb:38:in `<main>'
>
> Here's the fix:
>
> diff --git a/lib/whimsy/asf/ldap.rb b/lib/whimsy/asf/ldap.rb
> index 6f2065b..9d1c9f7 100644
> --- a/lib/whimsy/asf/ldap.rb
> +++ b/lib/whimsy/asf/ldap.rb
> @@ -1365,6 +1365,7 @@ module ASF
> def remove(people)
> @members = nil
> people = (Array(people) & members).map(&:dn)
> + return if people.empty?
> ASF::LDAP.modify(self.dn, [ASF::Base.mod_delete('member', people)])
> ensure
> @members = nil
> @@ -1374,6 +1375,7 @@ module ASF
> def add(people)
> @members = nil
> people = (Array(people) - members).map(&:dn)
> + if people.empty?
Is that supposed to be 'return if empty?'
> ASF::LDAP.modify(self.dn, [ASF::Base.mod_add('member', people)])
> ensure
> @members = nil
>
> I think for human experience it's bad to return without doing anything and
> without giving the user information that it's expected that nothing was done.
>
> Now that I know how this is supposed to work, the issue is where the checking
> should be done and how to report the issue to the user.
>
> The code in ldap.rb is used everywhere so it is probably not good to puts
> messages but perhaps better to raise a meaningful error instead of Object
> class exception.
>
> Craig
>
>> On Jan 9, 2018, at 8:13 PM, Sam Ruby <[email protected]> wrote:
>>
>> On Tue, Jan 9, 2018 at 6:11 PM, Craig Russell <[email protected]> wrote:
>>>
>>>> On Jan 8, 2018, at 11:55 PM, Sam Ruby <[email protected]> wrote:
>>>>
>>>> On Mon, Jan 8, 2018 at 11:34 PM, Craig Russell <[email protected]>
>>>> wrote:
>>>>>
>>>>>> On Jan 8, 2018, at 7:32 PM, Sam Ruby <[email protected]> wrote:
>>>>>>
>>>>>> On Mon, Jan 8, 2018 at 7:07 PM, Craig Russell <[email protected]>
>>>>>> wrote:
>>>>>>> /Users/clr/apache/git/whimsy/lib/whimsy/asf/ldap.rb:260:in `modify':
>>>>>>> Object class violation (LDAP::ResultError)
>>>>>>>
>>>>>>> And error reporting is not great. I guess more checking is needed but
>>>>>>> Object class violation is not very informative.
>>>>>>
>>>>>> Oh, and AGREED!
>>>>>>
>>>>>> LDAP sucks. You want to add zero members: Object class violation.
>>>>>> You want to add somebody who is already a member: Object class
>>>>>> violation. You want to remove somebody who is not a member: Object
>>>>>> class violation.
>>>>>>
>>>>>> That's why the action that caused the error is logged. In this case:
>>>>>>
>>>>>> LDAP_MOD_DELETE
>>>>>> {"member"=>[]}>
>>>>>> cn=pmc-chairs,ou=groups,ou=services,dc=apache,dc=org
>>>>>>
>>>>>> Here you are deleting nobody. That apparently is not allowed.
>>>>>
>>>>> Seems like there is some error checking that could be done in the ldap.rb
>>>>> code.
>>>>>
>>>>> Here is the remove code from lib/whimsy/asf/ldap.rb :
>>>>>
>>>>> # remove people from this service in LDAP
>>>>> def remove(people)
>>>>> @members = nil
>>>>> people = (Array(people) & members).map(&:dn)
>>>>> ASF::LDAP.modify(self.dn, [ASF::Base.mod_delete('member', people)])
>>>>> ensure
>>>>> @members = nil
>>>>> end
>>>>>
>>>>> Who wrote this code?
>>>>
>>>> That would be me.
>>>>
>>>>> Would it be possible for this code to check before calling
>>>>> ASF::LDAP.modify that the member actually exists already?
>>>>
>>>> The line above it takes the intersection between the list of people
>>>> passed to remove and the current set of members. So if a person who
>>>> is not a member of the service is passed in, they will be silently
>>>> removed from the set.
>>>
>>> Evidence suggests that the line and the corresponding line in Service.add
>>> are not working as intended.
>>>
>>> Where does the variable members come from? Is it different from @members
>>> that is set to nil just before trying to remove existing members?
>>
>> members is a method call. Essentially, self.members().
>>
>> https://whimsy.apache.org/docs/api/ASF/Service.html
>>
>> @members is the cached value for this function. Setting it to nil
>> will force the members method to fetch new data from LDAP.
>>
>> - Sam Ruby
>>
>>>> I'm not sure how to refactor it, but there are add and remove methods
>>>> for a number of LDAP objects: Committee, Project, etc.
>>>
>>> I'm not sure that a refactoring is needed, just testing to make it work as
>>> intended.
>>>
>>> Craig
>>>>
>>>> - Sam Ruby
>>>>
>>>>> Craig
>>>>>
>>>>>> - Sam Ruby
>>>>>>
>>>>>> - Sam Ruby
>>>>>
>>>>> Craig L Russell
>>>>> Secretary, Apache Software Foundation
>>>>> [email protected] http://db.apache.org/jdo
>>>
>>> Craig L Russell
>>> Secretary, Apache Software Foundation
>>> [email protected] http://db.apache.org/jdo
>>>
>
> Craig L Russell
> Secretary, Apache Software Foundation
> [email protected] http://db.apache.org/jdo
>
