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

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
>>>>
>>>