839/offline refill #908

cornelinux · 2018-01-22T13:56:43Z

Closes #839

File: privacyidea/api/validate.py:L162-205
I think this line can be deleted, as refilltoken is defined later on.
correct.
File: privacyidea/lib/applications/offline.py:L52-114
Maybe we should add a log message in case the serial has no matching token? Because currently, in this case, a new refilltoken and an empty list are returned.
You mean something in case len(toks) != 1?
Sounds sensible to me.
Hmm yeah, looking at it again, the current behavior seems okay at this point :)
General Comment
# Codecov Report

Merging #908 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #908      +/-   ##
==========================================
+ Coverage   95.38%   95.42%   +0.03%
==========================================
Files         129      129
Lines       15913    15978      +65
==========================================
+ Hits        15179    15247      +68
+ Misses        734      731       -3

Impacted Files	Coverage Δ
privacyidea/lib/machine.py	`95.34% <ø> (ø)`	⬆️
privacyidea/lib/tokens/hotptoken.py	`100% <100%> (ø)`	⬆️
privacyidea/api/validate.py	`100% <100%> (ø)`	⬆️
privacyidea/lib/applications/offline.py	`100% <100%> (ø)`	⬆️
privacyidea/lib/tokens/u2f.py	`95.79% <0%> (+1.68%)`	⬆️
privacyidea/lib/tokens/vascotoken.py	`98.66% <0%> (+2.11%)`	⬆️

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 32f6906...7621cb3. Read the comment docs.

I tried to make the code a bit clearer in 2c6bfad.
There, we have two methods for the two cases:

get_offline_otps: just retrieve a number of offline OTPs (= hash(PIN + OTP))
get_appropriate_refill: given a PIN+OTP, calculate the size of the refill and call get_offline_otps to get the right amount of new offline OTPs

This looks good to me.
But I have two comments:

I would rename the get_appropriate_refill to get_refill. I always hope that the return value of a function is in fact appropriate ;-)
In the method get_authentication_item I think the password will never be empty. Because in get its value from the pass parameter from validate/check. (see https://github.com/privacyidea/privacyidea/blob/master/privacyidea/api/lib/postpolicy.py#L480) The API call will always have a pass - or not? Do we have a test case for the empty password?

We might have had an off-by-one error in get_refill until now:
Assume a user rolls out a new token and attaches it to a machine. Then, the user uses the OTP value with count = 0 to get 100 offline OTPs. Then, the user presses the button once (= OTP with count = 1) and uses the OTP to get a refill. Up until now, the refill response had 0 elements. But shouldn't it have 1 element, as the user has consumed one OTP value already?
I've attempted to fix the code and tests accordingly and pushed 243533d. But we should probably double-check that this is now the intended behavior :)
@fredreichbier: I am merging this now!
Then we adapt the pam module and check it in real live.
File: privacyidea/lib/applications/offline.py:L52-114
Hmm, I think in case the passed OTP value is not found, matching_count is set to -1, which results in a negative counter_diff later on, which results in a decreasing OTP counter?
File: privacyidea/lib/applications/offline.py:L110 (f122e40)
Usually I do this like
options = options or {}
I usually use the if options is None: options = {} approach because options = options or {} allocates a new dictionary in case options is the empty dictionary. But I guess this is just a matter of style :)
File: privacyidea/lib/applications/offline.py:L53-134
@cornelinux I'm wondering if this is even a legal case? In case of a refill, shouldn't the current token counter value always be higher than the refill amount?
I think you are right. But I am not sure here. Because in some tests I ran into this problem.
if first_old_counter < 0:
log.error("This should not happen")
;-)
And I think you said: If we have a negative value, the counter on the server would be decreased. So this might be the safe net for states, that should not occur!
Ah, I guess this could happen if a token gets 100 offline OTPs (i.e. the online token counter is 101), then the administrator increases the offline OTP count to 200 and the client attempts a refill (i.e. 101 - 200 = -99).
But apart from that, we ensure that we never decrease the counter on the server by checking for amount < 0 in get_offline_otps.
File: privacyidea/api/validate.py:L162-205
Is this still a problem or is this outdated due to your rewrite?
I forgot about this :) ce085ac now deals with error cases.

...with the tokeninfo Working on #839

Closes #839

codecov-io · 2018-01-22T15:58:42Z

Codecov Report

Merging #908 into master will increase coverage by 0.39%.
The diff coverage is 96.2%.

@@            Coverage Diff             @@
##           master     #908      +/-   ##
==========================================
+ Coverage   95.38%   95.78%   +0.39%     
==========================================
  Files         129      129              
  Lines       15913    17490    +1577     
==========================================
+ Hits        15179    16752    +1573     
- Misses        734      738       +4

