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

Hi,

Resending the patch.
Missed the requirements.txt file in the previous patch.

Thanks,
Khushboo

On Wed, Apr 1, 2020 at 5:38 PM Khushboo Vashi <
khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:

> Hi,
>
> Please find the attached updated patch which includes the review comments
> given in the review meeting:
>
> 1. Do not store password for ldap user in sqlite database
> 2. Forgot Password : Give error to ldap users
> 3. User Management dialog changes
> 4. Authentication source display besides username / email after login
>
> Thanks,
> Khushboo
>
>
> On Tue, Mar 24, 2020 at 3:20 PM 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: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
>>>>>
>>>>