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

Hi Akshay,

Please find the attached updated patch.

On Thu, Apr 2, 2020 at 4:55 PM Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com>
wrote:

> Hi Khushboo
>
> Following are the initial review comments (GUI):
>
> *Desktop Mode: *
>
> - KeyError: '_auth_source_manager_obj' in desktop mode. (*Note* error
> occurs when the patch has applied and server mode is False.)
>
> Fixed.

> *Server Mode:*
>
> AUTHENTICATION_SOURCES = ['internal']
>
>
> - Try to add a new user with the same email address, it throws a
> unique key constraint error. Validation was there previously before saving
> it.
>
> Fixed.

> AUTHENTICATION_SOURCES = ['internal', 'ldap']
>
> - Try to add a new user with the same email address, it throws
> unique key constraint error which should not it may possible that the user
> has the same email address for internal and ldap.
>
> If the source is internal, it will not allow but with ldap, we can add the
user with the same email id.

> AUTHENTICATION_SOURCES = ['ldap']
>
> - If ipAddress or Port is not set in the configuration file then
> browser showing the following data, it should be shown proper error message
> on the login page
> - {"success":0,"errormsg":"Port could not be cast to integer value
> as '<port>'","info":"","result":null,"data":null}
>
> Done

>
> - If IP address and port is provided but it is wrong, it shows the
> following error, can we make a generic error message? Also clicking on the
> Close button on that error message is not working.
> [image: Screenshot 2020-04-02 at 4.23.55 PM.png]
>
> I will look into the close button issue as it is an existing issue.

>
> -
> - IP address and port of LDAP server are correct, give wrong user name
> and password, it shows error "Error binding to the LDAP Server: None".
> Please correct the appropriate error message.
>
> Fixed.

>
> - All the configuration parameter is correct and tries to log in on
> LDAP server using username (*not email address*) and password got following
> error:
>
> current_user.email.split('@')[0] if config.SERVER_MODE is True
> AttributeError: 'NoneType' object has no attribute 'split'.
>
> Fixed.

> Not able to test due to the above error. Please fix and resend the patch.
>

Thanks,
Khushboo

Thanks,
Khushboo

>
> On Thu, Apr 2, 2020 at 2:06 PM Khushboo Vashi <
> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>
>> 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
>>>>>>>
>>>>>>
>
> --
> *Thanks & Regards*
> *Akshay Joshi*
>
> *Sr. Software Architect*
> *EnterpriseDB Software India Private Limited*
> *Mobile: +91 976-788-8246*
>