Re: Patch: Two-factor Authentication (RM #6543)

From: Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com>
To: Ashesh Vashi <ashesh(dot)vashi(at)enterprisedb(dot)com>
Cc: pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>
Subject: Re: Patch: Two-factor Authentication (RM #6543)
Date: 2021-08-31 07:33:21
Message-ID: CANxoLDcH46q6YEjd_dWD23Mk43+SAaV2ayk=MxY+o7-RRxx4sQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgadmin-hackers

Hi Ashesh

Following are the review comments:

- *GUI*:
- The two-factor authentication dialog's size is too big, can we make
it a bit small?
- Dialog's header is showing "AlertifyJS", please provide some valid
header name.
- Documentation: Let users know from where they can access the
"Two-Factor Authentication" dialog, the screenshot of the top-right menu
option, and few lines.
- Do we need MFA_ENABLED and MFA_FORCE_REGISTRATION two different
parameters? For the users, if MFA_ENABLED is set to True then it
is as good
as users bound to register for MFA and after the successful validation,
they can access the pgAdmin 4 webpage.
- *Code*:
- Copyright headers are missing in some of the new files.
- Add comments/function descriptions wherever needed. It will be easy
for others to understand the logic.
- Remove unused imports from the new files.
- Sonarlint showing warning "Replace <i> tag with <em>" for some HTML
files.
- *send_email_otp.html* and *send_email_otp.txt* both files having
the same content, they both are needed?
- In "*test_mfa_view.py*" file line no 26 showing "Unresolved
reference ValidationException"
- In the "*mfa/tests/utils.py"* file Remove unused class declaration
"class DummyAuth". Also, update the "return true" with "return True" at
line no 31

*Should* *we consider*: What if pgAdmin 4 Administrator wants MFA force
registration for all the other users except himself/herself?

*Note*: I haven't run API test cases. Checked PEP8, Linter, and Jasmine
tests.

On Mon, Aug 30, 2021 at 6:23 PM Ashesh Vashi <ashesh(dot)vashi(at)enterprisedb(dot)com>
wrote:

> Hi Team,
>
> Please find the update patch after fixing some issues introduced during
> refactoring.
>
> On Mon, Aug 30, 2021 at 11:48 AM Ashesh Vashi <
> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>
>> Hi Team,
>>
>> Please find the attached patch for the two-factor authentication.
>> It supports sending an OTP to email, or use google authenticator to
>> increase the authentication process.
>>
>> As discussed earlier, I have followed the following design for
>> authentication.
>> [image: Screenshot 2021-06-17 at 7.09.47 PM.png]
>>
>>
>> Obviously - it will only be available for server mode.
>>
>> --
>>
>> Thanks & Regards,
>>
>> Ashesh Vashi
>> EnterpriseDB INDIA: Enterprise PostgreSQL Company
>> <http://www.enterprisedb.com>
>>
>>
>> *http://www.linkedin.com/in/asheshvashi*
>> <http://www.linkedin.com/in/asheshvashi>
>>
>

--
*Thanks & Regards*
*Akshay Joshi*
*pgAdmin Hacker | Principal Software Architect*
*EDB Postgres <http://edbpostgres.com>*

*Mobile: +91 976-788-8246*

In response to

Responses

Browse pgadmin-hackers by date

  From Date Subject
Next Message Nico Rikken 2021-08-31 07:55:26 Re: [pgAdmin][PATCH] Add OAUTH2_SCOPE variable for scope configuration
Previous Message Khushboo Vashi 2021-08-31 07:30:56 Re: [pgAdmin][PATCH] Add OAUTH2_SCOPE variable for scope configuration