Impacted Files	Coverage Δ
privacyidea/lib/machine.py	`95.34% <ø> (ø)`	⬆️
privacyidea/lib/tokens/hotptoken.py	`100% <100%> (ø)`	⬆️
privacyidea/api/validate.py	`100% <100%> (ø)`	⬆️
privacyidea/lib/applications/offline.py	`95.83% <93.61%> (-4.17%)`	⬇️
privacyidea/lib/tokens/yubicotoken.py	`94.78% <0%> (-1.06%)`	⬇️
privacyidea/lib/tokenclass.py	`99.13% <0%> (+0.19%)`	⬆️
privacyidea/lib/tokens/emailtoken.py	`99.58% <0%> (+0.23%)`	⬆️
privacyidea/lib/tokens/yubikeytoken.py	`97.9% <0%> (+0.36%)`	⬆️
privacyidea/lib/tokens/radiustoken.py	`98.75% <0%> (+0.52%)`	⬆️
... and 7 more

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 32f6906...243533d. Read the comment docs.

fredreichbier · 2018-01-22T15:25:08Z

privacyidea/api/validate.py

+    :param pass: the last password (maybe password+OTP) entered by the user
+    :return:
+    """
+    refilltoken = None


I think this line can be deleted, as refilltoken is defined later on.

fredreichbier · 2018-01-22T15:29:30Z

privacyidea/lib/applications/offline.py

+        rounds = int(options.get("rounds", ROUNDS))
+        new_refilltoken = geturandom(REFILLTOKEN_LENGTH, hex=True)
+        toks = get_tokens(serial=serial)
+        if len(toks) == 1:


Maybe we should add a log message in case the serial has no matching token? Because currently, in this case, a new refilltoken and an empty list are returned.

You mean something in case len(toks) != 1?

Sounds sensible to me. But if we are the the refill do we not always have one token with this very serial number?

Hmm yeah, looking at it again, the current behavior seems okay at this point :)

fredreichbier · 2018-01-22T15:37:11Z

privacyidea/api/validate.py

+            tokenobj = get_tokens(serial=serial)[0]
+            if tokenobj.get_tokeninfo("refilltoken") == refilltoken:
+                # refill
+                refilltoken, otps = MachineApplication.get_refill(serial, password, mdef.get("options"))


Reading the code, I think if the given refilltoken is wrong, we simply return it and an empty list of OTPs. Maybe we should instead return an error message?

fredreichbier · 2018-01-22T16:27:31Z

privacyidea/api/validate.py

+    """
+    refilltoken = None
+    otps = {}
+    serial = getParam(request.all_data, "serial", required)


In case the serial has no matching token, the following response is generated:

{ "detail": null, "id": 1, "jsonrpc": "2.0", "result": { "error": { "code": -500, "message": "'NoneType' object has no attribute 'machine_list'" }, "status": false }, ... }

This error is raised:

Traceback (most recent call last): File "/home/fred/privacyidea/privacyidea/venv/lib/python2.7/site-packages/flask/app.py", line 1817, in wsgi_app response = self.full_dispatch_request() File "/home/fred/privacyidea/privacyidea/venv/lib/python2.7/site-packages/flask/app.py", line 1477, in full_dispatch_request rv = self.handle_user_exception(e) File "/home/fred/privacyidea/privacyidea/venv/lib/python2.7/site-packages/flask/app.py", line 1381, in handle_user_exception reraise(exc_type, exc_value, tb) File "/home/fred/privacyidea/privacyidea/venv/lib/python2.7/site-packages/flask/app.py", line 1475, in full_dispatch_request rv = self.dispatch_request() File "/home/fred/privacyidea/privacyidea/venv/lib/python2.7/site-packages/flask/app.py", line 1461, in dispatch_request return self.view_functions[rule.endpoint](**req.view_args) File "/home/fred/privacyidea/privacyidea/privacyidea/lib/event.py", line 60, in event_wrapper f_result = func(*args, **kwds) File "/home/fred/privacyidea/privacyidea/privacyidea/api/validate.py", line 184, in offlinerefill machine_defs = list_token_machines(serial) File "/home/fred/privacyidea/privacyidea/privacyidea/lib/log.py", line 154, in log_wrapper return func(*args, **kwds) File "/home/fred/privacyidea/privacyidea/privacyidea/lib/machine.py", line 332, in list_token_machines for machine in db_token.machine_list: AttributeError: 'NoneType' object has no attribute 'machine_list'

Maybe we should return a descriptive error message.

Is this still a problem or is this outdated due to your rewrite?

I forgot about this :) ce085ac now deals with error cases.

fredreichbier · 2018-01-22T16:46:50Z

privacyidea/lib/applications/offline.py

+                first_old_counter = 0
+            if otpval:
+                # find the value in the old OTP values! This resets the token.count!
+                matching_count = token_obj.check_otp(otpval, first_old_counter, count)


Hmm, I think in case the passed OTP value is not found, matching_count is set to -1, which results in a negative counter_diff later on, which results in a decreasing OTP counter?

fredreichbier · 2018-01-24T13:36:03Z

I tried to make the code a bit clearer in 839/offline-refill...839/offline-refill-wip.
There, we have two methods for the two cases:

get_offline_otps: just retrieve a number of offline OTPs (= hash(PIN + OTP))
get_appropriate_refill: given a PIN+OTP, calculate the size of the refill and call get_offline_otps to get the right amount of new offline OTPs

