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 1m0NPl-0000vw-7I for pgadmin-hackers@arkaria.postgresql.org; Mon, 05 Jul 2021 12:09:29 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1m0NPk-0004Cp-7q for pgadmin-hackers@arkaria.postgresql.org; Mon, 05 Jul 2021 12:09:28 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1m0NPj-0004Ch-QF for pgadmin-hackers@lists.postgresql.org; Mon, 05 Jul 2021 12:09:28 +0000 Received: from mail-il1-x12b.google.com ([2607:f8b0:4864:20::12b]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1m0NPh-00054h-1k for pgadmin-hackers@postgresql.org; Mon, 05 Jul 2021 12:09:26 +0000 Received: by mail-il1-x12b.google.com with SMTP id f12so10828864ils.11 for ; Mon, 05 Jul 2021 05:09:24 -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=+MEY+9iO5udoI0ngn1suSl9Hd1lp1k91UX3Y0NdzoqQ=; b=J2Z498YA0FhrY34fc+cAsjPVpiMaPEMJtV0mOCwKU+2QXMjAqlPXD2qyStrFrDxQnr P5XXtBqLiIgj41Q2qS8Jsj/qNzmbop/GfJvqyKSK+8Xqo0lM48KljJxLLc4UINh/ntuA QdZyZ68ekenh9N5Us7VJjm1pumxku7Ea08BQ4jIOdlOqsP66GhtPGkE/4gU98qpopkDT O6JMblS5rWUwHyJ65QWy79ulmkyEa3UQvNl5MyE1cSXfLPKAQUeFOU3+r2AF9FUPZnaK J+bGyscj+nRMQ9zxZcRFXPpLkj1m3ZsfMOhuAsiYsrQrL13v9yjbJUaSH9WDxNOuej/g 957g== 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=+MEY+9iO5udoI0ngn1suSl9Hd1lp1k91UX3Y0NdzoqQ=; b=hX3Bhq6gLF8Ar/tXrUQrC4tQbCtUJdFmHIyQIB0jUjGLXQe4yLegqjKbTgyYFXclEn OF1NHSef+oKbxns/2FCP6QMl+OFfNgjjWA6RWgD+O4SytmsgULnKO8hU1mwhCjnY31sx 1cm0DhaNGSoKJRbtfq4dpB0G95YV+diQ7QBC/rZYK9vmME1He3YZBajFTLYgZolIEhO1 mm2kTVas6CqB71SfLb25fWVbVcbuXHOOBqwzNeMc/FRepOwYvnh5G+YFdcraOIYg6GLo SAEzpc/DhKOQXADDVfTOEy6DVK87KEaFnG+/NeypGcDZ9oy4di+4Grb4eFYRe6COmOB5 L2xg== X-Gm-Message-State: AOAM531RHc4TxZGD1FV+UoORhX5Jz/NVeIJEGGarBn4MxEeGbUxdcKxr K72VbC5gGDWNjzNvxOb7jR9iPlDriCi0a2MH/LD1G838FYszrtaqqWq2QychvAqbsgv9WfpoSwa dsQzrBAywHPlskdPdh0gztSdr35cefiiiosuxtoGM/Lbc4msNGMT7nUmbTErq2fh0OvktE5HyeF YIpOauHU9cdM1ykRbQ1KWLKWNQ5xMIfwUx7lg/gYEsCk2ca1fLyzcr9xiQNzoBt2IOCg== X-Google-Smtp-Source: ABdhPJyXfFKEdW8KDLAQgFcfFGT+CXSHmPMOjblI/PVlSapOprHCOuKReZ/+RuSRKoh7EDU1xL36/3OdSHhVm7d1NB8= X-Received: by 2002:a92:d44a:: with SMTP id r10mr10583693ilm.257.1625486963802; Mon, 05 Jul 2021 05:09:23 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Akshay Joshi Date: Mon, 5 Jul 2021 17:39:12 +0530 Message-ID: Subject: Re: [pgAdmin][Patch] - RM #5940 - Add support for Oauth 2 authentication To: Khushboo Vashi Cc: pgadmin-hackers Content-Type: multipart/alternative; boundary="00000000000010a2fa05c65f2dd7" X-CLOUD-SEC-AV-Info: enterprisedb,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: Archived-At: Precedence: bulk --00000000000010a2fa05c65f2dd7 Content-Type: text/plain; charset="UTF-8" Hi Khushboo Following are the review comments: *Scenario *: AUTHENTICATION_SOURCES = ['oauth2', 'internal'] - Don't set the OAUTH2_DISPLAY_NAME, Login button shows 'Login with None' which should be change. Maybe placeholder like . - Enter the email address and password. Click on the "Login with ..." button, internal server error displayed on the page, we should throw a proper error message. - *Code Review:* - Empty "__init__.py" file is added in the web folder, I think that is not required. - Any reason to change *messages.pot* file manually? It will be updated when we update the message catalog. - Fixed SonarQube issues in the new file 'oauth2.py' and 'test_oauth2_with_mocking.py'. Remove unused imports from both files. - Add copyright header to "kerberos.js" file. *Documentation*: - Remove "of the" from the description of OAUTH2_NAME it is duplicated. - String "Authorisation" should be "Authorization". - Instead of providing "Oauth2 secret", "Oauth2 base url" .... can we provide some more detailed information? *Note*: I haven't tested the patch with the proper OAuth2 configuration. On Mon, Jun 28, 2021 at 4:52 PM Khushboo Vashi < khushboo.vashi@enterprisedb.com> wrote: > Hi, > > Please find the attached rebased patch. > > Thanks, > Khushboo > > On Thu, Jun 17, 2021 at 4:22 PM Khushboo Vashi < > khushboo.vashi@enterprisedb.com> wrote: > >> Hi, >> >> Please find the attached patch for the RM #5940 - Add support for Oauth 2 >> authentication. >> >> The patch includes: >> >> - Pluggable Oauth 2 authentication support with the multiple providers >> - Configurable support for Master Key in Oauth2 and Kerberos >> authentication, so user can save the database server password >> - Introduced the separate module/blueprint for kerberos and Oauth2 under >> authentication module for the readability and maintainability >> >> Note: The initial patch for Oauth2 authentication was sent by Florian >> Sabonchi, I have worked on top of it. >> >> >> Thanks, >> Khushboo >> >> -- *Thanks & Regards* *Akshay Joshi* *pgAdmin Hacker | Principal Software Architect* *EDB Postgres * *Mobile: +91 976-788-8246* --00000000000010a2fa05c65f2dd7 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Khushboo

Following are the review co= mments:

Scenario :=C2=A0AUT= HENTICATION_SOURCES =3D=C2=A0['oauth2',=C2=A0'internal']
  • Do= n't set the OAUTH2_DISPLAY_NAME, Login button shows 'Login with Non= e' which should be change. Maybe placeholder like <oauth2 display na= me>.
  • Enter the email address and password. Cl= ick on the "Login with ..." button, internal server error display= ed on the page, we should throw a proper error message.

  • =
Code Review:
  • Empty "__in= it__.py" file is added in the web folder, I think that is not required= .
  • Any reason=C2=A0to change messages.pot file manually? It w= ill be updated when we update the message catalog.
  • Fixed SonarQube = issues in the new file 'oauth2.py' and 'test_oauth2_with_mockin= g.py'.=C2=A0Remove unused imports from both files.
  • Add copyrigh= t header to "kerberos.js" file.

D= ocumentation:
  • Remove "of the" from the desc= ription of OAUTH2_NAME it is duplicated.
  • String "Authorisation= " should be "Authorization".
  • Instead of providing &q= uot;Oauth2 secret", "Oauth2 base url" .... can we provide so= me more detailed=C2=A0information?

No= te: I haven't tested the patch with the proper=C2=A0OAuth2 configur= ation.

On Mon, Jun 28, 2021 at 4:52 PM Khushboo Vashi <khushboo.vashi@enterprisedb.co= m> wrote:
Hi,

Please find the attached rebased p= atch.

Thanks,
Khushboo

On Thu, Jun = 17, 2021 at 4:22 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wr= ote:
Hi,

Please find the attached patch for the RM #594= 0 - Add support for Oauth 2 authentication.=C2=A0

= The patch includes:

- Pluggable Oauth 2 authentica= tion support=C2=A0with the multiple providers
- Configurable supp= ort for Master Key in Oauth2 and Kerberos authentication, so user can save = the database server password
- Introduced the separate module/blu= eprint for kerberos and Oauth2 under authentication module for the readabil= ity and maintainability

Note: The initial patch fo= r Oauth2 authentication was sent by Florian Sabonchi, I have worked on top = of it.


Thanks,
Khushboo



--
Thanks & Regards
Akshay Joshi
pgAdmi= n Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246

--00000000000010a2fa05c65f2dd7--