Thread: [pgAdmin][Patch] - RM #5940 - Add support for Oauth 2 authentication
Hi,
Please find the attached patch for the RM #5940 - Add support for Oauth 2 authentication.
The patch includes:
- Pluggable Oauth 2 authentication support with the multiple providers
- Configurable support for Master Key in Oauth2 and Kerberos authentication, so user can save the database server password
- Introduced the separate module/blueprint for kerberos and Oauth2 under authentication module for the readability and maintainability
Note: The initial patch for Oauth2 authentication was sent by Florian Sabonchi, I have worked on top of it.
Thanks,
Khushboo
Attachment
Hi,
Please find the attached rebased patch.
Thanks,
Khushboo
On Thu, Jun 17, 2021 at 4:22 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:
Hi,Please find the attached patch for the RM #5940 - Add support for Oauth 2 authentication.The patch includes:- Pluggable Oauth 2 authentication support with the multiple providers- Configurable support for Master Key in Oauth2 and Kerberos authentication, so user can save the database server password- Introduced the separate module/blueprint for kerberos and Oauth2 under authentication module for the readability and maintainabilityNote: The initial patch for Oauth2 authentication was sent by Florian Sabonchi, I have worked on top of it.Thanks,Khushboo
Attachment
Hi Khushboo
Following are the review comments:
Scenario : AUTHENTICATION_SOURCES = ['oauth2', 'internal']
- Don't set the OAUTH2_DISPLAY_NAME, Login button shows 'Login with None' which should be change. Maybe placeholder like <oauth2 display name>.
- Enter the email address and password. Click on the "Login with ..." button, internal server error displayed on the page, we should throw a proper error message.
Code Review:
- Empty "__init__.py" file is added in the web folder, I think that is not required.
- Any reason to change messages.pot file manually? It will be updated when we update the message catalog.
- Fixed SonarQube issues in the new file 'oauth2.py' and 'test_oauth2_with_mocking.py'. Remove unused imports from both files.
- Add copyright header to "kerberos.js" file.
Documentation:
- Remove "of the" from the description of OAUTH2_NAME it is duplicated.
- String "Authorisation" should be "Authorization".
- Instead of providing "Oauth2 secret", "Oauth2 base url" .... can we provide some more detailed information?
Note: I haven't tested the patch with the proper OAuth2 configuration.
On Mon, Jun 28, 2021 at 4:52 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:
Hi,Please find the attached rebased patch.Thanks,KhushbooOn Thu, Jun 17, 2021 at 4:22 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:Hi,Please find the attached patch for the RM #5940 - Add support for Oauth 2 authentication.The patch includes:- Pluggable Oauth 2 authentication support with the multiple providers- Configurable support for Master Key in Oauth2 and Kerberos authentication, so user can save the database server password- Introduced the separate module/blueprint for kerberos and Oauth2 under authentication module for the readability and maintainabilityNote: The initial patch for Oauth2 authentication was sent by Florian Sabonchi, I have worked on top of it.Thanks,Khushboo
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB PostgresMobile: +91 976-788-8246
Hi Akshay,
Thanks,
Khushboo
On Mon, Jul 5, 2021 at 5:39 PM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi KhushbooFollowing are the review comments:Scenario : AUTHENTICATION_SOURCES = ['oauth2', 'internal']
- Don't set the OAUTH2_DISPLAY_NAME, Login button shows 'Login with None' which should be change. Maybe placeholder like <oauth2 display name>.
Fixed
- Enter the email address and password. Click on the "Login with ..." button, internal server error displayed on the page, we should throw a proper error message.
Fixed
Code Review:
- Empty "__init__.py" file is added in the web folder, I think that is not required.
By mistake, it was added.
- Any reason to change messages.pot file manually? It will be updated when we update the message catalog.
I must have run make docs, removed.
- Fixed SonarQube issues in the new file 'oauth2.py' and 'test_oauth2_with_mocking.py'. Remove unused imports from both files.
Fixed
- Add copyright header to "kerberos.js" file.
Fixed
Documentation:
- Remove "of the" from the description of OAUTH2_NAME it is duplicated.
- String "Authorisation" should be "Authorization".
- Instead of providing "Oauth2 secret", "Oauth2 base url" .... can we provide some more detailed information?
Fixed.
Note: I haven't tested the patch with the proper OAuth2 configuration.On Mon, Jun 28, 2021 at 4:52 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:Hi,Please find the attached rebased patch.Thanks,KhushbooOn Thu, Jun 17, 2021 at 4:22 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:Hi,Please find the attached patch for the RM #5940 - Add support for Oauth 2 authentication.The patch includes:- Pluggable Oauth 2 authentication support with the multiple providers- Configurable support for Master Key in Oauth2 and Kerberos authentication, so user can save the database server password- Introduced the separate module/blueprint for kerberos and Oauth2 under authentication module for the readability and maintainabilityNote: The initial patch for Oauth2 authentication was sent by Florian Sabonchi, I have worked on top of it.Thanks,Khushboo--Thanks & RegardsAkshay JoshipgAdmin Hacker | Principal Software ArchitectEDB PostgresMobile: +91 976-788-8246
Attachment
Thanks, the patch applied.
On Tue, Jul 6, 2021 at 12:01 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:
Hi Akshay,Please find the attached updated patch.Thanks,KhushbooOn Mon, Jul 5, 2021 at 5:39 PM Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:Hi KhushbooFollowing are the review comments:Scenario : AUTHENTICATION_SOURCES = ['oauth2', 'internal']
- Don't set the OAUTH2_DISPLAY_NAME, Login button shows 'Login with None' which should be change. Maybe placeholder like <oauth2 display name>.
Fixed
- Enter the email address and password. Click on the "Login with ..." button, internal server error displayed on the page, we should throw a proper error message.
FixedCode Review:
- Empty "__init__.py" file is added in the web folder, I think that is not required.
By mistake, it was added.
- Any reason to change messages.pot file manually? It will be updated when we update the message catalog.
I must have run make docs, removed.
- Fixed SonarQube issues in the new file 'oauth2.py' and 'test_oauth2_with_mocking.py'. Remove unused imports from both files.
Fixed
- Add copyright header to "kerberos.js" file.
FixedDocumentation:
- Remove "of the" from the description of OAUTH2_NAME it is duplicated.
- String "Authorisation" should be "Authorization".
- Instead of providing "Oauth2 secret", "Oauth2 base url" .... can we provide some more detailed information?
Fixed.Note: I haven't tested the patch with the proper OAuth2 configuration.On Mon, Jun 28, 2021 at 4:52 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:Hi,Please find the attached rebased patch.Thanks,KhushbooOn Thu, Jun 17, 2021 at 4:22 PM Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:Hi,Please find the attached patch for the RM #5940 - Add support for Oauth 2 authentication.The patch includes:- Pluggable Oauth 2 authentication support with the multiple providers- Configurable support for Master Key in Oauth2 and Kerberos authentication, so user can save the database server password- Introduced the separate module/blueprint for kerberos and Oauth2 under authentication module for the readability and maintainabilityNote: The initial patch for Oauth2 authentication was sent by Florian Sabonchi, I have worked on top of it.Thanks,Khushboo--Thanks & RegardsAkshay JoshipgAdmin Hacker | Principal Software ArchitectEDB PostgresMobile: +91 976-788-8246
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB PostgresMobile: +91 976-788-8246