Re: [pgAdmin4][Patch] - RM 5457 - Kerberos Authentication - Phase 1

Hi,

Please find the attached updated patch.

Thanks,
Khushboo

On Fri, Jan 1, 2021 at 1:07 PM Aditya Toshniwal <
aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:

> Hi Khushboo,
>
> I've just done the code review. Apart from below, the patch looks good to
> me:
>
> 1) Move the auth source constants -ldap, kerberos out of app object. They
> don't belong there. You can create the constants somewhere else and import
> them.
>
> +app.PGADMIN_LDAP_AUTH_SOURCE = 'ldap'
>
> +app.PGADMIN_KERBEROS_AUTH_SOURCE = 'kerberos'
>
>
> Done

> 2) Are we going to make kerberos default for wsgi ?
>
> *--- a/web/pgAdmin4.wsgi*
>
> *+++ b/web/pgAdmin4.wsgi*
>
> @@ -24,6 +24,10 @@ builtins.SERVER_MODE = True
>
>
>
> import config
>
>
>
> +
>
> +config.AUTHENTICATION_SOURCES = ['kerberos']
>
> +config.KERBEROS_AUTO_CREATE_USER = True
>
> +
>
>
> Removed, it was only for testing.

> 3) Remove the commented code.
>
> + # if self.form.data['email'] and self.form.data['password']
> and \
>
> + # source.get_source_name() ==\
>
> + # current_app.PGADMIN_KERBEROS_AUTH_SOURCE:
>
> + # continue
>
>
> Removed the comment, it is actually the part of the code.

> 4) KERBEROSAuthentication could be KerberosAuthentication
>
> class KERBEROSAuthentication(BaseAuthentication):
>
>
> Done.

> 5) You can use the constants (ldap, kerberos) you had defined when
> creating a user.
>
> + 'auth_source': 'kerberos'
>
>
> Done.

> 6) The below URLs belong to the authenticate module. Currently they are in
> the browser module. I would also suggest rephrasing the URL from
> /kerberos_login to /login/kerberos. Same for logout.
>
Done the rephrasing as well as moved to the authentication module.

> Also, even though the method GET works, we should use the POST method for
> login and DELETE for logout.
>
Kerberos_login just redirects the page to the actual login, so no need for
the POST method.
I followed the same method for the Logout user we have used for the normal
user.

> +(at)blueprint(dot)route("/kerberos_login",
>
> + endpoint="kerberos_login", methods=["GET"])
>
>
> +(at)blueprint(dot)route("/kerberos_logout",
>
> + endpoint="kerberos_logout", methods=["GET"])
>
>
>
>

> On Tue, Dec 22, 2020 at 6:07 PM Akshay Joshi <
> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>
>> Hi Aditya
>>
>> Can you please do the code review?
>>
>> On Tue, Dec 22, 2020 at 3:44 PM Khushboo Vashi <
>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>
>>> Hi,
>>>
>>> Please find the attached patch to support Kerberos Authentication in
>>> pgAdmin RM 5457.
>>>
>>> The patch introduces a new pluggable option for Kerberos authentication,
>>> using SPNEGO to forward kerberos tickets through a browser which will
>>> bypass the login page entirely if the Kerberos Authentication succeeds.
>>>
>>> The complete setup of the Kerberos Server + pgAdmin Server + Client is
>>> documented in a separate file and attached.
>>>
>>> This patch also includes the small fix related to logging #5829
>>>
>>> Thanks,
>>> Khushboo
>>>
>>
>>
>> --
>> *Thanks & Regards*
>> *Akshay Joshi*
>> *pgAdmin Hacker | Principal Software Architect*
>> *EDB Postgres <http://edbpostgres.com>*
>>
>> *Mobile: +91 976-788-8246*
>>
>
>
> --
> Thanks,
> Aditya Toshniwal
> pgAdmin hacker | Sr. Software Engineer | *edbpostgres.com*
> <http://edbpostgres.com>
> "Don't Complain about Heat, Plant a TREE"
>