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

Hi Khushboo

Some more review comments:

- Fix one small PEP8 issue.
- 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}
- Disable the Username field in the User Management dialog if the
authentication source is set to internal.
- 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.

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

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*