Skip to content

Conversation

Andy-Grigg
Copy link
Contributor

Closes #636

Add a new builder method which doesn't check the 401-response header and attempts to use basic auth. I also did some cleanup of the return type of the __test_connection() method, since it returns Literal[True] instead of bool.

I thought about the best way to implement this. We could have used an undocumented environment variable, but given that this is an open-source project it seemed wiser to implement this function, but to be very clear in the documentation that it should not be used in almost all cases.

I did think about marking the method as deprecated, but I think we can only really get away with this once Basic is included in the 401-response alongside Bearer from MI.

@Andy-Grigg Andy-Grigg requested a review from da1910 August 15, 2024 20:27
@github-actions github-actions bot added the enhancement New features or code improvements label Aug 15, 2024
@Andy-Grigg Andy-Grigg changed the title Add a pre-emptive basic auth method Support pre-emptive Basic authentication Aug 15, 2024
Copy link

codecov bot commented Aug 15, 2024

Codecov Report

Attention: Patch coverage is 97.36842% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.45%. Comparing base (d1851a8) to head (195fc95).
Report is 1 commits behind head on main.

Files Patch % Lines
src/ansys/openapi/common/_session.py 97.29% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #638      +/-   ##
==========================================
+ Coverage   94.24%   94.45%   +0.20%     
==========================================
  Files           7        7              
  Lines         782      793      +11     
==========================================
+ Hits          737      749      +12     
+ Misses         45       44       -1     

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

@wiz-inc-572fc38784
Copy link

Wiz Scan Summary

IaC Misconfigurations 0C 0H 0M 0L 0I
Vulnerabilities 0C 0H 1M 0L 1I
Sensitive Data 0C 0H 0M 0L 0I
Total 0C 0H 1M 0L 1I
Secrets 0🔑

@github-actions github-actions bot added documentation Improvements or additions to documentation dependencies Related with project dependencies maintenance Package and maintenance related labels Aug 16, 2024
@Andy-Grigg
Copy link
Contributor Author

Andy-Grigg commented Aug 16, 2024

Re-implemented this for with_autologon and with_credentials, with an enum that allows the authentication method to be chosen with each method. This rapidly gets complicated with different modes supported on different platforms.

Still TODO:

  • Fix a failing kerberos test - This was an invalid test case
  • Add unadvertised basic test
  • Add unadvertised kerberos test
  • Document the Enum
  • Perform some manual testing in Windows
  • Perform some manual testing in Linux

We don't have any credentialed windows auth tests (NTLM, kerberos, or negotiate) as far as I can see. We'll have to test this manually and check the server logs to see that there's no 401 response generated.

@Andy-Grigg Andy-Grigg changed the title Support pre-emptive Basic authentication Allow auth mode to be specified explicitly on 'with_credentials' and 'with_autologon' methods Aug 17, 2024
@Andy-Grigg Andy-Grigg changed the title Allow authenticate scheme to be specified explicitly on 'with_credentials' and 'with_autologon' methods Allow authentication scheme to be specified explicitly on 'with_credentials' and 'with_autologon' methods Aug 19, 2024
Copy link
Collaborator

@da1910 da1910 left a comment

Choose a reason for hiding this comment

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

Not sure what to do at this point, the requests-negotiate-sspi library won't allow us to force it to send the initial handshake if the server doesn't advertise it supports Negotiate.

@Andy-Grigg Andy-Grigg changed the title Allow authentication scheme to be specified explicitly on 'with_credentials' and 'with_autologon' methods Allow authentication scheme to be specified explicitly when connecting with credentials Aug 20, 2024
@Andy-Grigg Andy-Grigg requested a review from da1910 August 20, 2024 15:16
Copy link
Collaborator

@da1910 da1910 left a comment

Choose a reason for hiding this comment

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

Looks as discussed

@Andy-Grigg Andy-Grigg added this pull request to the merge queue Aug 20, 2024
Merged via the queue into main with commit 646f188 Aug 20, 2024
@da1910 da1910 deleted the feat/pre-emptive-basic-auth branch August 20, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Related with project dependencies documentation Improvements or additions to documentation enhancement New features or code improvements maintenance Package and maintenance related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add pre-emptive basic auth as an option when using with_credentials()

3 participants