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

On Tue, Sep 28, 2021 at 5:54 PM Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com>
wrote:

> Thanks, the patch applied with some documentation changes.
>
Thanks - Akshay!

--

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>

>
> On Thu, Sep 23, 2021 at 1:36 PM Ashesh Vashi <
> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>
>> On Tue, Aug 31, 2021 at 1:03 PM Akshay Joshi <
>> akshay(dot)joshi(at)enterprisedb(dot)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(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*
>>>
>>
>
> --
> *Thanks & Regards*
> *Akshay Joshi*
> *pgAdmin Hacker | Principal Software Architect*
> *EDB Postgres <http://edbpostgres.com>*
>
> *Mobile: +91 976-788-8246*
>