cornelinux · 2018-01-24T16:17:46Z

This looks good to me.
But I have two comments:

I would rename the get_appropriate_refill to get_refill. I always hope that the return value of a function is in fact appropriate ;-)
In the method get_authentication_item I think the password will never be empty. Because in get its value from the pass parameter from validate/check. (see https://github.com/privacyidea/privacyidea/blob/master/privacyidea/api/lib/postpolicy.py#L480) The API call will always have a pass - or not? Do we have a test case for the empty password?

Working on #839

fredreichbier · 2018-02-06T14:58:08Z

privacyidea/lib/applications/offline.py

+            raise ParameterError("Could not split password")
+        current_token_counter = token_obj.token.count
+        first_old_counter = current_token_counter - count
+        if first_old_counter < 0:


@cornelinux I'm wondering if this is even a legal case? In case of a refill, shouldn't the current token counter value always be higher than the refill amount?

I think you are right. But I am not sure here. Because in some tests I ran into this problem.

if first_old_counter < 0: log.error("This should not happen")

;-)

And I think you said: If we have a negative value, the counter on the server would be decreased. So this might be the safe net for states, that should not occur!

Ah, I guess this could happen if a token gets 100 offline OTPs (i.e. the online token counter is 101), then the administrator increases the offline OTP count to 200 and the client attempts a refill (i.e. 101 - 200 = -99).

But apart from that, we ensure that we never decrease the counter on the server by checking for amount < 0 in get_offline_otps.

possible off-by-one error?

fredreichbier · 2018-02-07T14:00:41Z

We might have had an off-by-one error in get_refill until now:
Assume a user rolls out a new token and attaches it to a machine. Then, the user uses the OTP value with count = 0 to get 100 offline OTPs. Then, the user presses the button once (= OTP with count = 1) and uses the OTP to get a refill. Up until now, the refill response had 0 elements. But shouldn't it have 1 element, as the user has consumed one OTP value already?
I've attempted to fix the code and tests accordingly and pushed 243533d. But we should probably double-check that this is now the intended behavior :)

cornelinux · 2018-02-08T11:54:12Z

@fredreichbier: I am merging this now!

Then we adapt the pam module and check it in real live.

cornelinux added 3 commits January 22, 2018 09:38

Create a refill-token and save it encrypted

23ea3c6

...with the tokeninfo Working on #839

Allow offline refill

4563de6

Closes #839

Fix increasing counters in tests

7621cb3

Closes #839

cornelinux requested a review from fredreichbier January 22, 2018 13:56

fredreichbier suggested changes Jan 22, 2018

View reviewed changes

fredreichbier reviewed Jan 22, 2018

View reviewed changes

cornelinux and others added 2 commits January 23, 2018 16:22

wrong OTP value raises exception

4cd1f94

WIP: Split refill methods into two

2c6bfad

Fixes with new function signatures

3e82422

Friedrich Weber added 3 commits February 6, 2018 10:42

Cosmetic API changes

f122e40

Cosmetic API changes (2)

736b0e6

Test error cases

7f40cff

Working on #839

fredreichbier reviewed Feb 6, 2018

View reviewed changes

Handle error cases better

ce085ac

fredreichbier previously approved these changes Feb 7, 2018

View reviewed changes

Friedrich Weber added 2 commits February 7, 2018 14:25

Cover error cases in unit tests

2319d34

Possible fix of an off-by-one bug

243533d

cornelinux merged commit 157b420 into master Feb 8, 2018

fredreichbier deleted the 839/offline-refill branch February 9, 2018 08:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

839/offline refill #908

839/offline refill #908

cornelinux commented Jan 22, 2018 •

edited

codecov-io commented Jan 22, 2018 •

edited

fredreichbier Jan 22, 2018

cornelinux Jan 22, 2018

fredreichbier Jan 22, 2018

cornelinux Feb 6, 2018 •

edited

fredreichbier Feb 7, 2018

fredreichbier Jan 22, 2018

fredreichbier Jan 22, 2018

cornelinux Feb 6, 2018

fredreichbier Feb 7, 2018

fredreichbier Jan 22, 2018

fredreichbier commented Jan 24, 2018 •

edited

cornelinux commented Jan 24, 2018

fredreichbier Feb 6, 2018

cornelinux Feb 6, 2018

fredreichbier Feb 7, 2018

fredreichbier commented Feb 7, 2018

cornelinux commented Feb 8, 2018

839/offline refill #908

839/offline refill #908

Conversation

cornelinux commented Jan 22, 2018 • edited

codecov-io commented Jan 22, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cornelinux Feb 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredreichbier commented Jan 24, 2018 • edited

cornelinux commented Jan 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredreichbier commented Feb 7, 2018

cornelinux commented Feb 8, 2018

cornelinux commented Jan 22, 2018 •

edited

codecov-io commented Jan 22, 2018 •

edited

cornelinux Feb 6, 2018 •

edited

fredreichbier commented Jan 24, 2018 •

edited