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

Handle non-existent resources more strictly #1310

Merged
merged 7 commits into from Nov 19, 2018

Conversation

Projects
None yet
2 participants
@fredreichbier
Member

fredreichbier commented Nov 13, 2018

On the library level, this introduces a new ResourceNotFoundError
that is thrown when a non-existent resource is referenced.

In particular, this improves the matching of tokens in lib/token.py.

The API level transforms these errors into HTTP 404 responses.
Thus, this commit changes API behavior.

Fixes #817
Fixes #1285

Merging #1310 into master will increase coverage by 0.01%.
The diff coverage is 98.84%.
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1310      +/-   ##
==========================================
+ Coverage    95.8%   95.81%   +0.01%
==========================================
Files         142      142
Lines       17238    17208      -30
==========================================
- Hits        16515    16488      -27
+ Misses        723      720       -3
Impacted Files Coverage Δ
privacyidea/lib/periodictask.py 96.77% <100%> (-0.07%) ⬇️
privacyidea/lib/radiusserver.py 97.26% <100%> (-0.15%) ⬇️
privacyidea/lib/realm.py 100% <100%> (ø) ⬆️
privacyidea/lib/machineresolver.py 100% <100%> (ø) ⬆️
privacyidea/lib/smtpserver.py 98.83% <100%> (-0.06%) ⬇️
privacyidea/api/lib/postpolicy.py 96.67% <100%> (ø) ⬆️
privacyidea/api/token.py 98.6% <100%> (ø) ⬆️
privacyidea/lib/utils.py 96.49% <100%> (+0.04%) ⬆️
privacyidea/lib/event.py 99.07% <100%> (-0.03%) ⬇️
privacyidea/lib/error.py 91.26% <100%> (+0.35%) ⬆️
... and 10 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 cadd085...708fb4c. Read the comment docs.

  • Of course the tests have conflicts now because of #1296 -- I'll resolve these after we the review of this PR. If I now merge master into this PR, it will get messy :-)
  •  File: privacyidea/lib/policy.py:L951-965
  • typo :-) "deletec"
  • Thx :) fixed!
  •  File: privacyidea/api/token.py:L441-456
  • By changing "True" to "res > 0" this would mean, if I assign "all tokens" of a user, who has not token, I would get a success=False.
    Is that, what we want?
  • I changed this to True again in dc564c7 :)
  •  File: privacyidea/api/token.py:L799-808
  • This does also not seem quite consistent to me.
    In this case we always add "success": True to the audit. Setting an empty list would remove the realms?
  • Hmm, I'm not sure here: set_realms only gets a serial, not a user, so it will raise a ResourceNotFound if the token with the serial does not exist. So if the line after set_realms is executed, I would say the request is successful?
  • I think you can forget my comment. It was in conjunction with the other success-audit-thingies.
    I also think that we can return True, if realms were successfully set, no matter if they were overwritten, added or deleted!
    Works for me.
  •  File: privacyidea/lib/error.py:L48-55
  • Why did yo choose 601? Why not a 404?
    Or did you want to leave our 4xx errors to the authentication process? (I do not know the number-spaces myself)
  • Works for me.
  •  File: privacyidea/lib/privacyideaserver.py:L159-168
  • I am currently not sure - does .delete() return the ID of the deleted object?
  • I forgot about this too, but .delete() is actually our own MethodsMixin method which returns the ID of the deleted object :-)
  • Oh. This is something I added right at the beginning - so I would have had to know about this! :-/
  •  File: privacyidea/lib/realm.py:L154-161
  • Why not do this as a one-liner?
  • fixed this in 708fb4c
  •  File: privacyidea/lib/token.py:L1127-1140
  • The logic now is very API centric.
    The library call "set_defaults" will now raise an exception is the serial does not exist. This is fine for an API call but may result in unexpected behaviour, if set_detaults is called from without other code.
    (justsaying)
  • Works for me.
  •  File: privacyidea/api/token.py:L426-434
  • {"success": True}
  • Actually, assign_token either raises an error or returns True, so if this line is executed at all, res will always be True.
    But maybe we should make this more explicit?
  • I think it is more clear to revert it to "True" again.Thanks a lot.
  • No problem, did this in dc564c7
  •  File: tests/test_lib_events.py:L571-577
  • What changed here?
    If the token does not exist, we can catch an exception.
    OK, due to the failing script, the token was not created? - right it is raise-error=True.
    Maybe it is a good idea to realy check in this case, that the originial request also failed.
    self.assertRaises(Exception, remove_token, "SPASS01")
  • I think this is a new test from #1307. My understanding is that the token "SPASS01" does not actually exist from the perspective of privacyIDEA (because we only test the ScriptEventHandler), so no need to try to remove it :-)
    I think I've already done the same for test_01_runscript.
  • You are right. The token really does not exist!
    So this is fine!

   

Handle non-existent resources more strictly
On the library level, this introduces a new ResourceNotFoundError
that is thrown when a non-existent resource is referenced.

In particular, this improves the matching of tokens in lib/token.py.

The API level transforms these errors into HTTP 404 responses.
Thus, this commit changes API behavior.

Fixes #817
Fixes #1285

@fredreichbier fredreichbier requested a review from privacyidea/core Nov 13, 2018

@codecov

This comment has been minimized.

codecov bot commented Nov 13, 2018

Codecov Report

Merging #1310 into master will increase coverage by 0.01%.
The diff coverage is 98.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1310      +/-   ##
==========================================
+ Coverage   95.78%   95.79%   +0.01%     
==========================================
  Files         142      142              
  Lines       17313    17283      -30     
==========================================
- Hits        16583    16557      -26     
+ Misses        730      726       -4
Impacted Files Coverage Δ
privacyidea/lib/periodictask.py 96.77% <100%> (-0.07%) ⬇️
privacyidea/lib/radiusserver.py 97.26% <100%> (-0.15%) ⬇️
privacyidea/lib/realm.py 100% <100%> (ø) ⬆️
privacyidea/lib/machineresolver.py 100% <100%> (ø) ⬆️
privacyidea/lib/smtpserver.py 98.83% <100%> (-0.06%) ⬇️
privacyidea/api/lib/postpolicy.py 96.73% <100%> (ø) ⬆️
privacyidea/api/token.py 98.6% <100%> (ø) ⬆️
privacyidea/lib/utils.py 96.78% <100%> (+0.03%) ⬆️
privacyidea/lib/event.py 99.07% <100%> (-0.03%) ⬇️
privacyidea/lib/error.py 92.23% <100%> (+0.31%) ⬆️
... and 10 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 da8b43b...2f0d61f. Read the comment docs.

:param name: the name of the policy to be deleted
:return: the count of the deleted policies.
:return: the ID of the deletec policy

This comment has been minimized.

@cornelinux

cornelinux Nov 13, 2018

Member

typo :-) "deletec"

This comment has been minimized.

@fredreichbier

fredreichbier Nov 15, 2018

Member

Thx :) fixed!

@fredreichbier

This comment has been minimized.

Member

fredreichbier commented Nov 15, 2018

Of course the tests have conflicts now because of #1296 -- I'll resolve these after we the review of this PR. If I now merge master into this PR, it will get messy :-)

:param name: the name of the policy to be deleted
:return: the count of the deleted policies.
:return: the ID of the deletec policy

This comment has been minimized.

@cornelinux
res = unassign_token(serial, user=user)
g.audit_object.log({"success": True})
g.audit_object.log({"success": res > 0})

This comment has been minimized.

@cornelinux

cornelinux Nov 19, 2018

Member

By changing "True" to "res > 0" this would mean, if I assign "all tokens" of a user, who has not token, I would get a success=False.
Is that, what we want?

This comment has been minimized.

@fredreichbier

fredreichbier Nov 19, 2018

Member

I changed this to True again in dc564c7 :)

g.audit_object.log({"success": True})
return send_result(res == 1)
return send_result(True)

This comment has been minimized.

@cornelinux

cornelinux Nov 19, 2018

Member

This does also not seem quite consistent to me.
In this case we always add "success": True to the audit. Setting an empty list would remove the realms?

This comment has been minimized.

@fredreichbier

fredreichbier Nov 19, 2018

Member

Hmm, I'm not sure here: set_realms only gets a serial, not a user, so it will raise a ResourceNotFound if the token with the serial does not exist. So if the line after set_realms is executed, I would say the request is successful?

This comment has been minimized.

@cornelinux

cornelinux Nov 19, 2018

Member

I think you can forget my comment. It was in conjunction with the other success-audit-thingies.
I also think that we can return True, if realms were successfully set, no matter if they were overwritten, added or deleted!
Works for me.

@@ -48,6 +48,7 @@ class ERROR:
AUTHENTICATE_TOKEN_EXPIRED = 4305
AUTHENTICATE_MISSING_RIGHT = 4306
CA = 503
RESOURCE_NOT_FOUND = 601

This comment has been minimized.

@cornelinux

cornelinux Nov 19, 2018

Member

Why did yo choose 601? Why not a 404?
Or did you want to leave our 4xx errors to the authentication process? (I do not know the number-spaces myself)

This comment has been minimized.

@cornelinux

cornelinux Nov 19, 2018

Member

Works for me.

ret = pi.id
return ret
return fetch_one_resource(PrivacyIDEAServerDB, identifier=identifier).delete()

This comment has been minimized.

@cornelinux

cornelinux Nov 19, 2018

Member

I am currently not sure - does .delete() return the ID of the deleted object?

This comment has been minimized.

@fredreichbier

fredreichbier Nov 19, 2018

Member

I forgot about this too, but .delete() is actually our own MethodsMixin method which returns the ID of the deleted object :-)

This comment has been minimized.

@cornelinux

cornelinux Nov 19, 2018

Member

Oh. This is something I added right at the beginning - so I would have had to know about this! :-/

@@ -154,7 +154,7 @@ def delete_realm(realmname):
defRealm = get_default_realm()
hadDefRealmBefore = (defRealm != "")
realm = Realm.query.filter_by(name=realmname).first()
realm = fetch_one_resource(Realm, name=realmname)
ret = realm.delete()

This comment has been minimized.

@cornelinux

cornelinux Nov 19, 2018

Member

Why not do this as a one-liner?

This comment has been minimized.

@fredreichbier

fredreichbier Nov 19, 2018

Member

fixed this in 708fb4c

db_token.maxfail = int(get_from_config("DefaultMaxFailCount", 15))
db_token.sync_window = int(get_from_config("DefaultSyncWindow", 1000))
db_token.tokentype = u"hotp"
db_token.save()

This comment has been minimized.

@cornelinux

cornelinux Nov 19, 2018

Member

The logic now is very API centric.
The library call "set_defaults" will now raise an exception is the serial does not exist. This is fine for an API call but may result in unexpected behaviour, if set_detaults is called from without other code.
(justsaying)

This comment has been minimized.

@cornelinux

cornelinux Nov 19, 2018

Member

Works for me.

@cornelinux

I like the get_on_token approach a lot.
Makes thinks clearer and probably faster.

I am sorry, my comments might be a bit awkward or naggy, maybe due to I am not reall fit.
However, there a lot of behavioral changes, so I would like to discuss them.
This is defenitively a change for version 3.0! :-)

err_message=err_message)
g.audit_object.log({"success": True})
res = assign_token(serial, user, pin=pin, encrypt_pin=encrypt_pin, err_message=err_message)
g.audit_object.log({"success": res})

This comment has been minimized.

@cornelinux

cornelinux Nov 19, 2018

Member

{"success": True}

This comment has been minimized.

@fredreichbier

fredreichbier Nov 19, 2018

Member

Actually, assign_token either raises an error or returns True, so if this line is executed at all, res will always be True.

But maybe we should make this more explicit?

This comment has been minimized.

@cornelinux

cornelinux Nov 19, 2018

Member

I think it is more clear to revert it to "True" again.Thanks a lot.

This comment has been minimized.

@fredreichbier

fredreichbier Nov 19, 2018

Member

No problem, did this in dc564c7

@@ -48,6 +48,7 @@ class ERROR:
AUTHENTICATE_TOKEN_EXPIRED = 4305
AUTHENTICATE_MISSING_RIGHT = 4306
CA = 503
RESOURCE_NOT_FOUND = 601

This comment has been minimized.

@cornelinux
db_token.maxfail = int(get_from_config("DefaultMaxFailCount", 15))
db_token.sync_window = int(get_from_config("DefaultSyncWindow", 1000))
db_token.tokentype = u"hotp"
db_token.save()

This comment has been minimized.

@cornelinux
@@ -490,17 +522,11 @@ def get_token_type(serial):
:return: tokentype
:rtype: string
"""
if serial and "*" in serial:
try:
return get_one_token(serial=serial).type

This comment has been minimized.

@cornelinux
ret = pi.id
return ret
return fetch_one_resource(PrivacyIDEAServerDB, identifier=identifier).delete()

This comment has been minimized.

@cornelinux
@@ -154,7 +154,7 @@ def delete_realm(realmname):
defRealm = get_default_realm()
hadDefRealmBefore = (defRealm != "")
realm = Realm.query.filter_by(name=realmname).first()
realm = fetch_one_resource(Realm, name=realmname)
ret = realm.delete()

This comment has been minimized.

@cornelinux
err_message=err_message)
g.audit_object.log({"success": True})
res = assign_token(serial, user, pin=pin, encrypt_pin=encrypt_pin, err_message=err_message)
g.audit_object.log({"success": res})

This comment has been minimized.

@cornelinux
g.audit_object.log({"success": True})
return send_result(res == 1)
return send_result(True)

This comment has been minimized.

@cornelinux
res = unassign_token(serial, user=user)
g.audit_object.log({"success": True})
g.audit_object.log({"success": res > 0})

This comment has been minimized.

@cornelinux
@cornelinux

This comment has been minimized.

Member

cornelinux commented Nov 19, 2018

:shipit:

@@ -571,7 +571,6 @@ def test_02_failscript(self):
d = "{0!s}/tests/testdata/scripts/".format(d)
t_handler = ScriptEventHandler(script_directory=d)
self.assertRaises(Exception, t_handler.do, script_name, options=options)
remove_token("SPASS01")

This comment has been minimized.

@cornelinux

cornelinux Nov 19, 2018

Member

What changed here?
If the token does not exist, we can catch an exception.

OK, due to the failing script, the token was not created? - right it is raise-error=True.
Maybe it is a good idea to realy check in this case, that the originial request also failed.

self.assertRaises(Exception, remove_token, "SPASS01")

This comment has been minimized.

@fredreichbier

fredreichbier Nov 19, 2018

Member

I think this is a new test from #1307. My understanding is that the token "SPASS01" does not actually exist from the perspective of privacyIDEA (because we only test the ScriptEventHandler), so no need to try to remove it :-)
I think I've already done the same for test_01_runscript.

This comment has been minimized.

@cornelinux

cornelinux Nov 19, 2018

Member

You are right. The token really does not exist!
So this is fine!

@@ -33,7 +33,7 @@
from privacyidea.lib.tokenclass import DATE_FORMAT
from privacyidea.lib.user import create_user, User
from privacyidea.lib.policy import ACTION
from privacyidea.lib.error import ParameterError

This comment has been minimized.

@cornelinux

@cornelinux cornelinux merged commit dccc9f4 into master Nov 19, 2018

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 98.84% of diff hit (target 95.78%)
Details
codecov/project 95.79% (+0.01%) compared to da8b43b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@cornelinux cornelinux deleted the 817/nonexistent-resources-api-changes branch Nov 19, 2018

@fredreichbier fredreichbier referenced this pull request Dec 11, 2018

Merged

Restore safeguards against wildcard serials #1336

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment