Skip to content

Conversation

Andy-Grigg
Copy link
Contributor

Closes #137

The OIDCCallbackHTTPServer wasn't correctly releasing TCP port 32284. This caused problems if we tried to authenticate to a second OIDC server in the same script, e.g. by using the STK.

This PR re-works the OIDCCallbackHTTPServer implementation to use handle_request() instead of serve_forever(), which seems to eliminate the need to run the server in a separate thread. Instead, we now synchronously wait the login_timeout length of time for the server to handle a request. If it does, we return the auth_code.

I have also added tests that check the status of the TCP port both when the server is running and when it has been closed.

@Andy-Grigg Andy-Grigg requested a review from da1910 February 14, 2022 23:54
@Andy-Grigg Andy-Grigg changed the title Fix/release OIDC port binding Ensure OIDC Callback Server releases bound TCP port Feb 14, 2022
@codecov
Copy link

codecov bot commented Feb 14, 2022

Codecov Report

Merging #138 (5a1f8ea) into main (8ba5ec2) will increase coverage by 0.18%.
The diff coverage is 27.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #138      +/-   ##
==========================================
+ Coverage   91.65%   91.83%   +0.18%     
==========================================
  Files           8        8              
  Lines         827      821       -6     
==========================================
- Hits          758      754       -4     
+ Misses         69       67       -2     
Impacted Files Coverage Δ
src/ansys/openapi/common/_oidc.py 74.49% <11.11%> (+1.16%) ⬆️
src/ansys/openapi/common/_util.py 99.44% <100.00%> (-0.02%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ba5ec2...5a1f8ea. Read the comment docs.

@Andy-Grigg
Copy link
Contributor Author

Andy-Grigg commented Feb 15, 2022

Will probably be superseded by #139

We have decided to merge this fix for the upcoming 1.0.0 release. #139 will be merged following the release.

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.

This all looks OK, the test is a nice touch!

@Andy-Grigg Andy-Grigg merged commit ba85923 into main Feb 15, 2022
@Andy-Grigg Andy-Grigg deleted the fix/release-oidc-port-binding branch February 15, 2022 18:17
@Andy-Grigg
Copy link
Contributor Author

We are aware that the coverage in this code area is sub-optimal. #139 will resolve this

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.

STK and openapi-common OIDC connections are not compatible

2 participants