bravado icon indicating copy to clipboard operation
bravado copied to clipboard

Making requests_client.py's Authenticator URL matches() port-tolerant

Open myselfhimself opened this issue 5 years ago • 6 comments

Authenticator::matches()'s documentation advertises full host, scheme and port match, when it just checks for hosts. However when checking hosts, it cannot compare well a configured self.host:withPort (eg. localhost:8080) with the outgoing request full URL's host. Such a corner case can happen with this piece of code: http_client.set_basic_auth( 'localhost:8080', BASIC_AUTH_LOGIN, BASIC_AUTH_PASSWORD )

myselfhimself avatar Jul 19 '18 13:07 myselfhimself

Same issue with the set_api_key() method. This makes authenticated bravado requests fail when unit testing on a local server hosted at localhost:8080..

myselfhimself avatar Jul 19 '18 13:07 myselfhimself

Coverage Status

Coverage remained the same at 98.499% when pulling 16cf8ea151d0b3e7f3377321bdba0e17ed03bbb9 on myselfhimself:myselfhimself-non-port-tolerant-authenticator-matches into d30b9bdd612d6dc282b18cc3ba9fd0e47b2dd74d on Yelp:master.

coveralls avatar Jul 19 '18 13:07 coveralls

:arrow_up:

axsapronov avatar Nov 26 '19 07:11 axsapronov

Hello, I am not working on this topic anymore and will not increase the code coverage of this PR sorry.

Le mar. 26 nov. 2019 à 08:38, Alexander Sapronov [email protected] a écrit :

⬆️

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Yelp/bravado/pull/384?email_source=notifications&email_token=AAJU5QX7RWNVQHGOFU2QGOLQVTG7PA5CNFSM4FKZPXCKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFFA7MY#issuecomment-558501811, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJU5QX7Y3I5FSUQBA7DNH3QVTG7PANCNFSM4FKZPXCA .

myselfhimself avatar Nov 26 '19 07:11 myselfhimself

This has been reworked in the meantime (in #444), and now compares host and port (correctly). Are you able to test with the latest master and confirm that it fixes the issue for you @myselfhimself ? I could then make a release that includes the change.

sjaensch avatar Feb 06 '20 08:02 sjaensch

Hello, I am sorry I do not have access to the customer's codebase anymore and have changed working fields so will not assist in testing more for now :-/ :)

čt 6. 2. 2020 v 9:21 odesílatel Stephan Jaensch [email protected] napsal:

This has been reworked in the meantime (in #444 https://github.com/Yelp/bravado/pull/444), and now compares host and port (correctly). Are you able to test with the latest master and confirm that it fixes the issue for you @myselfhimself https://github.com/myselfhimself ? I could then make a release that includes the change.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Yelp/bravado/pull/384?email_source=notifications&email_token=AAJU5QUJIMBYSUSYIALZBL3RBPB7VA5CNFSM4FKZPXCKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEK6KL7Q#issuecomment-582788606, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJU5QURNZGVBU7G32SGS73RBPB7VANCNFSM4FKZPXCA .

myselfhimself avatar Feb 07 '20 15:02 myselfhimself