Re: [pgAdmin4][Patch] - RM 2186 - Support external authentication sources [LDAP]

Please disregard my previous patch, attached the updated patch. :)

On Tue, Mar 24, 2020 at 10:32 AM Khushboo Vashi <
khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:

> Please disregard my previous patch, attached the updated patch.
>
> On Tue, Mar 24, 2020 at 10:29 AM Khushboo Vashi <
> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>
>> Hi,
>>
>> Please find the attached updated patch.
>>
>>
>> On Tue, Mar 17, 2020 at 4:11 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>> Hi
>>>
>>> On Tue, Mar 17, 2020 at 10:24 AM Khushboo Vashi <
>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi Dave,
>>>>
>>>> Thanks for the review.
>>>>
>>>> On Tue, Mar 17, 2020 at 3:42 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>
>>>>> Hi
>>>>>
>>>>> 30 second read of the first version of the patch...
>>>>>
>>>>> - Please move the configuration into config.py. Users should never
>>>>> have to modify a distributed file (it messes up packaging). I don't see any
>>>>> reason to use a different file just for auth config.
>>>>>
>>>>> There are many settings for the LDAP, and in the future we will add
>>>> other external sources also, so I thought it would be better if we have
>>>> different file for the authentication.
>>>>
>>>
>>> Sure, but our config file is small compared to many. Splitting things
>>> out is more confusing for users. If they want to do that themselves of
>>> course, they can add a config_local.py file which includes other files as
>>> needed.
>>>
>> Fixed.
>>
>>>
>>>
>>>> - I think all config options should be prefixed with LDAP_ as we may
>>>>> have things like CERT_FILE for other purposes too.
>>>>>
>>>>> Sure.
>>>>
>>> Done.
>>
>>> - I don't see any test cases.
>>>>>
>>>>> I will think about this, as right now no idea how to write test cases
>>>> for this.
>>>>
>>>
>>> It should be fairly straightforward to write tests for some of the
>>> functions in the auth classes. For testing the actual LDAP stuff, we
>>> probably need to add LDAP config options to test_config.json, and only if
>>> present, run the tests. That would probably need to support a list of LDAP
>>> servers, so we can test with different configurations (LDAP, LDAPS,
>>> LDAP_STARTTLS, AD etc).
>>>
>>>
>> Done.
>>
>> Thanks,
>> Khushboo
>>
>>> Thanks.
>>>>>
>>>>> Thanks,
>>>> Khushboo
>>>>
>>>>>
>>>>> On Tue, Mar 17, 2020 at 8:55 AM Khushboo Vashi <
>>>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Please find the attached patch to support LDAP Authentication in
>>>>>> Server mode.
>>>>>> To test the patch, config_auth.py needs to be configured for LDAP
>>>>>> configurations. The config settings are explained in this file in detail.
>>>>>> After configuring the parameters, start the pgadmin server in Server mode
>>>>>> and connect with LDAP server with the valid user via login page.
>>>>>>
>>>>>> I have tested this patch with ldap and ldap + ssl/tls. With the TLS,
>>>>>> I have used the default config of ldap3 without certificates.
>>>>>>
>>>>>> @Dave, can you please review this patch, as you have a better
>>>>>> understanding of LDAP and you can easily pointed out if I have missed
>>>>>> anything.
>>>>>>
>>>>>> Note: For the document update I will create the task and assign to
>>>>>> Nidhi for the same.
>>>>>>
>>>>>> Thanks,
>>>>>> Khushboo
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Dave Page
>>>>> Blog: http://pgsnake.blogspot.com
>>>>> Twitter: @pgsnake
>>>>>
>>>>> EnterpriseDB UK: http://www.enterprisedb.com
>>>>> The Enterprise PostgreSQL Company
>>>>>
>>>>
>>>
>>> --
>>> Dave Page
>>> Blog: http://pgsnake.blogspot.com
>>> Twitter: @pgsnake
>>>
>>> EnterpriseDB UK: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>