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

Thanks, patch applied.

On Fri, Apr 3, 2020 at 2:37 PM Khushboo Vashi <
khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:

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

--
*Thanks & Regards*
*Akshay Joshi*

*Sr. Software Architect*
*EnterpriseDB Software India Private Limited*
*Mobile: +91 976-788-8246*