public inbox for [email protected]  

help / color / mirror / Atom feed

From: Akshay Joshi <[email protected]>
To: Khushboo Vashi <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Subject: Re: [pgAdmin][Patch] - RM #5940 - Add support for Oauth 2 authentication
Date: Mon, 5 Jul 2021 17:39:12 +0530
Message-ID: <CANxoLDcfbSck_siAM+kaEQnedwZURPLP_5W8krcpwnfzA6bULQ@mail.gmail.com> (raw)
In-Reply-To: <CAFOhELdfoR+d16-K2dcr11JaXz9eqgksg+1Z1X0Qgq-BZ6AqbQ@mail.gmail.com>
References: <CAFOhELek51f+q6gb2Sdx4CX94fSNVfye2k8-dXKJm3afDWiL6w@mail.gmail.com>
	<CAFOhELdfoR+d16-K2dcr11JaXz9eqgksg+1Z1X0Qgq-BZ6AqbQ@mail.gmail.com>

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 <oauth2 display name>.
   - 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 <
[email protected]> wrote:

> Hi,
>
> Please find the attached rebased patch.
>
> Thanks,
> Khushboo
>
> On Thu, Jun 17, 2021 at 4:22 PM Khushboo Vashi <
> [email protected]> 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 <http://edbpostgres.com>*

*Mobile: +91 976-788-8246*

view thread (5+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected]
  Subject: Re: [pgAdmin][Patch] - RM #5940 - Add support for Oauth 2 authentication
  In-Reply-To: <CANxoLDcfbSck_siAM+kaEQnedwZURPLP_5W8krcpwnfzA6bULQ@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox