MIME-Version: 1.0
References: <24c38b02-7b18-13e7-b42a-d70194211f21@posteo.de>
In-Reply-To: <24c38b02-7b18-13e7-b42a-d70194211f21@posteo.de>
From: Dave Page <dpage@pgadmin.org>
Date: Tue, 30 Mar 2021 15:48:32 +0000
Message-ID: 
 <CA+OCxoz72Hs6t35AXD7RWkPq6othvAFEg-_N7brUkEOyOTYfHA@mail.gmail.com>
Subject: Re: OAUTH2 implementation
To: Florian Sabonchi <sabonchi@posteo.de>
Cc: pgadmin-hackers <pgadmin-hackers@postgresql.org>
Content-Type: multipart/alternative; boundary="000000000000da05f505bec2eed9"
Archived-At: 
 <https://www.postgresql.org/message-id/CA%2BOCxoz72Hs6t35AXD7RWkPq6othvAFEg-_N7brUkEOyOTYfHA%40mail.gmail.com>
Precedence: bulk

--000000000000da05f505bec2eed9
Content-Type: text/plain; charset="UTF-8"

Hi

On Tue, Mar 30, 2021 at 3:36 PM Florian Sabonchi <sabonchi@posteo.de> wrote:

> Hello in this patch I have implemented oauth2
>
> Cool!

Unfortunately the patch seems to be messed up. It adds a number of commits
that are already in the primary repo, and attempts to remove your OAuth
support, rather than adding it. Can you rebase it and make sure it only
includes the addition of your work please?

Some other comments (keep in mind it's hard to read the mangled patch, so I
may be missing something):

- There don't seem to be any documentation updates
- There don't seem to be any tests (which I grant may not be feasible to
add unless an OAuth service can be mocked)
- I can't see how you've dealt with password saving, which currently
requires a password from the user to be secure.

-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EDB: http://www.enterprisedb.com

--000000000000da05f505bec2eed9
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr">Hi</div><br><div class=3D"gmail_quote"><d=
iv dir=3D"ltr" class=3D"gmail_attr">On Tue, Mar 30, 2021 at 3:36 PM Florian=
 Sabonchi &lt;<a href=3D"mailto:sabonchi@posteo.de">sabonchi@posteo.de</a>&=
gt; wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0=
px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-colo=
r:rgb(204,204,204);padding-left:1ex">Hello in this patch I have implemented=
 oauth2<br>
<br>
</blockquote></div>Cool!<div><br></div><div>Unfortunately the patch seems t=
o be messed up. It adds a number of commits that are already in the primary=
 repo, and attempts to remove your OAuth support, rather than adding it. Ca=
n you rebase it and make sure it only includes the addition of your work pl=
ease?</div><div><br></div><div>Some other comments (keep in mind it&#39;s h=
ard to read the mangled patch, so I may be missing something):</div><div><b=
r></div><div>- There don&#39;t seem to be any documentation updates</div><d=
iv>- There don&#39;t seem to be any tests (which I grant may not be feasibl=
e to add unless an OAuth service can be mocked)</div><div>- I can&#39;t see=
 how you&#39;ve dealt with password saving, which currently requires a pass=
word from the user to be secure.<br clear=3D"all"><div><br></div>-- <br><di=
v dir=3D"ltr" class=3D"gmail_signature"><div dir=3D"ltr">Dave Page<br>Blog:=
 <a href=3D"http://pgsnake.blogspot.com" target=3D"_blank">http://pgsnake.b=
logspot.com</a><br>Twitter: @pgsnake<br><br>EDB: <a href=3D"http://www.ente=
rprisedb.com" target=3D"_blank">http://www.enterprisedb.com</a><br><br></di=
v></div></div></div>

--000000000000da05f505bec2eed9--