Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jKHYo-0005PP-UP for pgadmin-hackers@arkaria.postgresql.org; Fri, 03 Apr 2020 08:20:19 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1jKHYn-00015S-M9 for pgadmin-hackers@arkaria.postgresql.org; Fri, 03 Apr 2020 08:20:17 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jKHYn-00015J-85 for pgadmin-hackers@lists.postgresql.org; Fri, 03 Apr 2020 08:20:17 +0000 Received: from mail-il1-x12e.google.com ([2607:f8b0:4864:20::12e]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1jKHYe-0003wq-W0 for pgadmin-hackers@postgresql.org; Fri, 03 Apr 2020 08:20:16 +0000 Received: by mail-il1-x12e.google.com with SMTP id f16so6395564ilj.9 for ; Fri, 03 Apr 2020 01:20:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Wk/eOlxmhxWJMzcxrzHx7cIHgo0LkOonjOi3neAs0jo=; b=HaoSj+kHbnrnhft6VLMpWkPRCy8kHl3bcjX6CkyMI+mzSvJvFyWcJ3iM7M01oV5yJc 8rCv46UNaXvq1p7izLjgPW7+DaqmpWQ8rhCSPllzmGh1HdykmJj+1krUPHjiMoz+hIgB AIeYx0T8AP4XFm7jz8zsW3p/8lZVO0l+kHqVwqXDTkv7zijvwCZyKfOsANZ8X0w/inyK 56LTCZXV+2yiXmz8TUdZM2oPpdwIIjJohkx2xpUM9MvUFdw9nEcxI9QieQ/ESDnh9lG2 n9CSMvOmDtBQrS3X8ES5+kWR/cuwbuBaxhMshbRgyI01tv7ouwmshMSREwQWp/jEgPxR j2MQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Wk/eOlxmhxWJMzcxrzHx7cIHgo0LkOonjOi3neAs0jo=; b=lX7+kyOzcE6cVwv+yuITIamSVm1x+g8TrdMqEQ5jXbrnBT0kP4bIEhz4hDqoodxySy xQCUFXkOZUc2LT4Jl42TC1emDurzcKbgb1TD7ejhGgBYxWiNjm2L8r6iCOjnr1AlFGwy 6vRF/07carp5fWbmFWd9QWs2+c3HBf4IoE84CSviRWH9mdQ8AYkCjBBFS0oxO2uxhaPv BFOv4BJZJt9F0JEx+OP0SP+67ZFNwFMKdVwWKiD7xuypG8Xh44qSuXANNvKLICEUsTsA GCBpPVxan/4qKniwWzTmIEInOQUiCBc/BJFIU5W8joEYzkVLKTzNvVo2Zpr5Cub1NegY I/dw== X-Gm-Message-State: AGi0PuY4e1o3+Vl1BniNrOhnWHrn+wJohCPfLch17G7vhiw/APEtxD7P Q5TfnN2oIdkCzOpHnZZTBmAas/kFSdSLaC8oyh3Bkp0LSpLAf/EGGvQhT5RZJ3NNb6nns16NOn+ o0nW2f3l28jEgheWVUGiazcjstnIJkwkkqOOrV3d/gyHW5fRDBIibzBNSreh4Y4+fIeAbWZHP5V kOmKedkt1NT+btYQjTznlppKJ/TOqkm3koUIKd+nJgt09NTygw56g= X-Google-Smtp-Source: APiQypJdLmg+ZJ6mUOwJBVSqL7OnSjtNkm+NjJCr07A5DQXHmDelqc/Uah4qtl5l0YcwgI4/SA1ucULYW1bdNDix4+8= X-Received: by 2002:a92:5b17:: with SMTP id p23mr7376377ilb.121.1585902006923; Fri, 03 Apr 2020 01:20:06 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Akshay Joshi Date: Fri, 3 Apr 2020 13:49:55 +0530 Message-ID: Subject: Re: [pgAdmin4][Patch] - RM 2186 - Support external authentication sources [LDAP] To: Khushboo Vashi , Dave Page Cc: pgadmin-hackers Content-Type: multipart/alternative; boundary="000000000000c58fb605a25e95e6" X-CLOUD-SEC-AV-Info: edb,google_mail,monitor X-CLOUD-SEC-AV-Sent: true X-Gm-Spam: 0 X-Gm-Phishy: 0 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000c58fb605a25e95e6 Content-Type: text/plain; charset="UTF-8" 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 ''","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.vashi@enterprisedb.com> wrote: > Hi Akshay, > > Please find the attached updated patch. > > On Thu, Apr 2, 2020 at 4:55 PM Akshay Joshi > 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 ''","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.vashi@enterprisedb.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.vashi@enterprisedb.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.vashi@enterprisedb.com> wrote: >>>> >>>>> Please disregard my previous patch, attached the updated patch. :) >>>>> >>>>> >>>>> On Tue, Mar 24, 2020 at 10:32 AM Khushboo Vashi < >>>>> khushboo.vashi@enterprisedb.com> wrote: >>>>> >>>>>> Please disregard my previous patch, attached the updated patch. >>>>>> >>>>>> On Tue, Mar 24, 2020 at 10:29 AM Khushboo Vashi < >>>>>> khushboo.vashi@enterprisedb.com> wrote: >>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> Please find the attached updated patch. >>>>>>> >>>>>>> >>>>>>> On Tue, Mar 17, 2020 at 4:11 PM Dave Page wrote: >>>>>>> >>>>>>>> Hi >>>>>>>> >>>>>>>> On Tue, Mar 17, 2020 at 10:24 AM Khushboo Vashi < >>>>>>>> khushboo.vashi@enterprisedb.com> wrote: >>>>>>>> >>>>>>>>> Hi Dave, >>>>>>>>> >>>>>>>>> Thanks for the review. >>>>>>>>> >>>>>>>>> On Tue, Mar 17, 2020 at 3:42 PM Dave Page >>>>>>>>> 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.vashi@enterprisedb.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* --000000000000c58fb605a25e95e6 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi=C2=A0Khushboo
Some more review comments:
  • Fix one small PEP= 8 issue.
  • If ipAddress or Port is not set in the configuration file = then browser showing the following data, it should be shown proper error me= ssage on the login page
    • {"success":0,"err= ormsg":"Port could not be cast to integer value as '<port&= gt;'","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=C2=A0= in the test_config.json=C2=A0file. If the configuration is not provided the= n LDAP tests should be skipped.=C2=A0
@Dave, I have tested and don= e the code review. Can you please do it once as well, whenever Khushboo wil= l fix and send the updated patch?


On Thu, Apr 2, 20= 20 at 7:00 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:
H= i Akshay,

Please find the attached upd= ated patch.

On Thu, Apr 2, 2020 at 4:55 PM Akshay Joshi <akshay.joshi@enterprised= b.com> wrote:
Hi=C2=A0Khushboo

Following are the= initial review comments (GUI):

Desktop Mo= de:=C2=A0
  • KeyError: '_auth_source_manager_obj' in = desktop mode. (Note error occurs when the patch has applied and serv= er mode is False.)
Fixed.=C2=A0
Server Mode:
AUTHENTICATION_SOURCES =3D ['internal']=
  • Try to add a new user with the s= ame=C2=A0email address, it throws a unique=C2=A0key constraint=C2=A0error. = Validation was there previously before saving it.
Fixed.=C2=A0
AUTHENTICATION_= SOURCES =3D ['internal', 'ldap']
  • Try to add a ne= w user with the same=C2=A0email address, it throws unique=C2=A0key constrai= nt=C2=A0error which should not it may possible that=C2=A0the user has the s= ame 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.=C2=A0
AUT= HENTICATION_SOURCES =3D ['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,&q= uot;errormsg":"Port could not be cast to integer value as '&l= t;port>'","info":"","result":null= ,"data":null}
Done=C2=A0
  • = If IP address and port is provided but= it is wrong, it shows the following error, can we make a generic error mes= sage? Also clicking on the Close button on that error message is not workin= g.
    3D"Screenshot
I will look into the c= lose 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: N= one". Please correct the appropriate error message.
Fixed.=C2=A0
  • All the configuration parameter is correct and tries to log in on= LDAP server using username (*not email address*) and password got followin= g error:
    • current_user.email.split(&= #39;@')[0] if config.SERVER_MODE is True
    AttributeError: 'NoneTy= pe' object has no attribute 'split'.
    Fixed.=C2=A0
    Not able to test du= e 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.vashi@enterprisedb.c= om> wrote:
    Hi,

    Resending the patch.=C2=A0
    <= div>Missed the requirements.txt file in the previous patch.

    <= /div>
    Thanks,
    Khushboo

    On Wed, Apr 1, 2020 at 5:38 PM Kh= ushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:
    Hi,

    Please find the attached updated patch which includes the review co= mments given in the review meeting:

    1. Do not stor= e password=C2=A0for ldap user in sqlite database
    2. Forgot Passwo= rd : Give error to ldap users
    3. User Management dialog changes= =C2=A0
    4. Authentication source display besides username / email = after login

    Thanks,
    Khushboo
    <= br>

    On Tue, Mar 24, 2020 at 3:20 PM Khushboo Vashi <khushboo.vashi@enterp= risedb.com> wrote:
    Please disregard my previous p= atch, attached the updated patch. :)


    On Tue, Mar 24, 2020 at= 10:32 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:
    Plea= se disregard my previous patch, attached the updated patch.

    On Tue, Mar 24, = 2020 at 10:29 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote= :
    Hi,

    Please find the attached updated= patch.


    On Tue, Mar 17, 2020 at 4:11 PM Dave Page <= ;dpage@pgadmin.org> wrote:
    Hi

    On Tue, Mar 17, 2020 at 10:24 AM Khushboo Vashi <<= a href=3D"mailto:khushboo.vashi@enterprisedb.com" target=3D"_blank">khushbo= o.vashi@enterprisedb.com> wrote:
    Hi Dave,

    =
    Thanks for the review.

    On Tue, Mar 17, 2020 at 3:42 PM Dave Page <dpage@pgadmin.org&= gt; wrote:
    Hi

    30 second read of the first version of th= e patch...

    - Please move the configuration into co= nfig.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 au= th config.

    There are many setti= ngs 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 authent= ication.

    Sure, but our co= nfig 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 co= nfig_local.py file which includes other files as needed.
    Fixed.=C2=A0
    =C2= =A0
    - I think all config options should be = prefixed with LDAP_ as we may have things like CERT_FILE for other purposes= too.

    Sure.=C2=A0
    Done.=C2=A0
    =
    - I don't see any test cases.
    <= div>
    I will think about this, as right now= no idea how to write test cases for this.=C2=A0

    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).
    =C2=A0
    Done.

    Thanks,
    Khushboo
    Thanks.

    Thanks,
    Khushboo=C2=A0

    On Tue, Mar 17, 2020= at 8:55 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:
    =
    Hi= ,

    Please find the attached patch to support LDAP Authent= ication in Server mode.
    To test the patch, config_auth.py needs t= o 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 an= d ldap=C2=A0+ ssl/tls. With the TLS, I have used the default config of ldap= 3 without certificates.

    @Dave, can you please revi= ew this patch, as you have a better understanding of LDAP and you can easil= y pointed out if I have missed anything.

    Note: For= the document update I will create the task and assign to Nidhi for the sam= e.

    Thanks,
    Khushboo


    --
    Dave Page
    Blog: http://pgsnake.blogspot.com
    Twitter: @pgsnake

    EnterpriseDB= UK: http://www.e= nterprisedb.com
    The Enterprise PostgreSQL Company


    --
    Dave Page
    Blog: http://pgsnake.blogspot.com
    Twitter: @pgsnake

    EnterpriseDB= UK: http://www.e= nterprisedb.com
    The Enterprise PostgreSQL Company


    --
    Thanks & Regards
    Akshay Joshi
    <= font color=3D"#3333FF">Sr. Software Architect=
    <= font color=3D"#000000" face=3D"arial, sans-serif">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
    --000000000000c58fb605a25e95e6--