Provide option to retry calls to ScriptAuthorizer.refresh #120
Conversation
MaybeNetwork
force-pushed
the
retry-refresh
branch
2 times, most recently
from
d0883fe to
11ac066
Compare
There was a problem hiding this comment.
I like what this PR is trying to solve. However, I'd like to recommend solving it a different way.
Rather than providing control logic within the response of the two_factor_callback, I'd like to have the number of two_factor_callback retries be configured as we configure other values (and raise an exception if the value is too large -- if we really want to control that).
I don't think we should introduce any concept of delay, as that can be introduced directly into the two_factor_callback method to ensure it's not submitting the same code more than once (we could add documentation to that effect, or even provide an example).
CHANGES.rst
Outdated
| @@ -9,6 +9,14 @@ Unreleased | |||
| **Added** | |||
|
|
|||
| - 301 redirects result in a ``Redirect`` exception. | |||
| - Calls to method ``ScriptAuthorizer.refresh`` are handled by a private method, | |||
| ``ScriptAuthorizer._refresh_with_retries``. The latter provides the option to retry | |||
There was a problem hiding this comment.
I don't think it's necessary to mention the private method in the change log. Just that retries are now supported.
prawcore/auth.py
Outdated
| :param two_factor_callback: A function that returns OTPs (One-Time | ||
| Passcodes), also known as 2FA auth codes. If this function is | ||
| provided, prawcore will call it when authenticating. | ||
| :param two_factor_callback: (Optional) A function that returns OTPs (One-Time |
There was a problem hiding this comment.
Let's not use OTP or 2FA in the documentation and instead say two factor code.
Also, I prefer to not use tuples for return values unless it's at most a pair. Rather a dictionary with named values makes the result more flexible in the event we want to make changes.
There was a problem hiding this comment.
I like what this PR is trying to solve. However, I'd like to recommend solving it a different way.
Rather than providing control logic within the response of the
two_factor_callback, I'd like to have the number oftwo_factor_callbackretries be configured as we configure other values (and raise an exception if the value is too large -- if we really want to control that).I don't think we should introduce any concept of delay, as that can be introduced directly into the
two_factor_callbackmethod to ensure it's not submitting the same code more than once (we could add documentation to that effect, or even provide an example).
Edit: Sorry, I misunderstood. Moving the delay to the callback is not a problem.
prawcore/auth.py
Outdated
| if delay > 0: | ||
| time.sleep(delay) | ||
| additional_kwargs = {} | ||
| otp = self._two_factor_callback and self._two_factor_callback() |
There was a problem hiding this comment.
Will you replace otp with two_factor_code.
prawcore/auth.py
Outdated
| except OAuthException: | ||
| if otp and isinstance(otp, tuple) and len(otp) == 3: | ||
| _, delay, maxcount = otp | ||
| if count >= min(maxcount, 10): |
There was a problem hiding this comment.
Rather than hidden limit to 10, I'd prefer raising an exception if >10 is provided.
There was a problem hiding this comment.
I'm not sure that raising a separate exception from the last OAuthException after 10 invalid grants is better than just documenting the limit and raising the last OAuthException when the limit is hit. I could be conviced otherwise though.
tests/test_authorizer.py
Outdated
| "dummy", | ||
| two_factor_callback=lambda: ("123456", 31, 2), | ||
| ) | ||
| with self.assertRaises(prawcore.OAuthException): |
There was a problem hiding this comment.
This test should probably make an assertion on the number of times that the lambda function is called (may need to write an actual function to keep track of the count).
MaybeNetwork
force-pushed
the
retry-refresh
branch
5 times, most recently
from
e8cb905 to
bfcbb6b
Compare
MaybeNetwork
force-pushed
the
retry-refresh
branch
from
bfcbb6b to
ea07207
Compare
Feature Summary and Justification
The password flow with 2fa has two circumstances under which it is desirable to retry (or more accurately, re-do) an authorization attempt that results in an invalid grant. The first case is the one where a user accidentally enters the wrong OTP. Instead of immediately raising an OAuthException, which would be inconvenient in the middle of a batch request, it would be helpful to let the user try again. The second case is the one where a user supplies all of the right credentials, but authorizes at the wrong time. Reddit doesn't allow the same OTP to be used to obtain two access tokens, as a security precaution against the code being intercepted. It would be helpful if there was an option to re-do the authorization request after a delay.
The proposed solution is to let the two factor callback return the otp as either a string or a 3-tuple of the form
(<OTP>, <DELAY>, <TRIES>), and to changeScriptAuthorizer.refreshaccordingly. Prawcore would use then use the form of the OTP to determine if and how to re-do attempts to refresh the authorization when it encounters an OAuthException.I have a PR ready to close the PRAW PR, and it could proceed without this change, but this would make things easier.