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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

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

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

RM_2186.patch (45K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Navnath Gadakh
Hi Khushboo,
       I think there is no use of 

+    if app is not None:
+        AuthSourceRegistry.load_auth_sources()
+

in get_auth_sources() function.


On Tue, Mar 17, 2020 at 2:25 PM Khushboo Vashi <[hidden email]> 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


--
-- Navnath
Reply | Threaded
Open this post in threaded view
|

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

Dave Page-7
In reply to this post by Khushboo Vashi
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.

- I think all config options should be prefixed with LDAP_ as we may have things like CERT_FILE for other purposes too.

- I don't see any test cases.

Thanks.


On Tue, Mar 17, 2020 at 8:55 AM Khushboo Vashi <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Khushboo Vashi
Hi Dave,

Thanks for the review.

On Tue, Mar 17, 2020 at 3:42 PM Dave Page <[hidden email]> 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.
- I think all config options should be prefixed with LDAP_ as we may have things like CERT_FILE for other purposes too.

Sure. 
- I don't see any test cases.

I will think about this, as right now no idea how to write test cases for this. 
Thanks.

Thanks,
Khushboo 

On Tue, Mar 17, 2020 at 8:55 AM Khushboo Vashi <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Khushboo Vashi
In reply to this post by Navnath Gadakh
Hi Navnath,

On Tue, Mar 17, 2020 at 3:37 PM navnath gadakh <[hidden email]> wrote:
Hi Khushboo,
       I think there is no use of 

+    if app is not None:
+        AuthSourceRegistry.load_auth_sources()
+

in get_auth_sources() function.

Thanks for the review, I will look into it.

Thanks,
Khushboo 

On Tue, Mar 17, 2020 at 2:25 PM Khushboo Vashi <[hidden email]> 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


--
-- Navnath
Reply | Threaded
Open this post in threaded view
|

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

Dave Page-7
In reply to this post by Khushboo Vashi
Hi

On Tue, Mar 17, 2020 at 10:24 AM Khushboo Vashi <[hidden email]> wrote:
Hi Dave,

Thanks for the review.

On Tue, Mar 17, 2020 at 3:42 PM Dave Page <[hidden email]> 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.
 
- I think all config options should be prefixed with LDAP_ as we may have things like CERT_FILE for other purposes too.

Sure. 
- 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).
 
Thanks.

Thanks,
Khushboo 

On Tue, Mar 17, 2020 at 8:55 AM Khushboo Vashi <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Khushboo Vashi
Hi,

Please find the attached updated patch.


On Tue, Mar 17, 2020 at 4:11 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, Mar 17, 2020 at 10:24 AM Khushboo Vashi <[hidden email]> wrote:
Hi Dave,

Thanks for the review.

On Tue, Mar 17, 2020 at 3:42 PM Dave Page <[hidden email]> 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 <[hidden email]> 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

RM_2186_v1.patch (63K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Khushboo Vashi
Please disregard my previous patch, attached the updated patch.

On Tue, Mar 24, 2020 at 10:29 AM Khushboo Vashi <[hidden email]> wrote:
Hi,

Please find the attached updated patch.


On Tue, Mar 17, 2020 at 4:11 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, Mar 17, 2020 at 10:24 AM Khushboo Vashi <[hidden email]> wrote:
Hi Dave,

Thanks for the review.

On Tue, Mar 17, 2020 at 3:42 PM Dave Page <[hidden email]> 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 <[hidden email]> 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

RM_2186_v1.patch (62K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Khushboo Vashi
Please disregard my previous patch, attached the updated patch. :)


On Tue, Mar 24, 2020 at 10:32 AM Khushboo Vashi <[hidden email]> wrote:
Please disregard my previous patch, attached the updated patch.

On Tue, Mar 24, 2020 at 10:29 AM Khushboo Vashi <[hidden email]> wrote:
Hi,

Please find the attached updated patch.


On Tue, Mar 17, 2020 at 4:11 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, Mar 17, 2020 at 10:24 AM Khushboo Vashi <[hidden email]> wrote:
Hi Dave,

Thanks for the review.

On Tue, Mar 17, 2020 at 3:42 PM Dave Page <[hidden email]> 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 <[hidden email]> 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

RM_2186_v1.patch (62K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Khushboo Vashi
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 <[hidden email]> wrote:
Please disregard my previous patch, attached the updated patch. :)


On Tue, Mar 24, 2020 at 10:32 AM Khushboo Vashi <[hidden email]> wrote:
Please disregard my previous patch, attached the updated patch.

On Tue, Mar 24, 2020 at 10:29 AM Khushboo Vashi <[hidden email]> wrote:
Hi,

Please find the attached updated patch.


On Tue, Mar 17, 2020 at 4:11 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, Mar 17, 2020 at 10:24 AM Khushboo Vashi <[hidden email]> wrote:
Hi Dave,

Thanks for the review.

On Tue, Mar 17, 2020 at 3:42 PM Dave Page <[hidden email]> 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 <[hidden email]> 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

RM_2186_v2.patch (99K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Khushboo Vashi
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 <[hidden email]> 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 <[hidden email]> wrote:
Please disregard my previous patch, attached the updated patch. :)


On Tue, Mar 24, 2020 at 10:32 AM Khushboo Vashi <[hidden email]> wrote:
Please disregard my previous patch, attached the updated patch.

On Tue, Mar 24, 2020 at 10:29 AM Khushboo Vashi <[hidden email]> wrote:
Hi,

Please find the attached updated patch.


On Tue, Mar 17, 2020 at 4:11 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, Mar 17, 2020 at 10:24 AM Khushboo Vashi <[hidden email]> wrote:
Hi Dave,

Thanks for the review.

On Tue, Mar 17, 2020 at 3:42 PM Dave Page <[hidden email]> 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 <[hidden email]> 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

RM_2186_v3.patch (99K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Akshay Joshi
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.)
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.
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.
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}
  • 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.
  • 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.
  • 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'.

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

