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

Hi,

Please find the attached updated patch.

On Fri, Apr 3, 2020 at 1:50 PM Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com>
wrote:

> Hi Khushboo
>
> Some more review comments:
>
> - Fix one small PEP8 issue.
>
> Fixed.

>
> - 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}
>
> Fixed.

>
> - Disable the Username field in the User Management dialog if the
> authentication source is set to internal.
>
> Done.

>
> - API Test cases are failing if LDAP related settings are not
> provided in the test_config.json file. If the configuration is not provided
> then LDAP tests should be skipped.
>
> Fixed.

> @Dave, I have tested and done the code review. Can you please do it once
> as well, whenever Khushboo will fix and send the updated patch?
>
> Thanks,
Khushboo

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