Re: Patch: Two-factor Authentication (RM #6543) - Mailing list pgadmin-hackers

On Tue, Aug 31, 2021 at 1:03 PM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:

Hi Ashesh 

Hi Team,

Please find the updated patch with the following review comments, and also some fixes related to validating the code on email authentication,found during unit testing. 

Following are the review comments:

• GUI:
  • The two-factor authentication dialog's size is too big, can we make it a bit small?

As discussed in the review meeting, let's keep it as is.

I've modified the backgrid class to work better with smaller resolution too.

• Dialog's header is showing "AlertifyJS", please provide some valid header name.

Done 

• 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.

I will work with Nidhi to add more robust documentation. 

• 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.

As discussed in the review meeting, we won't implement that.

You can create a RM in case you're keen to work on it in future.

• Code:
  • Copyright headers are missing in some of the new files.

Done. 

• Add comments/function descriptions wherever needed. It will be easy for others to understand the logic.

Done. 

• Remove unused imports from the new files.

Done. 

• Sonarlint showing warning "Replace <i> tag with <em>" for some HTML files.

We always use <i> for icons. It's not an issue, you can ignore it. 

• send_email_otp.html and send_email_otp.txt both files having the same content, they both are needed?

Both are needed, as it depends on EMAIL_PLAINTEXT and EMAIL_HTML settings of the flask_security package, which is used to send the email.

• In "test_mfa_view.py" file line no 26 showing "Unresolved reference ValidationException"

Done 

• In the "mfa/tests/utils.py" file Remove unused class declaration "class DummyAuth". Also, update the "return true" with "return True" at line no 31th 

That's necessary - that is added for a reason for adding unit test cases for defining a auth class for testing purpose.

An object of the class is initialized automatically by the registry, so - it is required. I've add # NOSONAR to suppression the warning.

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

It is out of scope of this patch. 

-- Thanks, Ashesh

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.vashi@enterprisedb.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.vashi@enterprisedb.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.

Obviously - it will only be available for server mode.

--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company

http://www.linkedin.com/in/asheshvashi

--

Thanks & Regards

Akshay Joshi

pgAdmin Hacker | Principal Software Architect

EDB Postgres

Mobile: +91 976-788-8246