public inbox for [email protected]
help / color / mirror / Atom feedFrom: Khushboo Vashi <[email protected]>
To: Aditya Toshniwal <[email protected]>
Cc: Akshay Joshi <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Subject: Re: [pgAdmin4][Patch] - RM 5457 - Kerberos Authentication - Phase 1
Date: Wed, 13 Jan 2021 14:35:38 +0530
Message-ID: <CAFOhELeFZAXSWQYFoRGvfOEZ+Kt_sWWxPFte17o-maqq0JshXg@mail.gmail.com> (raw)
In-Reply-To: <CAM9w-_koVr9Qy3hDaSaDC9pX6jeDzj5gSghYjT2yxQ2h6zbr=w@mail.gmail.com>
References: <CAFOhELdXhWMR2zS4dnH+SudN0s7LiENH+vczC0YhuifPgm+G5g@mail.gmail.com>
<CANxoLDeKWqKP-6=KRTY_-vDSZv2g=P7dKJkzWPS7aOB3EoZOoA@mail.gmail.com>
<CAM9w-_koVr9Qy3hDaSaDC9pX6jeDzj5gSghYjT2yxQ2h6zbr=w@mail.gmail.com>
Hi,
Please find the attached updated patch.
Thanks,
Khushboo
On Fri, Jan 1, 2021 at 1:07 PM Aditya Toshniwal <
[email protected]> 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.
> [email protected]("/kerberos_login",
>
> + endpoint="kerberos_login", methods=["GET"])
>
>
> [email protected]("/kerberos_logout",
>
> + endpoint="kerberos_logout", methods=["GET"])
>
>
>
>
> On Tue, Dec 22, 2020 at 6:07 PM Akshay Joshi <
> [email protected]> wrote:
>
>> Hi Aditya
>>
>> Can you please do the code review?
>>
>> On Tue, Dec 22, 2020 at 3:44 PM Khushboo Vashi <
>> [email protected]> 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"
>
Attachments:
[application/octet-stream] RM_5457_v1.patch (1.4K, 3-RM_5457_v1.patch)
download | inline diff:
diff --git a/web/pgadmin/static/js/sqleditor/macro.js b/web/pgadmin/static/js/sqleditor/macro.js
index c8f9292f3..da4f3b4c7 100644
--- a/web/pgadmin/static/js/sqleditor/macro.js
+++ b/web/pgadmin/static/js/sqleditor/macro.js
@@ -272,11 +272,12 @@ let MacroDialog = {
<a class="dropdown-item" id="btn-manage-macros" href="#" tabindex="0">
<span> ${gettext('Manage Macros...')} </span>
</a>
- </li>
- <li class="dropdown-divider"></li>`;
+ </li>`;
+
+ let macro_list_tmpl = '';
_.each(macros, function(m) {
if (m.name) {
- str += `<li>
+ macro_list_tmpl += `<li>
<a class="dropdown-item btn-macro" data-macro-id="${m.id}" href="#" tabindex="0">
<span> ${_.escape(m.name)} </span>
<span> (${m.key_label}) </span>
@@ -285,6 +286,7 @@ let MacroDialog = {
}
});
+ if (macro_list_tmpl.length > 0) str += '<li class="dropdown-divider"></li>' + macro_list_tmpl;
$($.find('div.btn-group.mr-1.user_macros ul.dropdown-menu')).html($(str));
self.close(); // Close the dialog now
view thread (32+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: [pgAdmin4][Patch] - RM 5457 - Kerberos Authentication - Phase 1
In-Reply-To: <CAFOhELeFZAXSWQYFoRGvfOEZ+Kt_sWWxPFte17o-maqq0JshXg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox