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 1jJxz5-0006EG-Fx for pgadmin-hackers@arkaria.postgresql.org; Thu, 02 Apr 2020 11:26:07 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1jJxz4-0002LD-0K for pgadmin-hackers@arkaria.postgresql.org; Thu, 02 Apr 2020 11:26:06 +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 1jJxz3-0002JK-Ns for pgadmin-hackers@lists.postgresql.org; Thu, 02 Apr 2020 11:26:05 +0000 Received: from mail-il1-x143.google.com ([2607:f8b0:4864:20::143]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1jJxyw-0002D0-7K for pgadmin-hackers@postgresql.org; Thu, 02 Apr 2020 11:26:05 +0000 Received: by mail-il1-x143.google.com with SMTP id g15so3173601ilj.10 for ; Thu, 02 Apr 2020 04:25:57 -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=1Wd1vl1r3rFrtgR8S69N/XFjAQ38uN91SRMDJHHJZfs=; b=DFLNmEteJFqZ5W6kD6suZkJmMv6uhh12KDwf7xJ7EtzK6hmpPg+MqZdtbyuGXFg29F TL7nIbT/yxVbczFSRMPEDtQFR9kEnUl8S6ySR20IF4QX8JBCSKaPgyccigEh8C6hlmU/ 12R9EaOtFcNfUmJjwQzou3cUmi3DPtb3vL2lAo80+ZyrliRqP8O1AQCbjnfl/97ic2Xy oViTWtKEZ7z2kR36O1nQgPu9R2iyReMSc+Rq7DMhuMllOp06QWtaJs1joMFi8n3AeJRC 7hWKHZKC3F/SYPCl0eBF7pPe6AA3cXNJ9yzcgwbA137VYe1CX9a7DWjGhIFx+hw7Wsb4 2Q+g== 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=1Wd1vl1r3rFrtgR8S69N/XFjAQ38uN91SRMDJHHJZfs=; b=f9Gg+58dx3BQNB442RfqVfWXiW4OFXM/7qBMDLIa5WzXbP6C9/7wZDTd+qN7pTb+14 TFm++1VA4iumYv31DpkpT3Pn1ZOLFv66pRRWJLYDOXdkvEeDsdAiDfydKijKK5wzX7T+ 2RerVwMZKNNI5bvK//D2L4GyIVnbOzSieNkq0QyhDM+BSZ9NYfpjRC65S8h2FvXzdYq7 rcAONgVynTym23LM51ciKghSJoTIP58uN2wRyGMKwJyeKggQoMIyUjemsxGqa7nGuaf6 798IW6PSg/om446e2fAZEnjzJ/D4iL59gqCoN3xVM8rRMHOp+qbdbKtKfAEJB+4+ETH7 tPxQ== X-Gm-Message-State: AGi0PuZSYQHoZL3pClYhzJX49YROFlODRPQrR7+pFQNEV8RfGmxK2Gxg FW0ndtM8FTG5re/cODc9YhkcrkKHhUrN9aocd4teFI8S8dQyo2Op4K3Su7CM+lQEJCz8l/jd08+ 2SaRGV0vrrdpKJzWgXVC77pfTptoYhaIslHf8KBCUIHPz2NiRu3DctxGsqaL0icainismMPxb3w MHXdPSE2srKHX8lrAZbktWF8H+3BRqZLo8HaP4qajb3Nr2hIrRru0= X-Google-Smtp-Source: APiQypKKGC18DGWTiyOiqXqIu4UZvwqqk6482+tN9wlJrl2LWIL8vh858Jsda8NQMC8+PaQOUkJjR9EfXGR+z4n2Fp0= X-Received: by 2002:a92:9fd0:: with SMTP id z77mr2725708ilk.257.1585826756358; Thu, 02 Apr 2020 04:25:56 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Akshay Joshi Date: Thu, 2 Apr 2020 16:55:45 +0530 Message-ID: Subject: Re: [pgAdmin4][Patch] - RM 2186 - Support external authentication sources [LDAP] To: Khushboo Vashi Cc: Dave Page , pgadmin-hackers Content-Type: multipart/alternative; boundary="0000000000007d187d05a24d10c9" 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 --0000000000007d187d05a24d10c9 Content-Type: text/plain; charset="UTF-8" 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 ''","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. [image: Screenshot 2020-04-02 at 4.23.55 PM.png] - 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 < 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* --0000000000007d187d05a24d10c9 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi=C2=A0Khushboo

Following are the init= ial review comments (GUI):

Desktop Mode:= =C2=A0
  • KeyError: '_auth_source_manager_obj' in des= ktop mode. (Note error occurs when the patch has applied and server = mode is False.)
Server Mode:
AUTHENTICATION_SOURCES =3D= ['internal']
  • Try to add = a new user with the same=C2=A0email address, it throws a unique=C2=A0key co= nstraint=C2=A0error. Validation was there previously before saving it.
    • =
AUTHENTICATION_SOURCES =3D ['internal', 'ldap']
  • Try to add a new user with the same=C2=A0email address, it throws uniq= ue=C2=A0key constraint=C2=A0error which should not it may possible that=C2= =A0the user has the same email address for internal and ldap.
    • AUTH= ENTICATION_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,&qu= ot;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 w= rong, it shows the following error, can we make a generic error message? Al= so clicking on the Close button on that error message is not working.
      3D"Screenshot<= /div>
    • IP address and port of LDAP server are correct, give wro= ng user name and password, it shows error "Error binding to the LDAP S= erver: 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(&#= 39;@')[0] if config.SERVER_MODE is True
    AttributeError: 'NoneTyp= e' object has no attribute 'split'.

    <= /blockquote>
    Not able to test due to the above error. Please fi= x and resend the patch.

    On Thu, Apr 2, 2020 at 2:06 PM Khushboo Vashi= <khushboo.vashi@ente= rprisedb.com> wrote:
    Hi,

    Resending the patch.=C2= =A0
    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=C2=A0for ldap user in sqlite database
    2. Forg= ot Password : Give error to ldap users
    3. User Management dialog = changes=C2=A0
    4. Authentication source display besides username /= email after login

    Thanks,
    Khushboo


    On Tue, Mar 24, 2020 at 3:20 PM Khushboo Vashi <khushboo.vash= i@enterprisedb.com> wrote:
    Please disregard my pre= vious 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, M= ar 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 Pag= e <dpage@pgadmin.= org> wrote:
    Hi

    On Tue, Mar 17, 2020 at 10:24 AM Khushboo Vashi = <kh= ushboo.vashi@enterprisedb.com> wrote:
    Hi Dave,

    <= /div>
    Thanks for the review.

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

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

    - Please move the configuration in= to config.py. Users should never have to modify a distributed file (it mess= es up packaging). I don't see any reason to use a different file just f= or 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 au= thentication.

    Sure, but o= ur config file is small compared to many. Splitting things out is more conf= using 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.=C2=A0
    = =C2=A0
    - I think all config options should = be prefixed with LDAP_ as we may have things like CERT_FILE for other purpo= ses too.

    Sure.=C2=A0
    Done.=C2=A0
    - I don't see any test cases.

    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 tes= ts for some of the functions in the auth classes. For testing the actual LD= AP 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 l= ist of LDAP servers, so we can test with different configurations (LDAP, LD= APS, LDAP_STARTTLS, AD etc).
    =C2=A0
    Done.

    Thanks,
    Khushboo
    Thanks.

    <= /blockquote>
    Thanks,
    Khushboo=C2=A0

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

    Please find the attached patch to support LDAP Auth= entication in Server mode.
    To test the patch, config_auth.py need= s to be configured for LDAP configurations. The config settings are explain= ed in this file in detail. After configuring the parameters, start the pgad= min 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=C2=A0+ ssl/tls. With the TLS, I have used the default config of l= dap3 without certificates.

    @Dave, can you please r= eview this patch, as you have a better understanding of LDAP and you can ea= sily 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
    <= /div>


    --
    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
    Sr. Software Architect
    EnterpriseDB Software India Private Limited
    =
    Mobile: += 91 976-788-8246
    --0000000000007d187d05a24d10c9--