-
Notifications
You must be signed in to change notification settings - Fork 1.1k
urequests: Added Basic Authentication to requests. #310
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes were obviously not tested, since the code does not work as is.
urequests/urequests.py
Outdated
raise TypeError("expected bytes, not %s" | ||
% altchars.__class__.__name__) | ||
assert len(altchars) == 2, repr(altchars) | ||
return encoded.translate(bytes.maketrans(b'+/', altchars)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The translate
method is not supported on MicroPython, neither is maketrans
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know. Shouldn't we resolve it here then? As mentioned, it's a simply a copy.
urequests/urequests.py
Outdated
% altchars.__class__.__name__) | ||
assert len(altchars) == 2, repr(altchars) | ||
return encoded.translate(bytes.maketrans(b'+/', altchars)) | ||
return encoded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This whole
b64encode
function is superfluous. Thealtchars
parameter is never used by the rest of the module and the only other actual functionality left then is being a wrapper forubinascii.b2a_base64
. This can be done inencode_basic_auth
directly. - Stripping the trailing newline is better done with
.rstrip(b'\n')
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good remark. Seems like I wasn't paying much attention to the functionality of b64encode
. Simply knew I needed it ;-)
urequests/urequests.py
Outdated
|
||
def encode_basic_auth(auth): | ||
auth_encoded = b64encode(b"%s:%s" % (auth[0], auth[1])).decode("ascii") | ||
return {'Authorization' : 'Basic %s' % auth_encoded} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several problems with this function too:
- Should take
user
andpassword
as separate params, not as a singleauth
param, to increase clarity. - Decoding the return value of
b64encode
is not necessary, since the header dict should contain byte strings anyway. - Non-pep-8-compliant whitespace.
Suggested version:
def encode_basic_auth(user, password):
from ubinascii import b2a_base64
auth_encoded = b2a_base64(b"%s:%s" % (user, password)).rstrip(b'\n')
return {b'Authorization': b'Basic %s' % auth_encoded}
Using the %
string formatting operator here allows for user
and password
to be passed as either str
or bytes
, which is a bit unclean and wouldn't work in CPython, but works in MicroPython.
urequests/urequests.py
Outdated
@@ -33,6 +33,9 @@ def json(self): | |||
|
|||
|
|||
def request(method, url, data=None, json=None, headers={}, stream=None): | |||
if auth is not None: | |||
headers.update(encode_basic_auth(auth)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
An
auth
parameter needs to be added to therequest
function signature (defaulting toNone
). -
If following my suggested changes for
encode_basic_auth
, this should be changed to the following then:if auth: headers.update(encode_basic_auth(auth[0], auth[1]))
(Also changing
if auth is not None
toif auth
so that passing an empty tuple works as well.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be also a good idea to allow passing a callable for auth (but this would be an extension not supported by the original requests.request()
.
if auth:
headers.update(auth() if callable(auth) else encode_basic_auth(auth[0], auth[1]))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously that mistake had to happen to me 🤦
Seems like I messed up when developing...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... this would be an extension not supported by the original
requests.request()
.
How are we dealing with that? Do we actually want to extend the original or simply replicate it's functionality? Is there any guidelines for that?
I have decided to open a new pull request with a working version and some of the changes suggested by @SpotlightKid. |
I'm getting this error with urequests. (micropython 1.12 from repo, ESP32)
Sorry about to open this thread again, but I cannot find the "auth" parameter in the actual code, even in the #311. |
micropython-lib/urequests/urequests.py Line 35 in 46e0fa1
|
The line in the master branch is as follow:
This pull request was not applied? |
No, as you can see from the PR still being open. |
To whoever it was who asked me whether I found a solution or not, the answer is sort of. I am using JWT (Token Auth) instead of Basic Auth for my esp32 Micropython project now. It gets and refreshes the token every 5 minutes from the server with the username and password passed as clear text params and then the tokens are used in the header of each request. Hope it helps. |
Good catch. Can you provide some basic example? |
No description provided.