On Thu, Apr 2, 2020 at 2:06 PM Khushboo Vashi <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
Please disregard my previous patch, attached the updated patch. :)


On Tue, Mar 24, 2020 at 10:32 AM Khushboo Vashi <[hidden email]> wrote:
Please disregard my previous patch, attached the updated patch.

On Tue, Mar 24, 2020 at 10:29 AM Khushboo Vashi <[hidden email]> wrote:
Hi,

Please find the attached updated patch.


On Tue, Mar 17, 2020 at 4:11 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, Mar 17, 2020 at 10:24 AM Khushboo Vashi <[hidden email]> wrote:
Hi Dave,

Thanks for the review.

On Tue, Mar 17, 2020 at 3:42 PM Dave Page <[hidden email]> 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 <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Khushboo Vashi
Hi Akshay,

Please find the attached updated patch.

On Thu, Apr 2, 2020 at 4:55 PM Akshay Joshi <[hidden email]> 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.
    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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
Please disregard my previous patch, attached the updated patch. :)


On Tue, Mar 24, 2020 at 10:32 AM Khushboo Vashi <[hidden email]> wrote:
Please disregard my previous patch, attached the updated patch.

On Tue, Mar 24, 2020 at 10:29 AM Khushboo Vashi <[hidden email]> wrote:
Hi,

Please find the attached updated patch.


On Tue, Mar 17, 2020 at 4:11 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, Mar 17, 2020 at 10:24 AM Khushboo Vashi <[hidden email]> wrote:
Hi Dave,

Thanks for the review.

On Tue, Mar 17, 2020 at 3:42 PM Dave Page <[hidden email]> 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 <[hidden email]> 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

RM_2186_v4.patch (96K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Akshay Joshi
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 <[hidden email]> wrote:
Hi Akshay,

Please find the attached updated patch.

On Thu, Apr 2, 2020 at 4:55 PM Akshay Joshi <[hidden email]> 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.
    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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
Please disregard my previous patch, attached the updated patch. :)


On Tue, Mar 24, 2020 at 10:32 AM Khushboo Vashi <[hidden email]> wrote:
Please disregard my previous patch, attached the updated patch.

On Tue, Mar 24, 2020 at 10:29 AM Khushboo Vashi <[hidden email]> wrote:
Hi,

Please find the attached updated patch.


On Tue, Mar 17, 2020 at 4:11 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, Mar 17, 2020 at 10:24 AM Khushboo Vashi <[hidden email]> wrote:
Hi Dave,

Thanks for the review.

On Tue, Mar 17, 2020 at 3:42 PM Dave Page <[hidden email]> 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 <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Khushboo Vashi
Hi,

Please find the attached updated patch.

On Fri, Apr 3, 2020 at 1:50 PM Akshay Joshi <[hidden email]> 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 <[hidden email]> wrote:
Hi Akshay,

Please find the attached updated patch.

On Thu, Apr 2, 2020 at 4:55 PM Akshay Joshi <[hidden email]> 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.
    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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
Please disregard my previous patch, attached the updated patch. :)


On Tue, Mar 24, 2020 at 10:32 AM Khushboo Vashi <[hidden email]> wrote:
Please disregard my previous patch, attached the updated patch.

On Tue, Mar 24, 2020 at 10:29 AM Khushboo Vashi <[hidden email]> wrote:
Hi,

Please find the attached updated patch.


On Tue, Mar 17, 2020 at 4:11 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, Mar 17, 2020 at 10:24 AM Khushboo Vashi <[hidden email]> wrote:
Hi Dave,

Thanks for the review.

On Tue, Mar 17, 2020 at 3:42 PM Dave Page <[hidden email]> 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 <[hidden email]> 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

RM_2186_v5.patch (96K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Akshay Joshi
Thanks, patch applied.

On Fri, Apr 3, 2020 at 2:37 PM Khushboo Vashi <[hidden email]> wrote:
Hi,

Please find the attached updated patch.

On Fri, Apr 3, 2020 at 1:50 PM Akshay Joshi <[hidden email]> 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 <[hidden email]> wrote:
Hi Akshay,

Please find the attached updated patch.

On Thu, Apr 2, 2020 at 4:55 PM Akshay Joshi <[hidden email]> 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.
    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 <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
Please disregard my previous patch, attached the updated patch. :)


On Tue, Mar 24, 2020 at 10:32 AM Khushboo Vashi <[hidden email]> wrote:
Please disregard my previous patch, attached the updated patch.

On Tue, Mar 24, 2020 at 10:29 AM Khushboo Vashi <[hidden email]> wrote:
Hi,

Please find the attached updated patch.


On Tue, Mar 17, 2020 at 4:11 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, Mar 17, 2020 at 10:24 AM Khushboo Vashi <[hidden email]> wrote:
Hi Dave,

Thanks for the review.

On Tue, Mar 17, 2020 at 3:42 PM Dave Page <[hidden email]> 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 <[hidden email]> 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