Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NXPY-254: Authorization Error for OAuth #302

Merged
merged 12 commits into from
Apr 2, 2024

Conversation

gitofanindya
Copy link
Collaborator

No description provided.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Type: Enhancement

PR Summary: This pull request introduces enhancements to the OAuth2 authentication mechanism within the Nuxeo Python client. Specifically, it adds the ability to verify SSL certificates during the OAuth2 authorization process by introducing a new 'verify' attribute. This attribute is used in various methods to ensure that SSL verification can be enabled or disabled as needed. The changes include initializing this attribute in the class constructor, modifying the token refresh logic to accept this parameter, and ensuring it is passed correctly when requesting and refreshing tokens.

Decision: Comment

📝 Type: 'Enhancement' - not supported yet.
  • Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
📝 Complexity: the changes are too large or complex for Sourcery to approve.
  • Unsupported files: the diff contains files that Sourcery does not currently support during reviews.

General suggestions:

  • Consider adding more detailed documentation or comments explaining the purpose and usage of the 'verify' parameter, especially for users who might not be familiar with SSL certificate verification in HTTP requests.
  • It might be beneficial to include examples or use cases in the documentation where disabling SSL verification could be necessary or advised, providing users with clearer guidance on how to use this new feature securely.
  • Ensure thorough testing of both scenarios where SSL verification is enabled and disabled. This includes testing with valid and invalid certificates to confirm that the behavior aligns with expectations and does not introduce security vulnerabilities.

Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

nuxeo/auth/oauth2.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Feb 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.08%. Comparing base (54400d7) to head (1b35ebf).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #302      +/-   ##
==========================================
+ Coverage   96.63%   97.08%   +0.45%     
==========================================
  Files          29       29              
  Lines        1811     1819       +8     
==========================================
+ Hits         1750     1766      +16     
+ Misses         61       53       -8     
Flag Coverage Δ
functional 89.60% <10.00%> (-0.40%) ⬇️
unit 52.22% <100.00%> (+0.65%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gitofanindya gitofanindya force-pushed the wip-NXPY-254-Authorization-Error-for-OAuth branch 4 times, most recently from fe40a96 to 164da71 Compare February 22, 2024 08:52
@gitofanindya gitofanindya force-pushed the wip-NXPY-254-Authorization-Error-for-OAuth branch from bd36205 to c47552f Compare February 27, 2024 09:25
Copy link
Collaborator

@mr-shekhar mr-shekhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test cases are not properly written. Pl. follow the standard practise of writing testcases, as discussed.

@gitofanindya
Copy link
Collaborator Author

An SSL/TLS certificate is a digital object that allows systems to verify the identity & subsequently establish an encrypted network connection to another system using the Secure Sockets Layer/Transport Layer Security (SSL/TLS) protocol. Certificates are used within a cryptographic system known as a public key infrastructure (PKI). PKI provides a way for one party to establish the identity of another party using certificates if they both trust a third-party - known as a certificate authority. SSL/TLS certificates thus act as digital identity cards to secure network communications, establish the identity of websites over the Internet as well as resources on private networks.

A self-signed TLS/SSL certificate is not signed by a publicly trusted certificate authority (CA) but instead by the developer or company that is responsible for the website; as they are not signed by a publicly trusted CA, they are usually considered unsafe for public applications and websites.
The role of a public CA is to guarantee the validity of the information included in a certificate, especially the ownership and/or control of the domain name(s) associated with the certificate in the case of [TLS/SSL]. Therefore, using self-signed certificates is the equivalent of using credentials that have not been issued by a valid authority.

However, if a self-signed certificate needs to be used, we need to turn off the certificate verification as it is not signed by any trusted CA, the verification will fail.

This PR adds improvements to the Nuxeo Python client's OAuth2 authentication system. In particular, it introduces a new 'verify' feature that allows SSL certificates to be verified during the OAuth2 authorization process. This property ensures that SSL verification can be turned on or off as needed in a number of ways. The adjustments include setting this parameter's initial value in the class constructor, adapting the token refresh mechanism to take it, and making sure it is supplied accurately when requesting and refreshing tokens.

Copy link
Collaborator

@mr-shekhar mr-shekhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@gitofanindya gitofanindya merged commit dd53b7a into master Apr 2, 2024
8 checks passed
@gitofanindya gitofanindya deleted the wip-NXPY-254-Authorization-Error-for-OAuth branch April 2, 2024 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants