Re: Patch: Two-factor Authentication (RM #6543) - Mailing list pgadmin-hackers
From | Ashesh Vashi |
---|---|
Subject | Re: Patch: Two-factor Authentication (RM #6543) |
Date | |
Msg-id | CAG7mmowhW0sY4naXKm=7b3fN2_OxsOd5iv1kBUh56OUUZnxA-A@mail.gmail.com Whole thread Raw |
In response to | Re: Patch: Two-factor Authentication (RM #6543) (Akshay Joshi <akshay.joshi@enterprisedb.com>) |
List | pgadmin-hackers |
On Tue, Sep 28, 2021 at 5:54 PM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
Thanks - Akshay!Thanks, the patch applied with some documentation changes.
On Thu, Sep 23, 2021 at 1:36 PM Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:On Tue, Aug 31, 2021 at 1:03 PM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:Hi AsheshHi 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, AsheshNote: 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 & RegardsAkshay JoshipgAdmin Hacker | Principal Software ArchitectEDB PostgresMobile: +91 976-788-8246--Thanks & RegardsAkshay JoshipgAdmin Hacker | Principal Software ArchitectEDB PostgresMobile: +91 976-788-8246
Attachment
pgadmin-hackers by date: