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
Changes from 1 commit
9815056
4ea0cc8
dc564c7
708fb4c
770282b
ab56c32
2f0d61f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -426,9 +426,8 @@ def assign_api(): | |
err_message = "Token already assigned to another user." | ||
else: | ||
err_message = None | ||
res = assign_token(serial, user, pin=pin, encrypt_pin=encrypt_pin, | ||
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}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
return send_result(res) | ||
|
||
|
||
|
@@ -442,13 +441,15 @@ def unassign_api(): | |
You can either provide "serial" as an argument to unassign this very | ||
token or you can provide user and realm, to unassign all tokens of a user. | ||
|
||
:return: In case of success it returns "value": True. | ||
:rtype: json object | ||
:return: In case of success it returns the number of unassigned tokens in "value". | ||
:rtype: JSON object | ||
""" | ||
user = get_user_from_param(request.all_data, optional) | ||
serial = getParam(request.all_data, "serial", optional) | ||
g.audit_object.log({"serial": serial}) | ||
|
||
res = unassign_token(serial, user=user) | ||
g.audit_object.log({"success": True}) | ||
g.audit_object.log({"success": res > 0}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
return send_result(res) | ||
|
||
|
||
|
@@ -546,13 +547,11 @@ def disable_api(serial=None): | |
@prepolicy(check_base_action, request, action=ACTION.DELETE) | ||
@event("token_delete", request, g) | ||
@log_with(log) | ||
def delete_api(serial=None): | ||
def delete_api(serial): | ||
""" | ||
Delete a token by its serial number or delete all tokens of a user. | ||
Delete a token by its serial number. | ||
|
||
:jsonparam serial: The serial number of a single token. | ||
:jsonparam user: The username of the user, whose tokens should be deleted. | ||
:jsonparam realm: The realm of the user. | ||
|
||
:return: In case of success it return the number of deleted tokens in | ||
"value" | ||
|
@@ -800,9 +799,9 @@ def tokenrealm_api(serial=None): | |
realm_list = [r.strip() for r in realms.split(",")] | ||
g.audit_object.log({"serial": serial}) | ||
|
||
res = set_realms(serial, realms=realm_list) | ||
set_realms(serial, realms=realm_list) | ||
g.audit_object.log({"success": True}) | ||
return send_result(res == 1) | ||
return send_result(True) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This does also not seem quite consistent to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I'm not sure here: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can forget my comment. It was in conjunction with the other success-audit-thingies. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
|
||
@token_blueprint.route('/load/<filename>', methods=['POST']) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,6 +48,7 @@ class ERROR: | |
AUTHENTICATE_TOKEN_EXPIRED = 4305 | ||
AUTHENTICATE_MISSING_RIGHT = 4306 | ||
CA = 503 | ||
RESOURCE_NOT_FOUND = 601 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did yo choose 601? Why not a 404? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Works for me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
HSM = 707 | ||
SELFSERVICE = 807 | ||
SERVER = 903 | ||
|
@@ -112,7 +113,12 @@ class AuthError(privacyIDEAError): | |
def __init__(self, description, id=ERROR.AUTHENTICATE, details=None): | ||
self.details = details | ||
privacyIDEAError.__init__(self, description=description, id=id) | ||
|
||
|
||
|
||
class ResourceNotFoundError(privacyIDEAError): | ||
def __init__(self, description, id=ERROR.RESOURCE_NOT_FOUND): | ||
privacyIDEAError.__init__(self, description=description, id=id) | ||
|
||
|
||
class PolicyError(privacyIDEAError): | ||
def __init__(self, description, id=ERROR.POLICY): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -164,12 +164,12 @@ | |
from flask import current_app | ||
from privacyidea.lib.config import (get_token_classes, get_token_types, | ||
Singleton) | ||
from privacyidea.lib.error import ParameterError, PolicyError | ||
from privacyidea.lib.error import ParameterError, PolicyError, ResourceNotFoundError | ||
from privacyidea.lib.realm import get_realms | ||
from privacyidea.lib.resolver import get_resolver_list | ||
from privacyidea.lib.smtpserver import get_smtpservers | ||
from privacyidea.lib.radiusserver import get_radiusservers | ||
from privacyidea.lib.utils import check_time_in_range, reload_db | ||
from privacyidea.lib.utils import check_time_in_range, reload_db, fetch_one_resource | ||
from privacyidea.lib.user import User | ||
from privacyidea.lib import _ | ||
import datetime | ||
|
@@ -941,7 +941,7 @@ def enable_policy(name, enable=True): | |
:return: ID of the policy | ||
""" | ||
if not Policy.query.filter(Policy.name == name).first(): | ||
raise ParameterError("The policy with name '{0!s}' does not exist".format(name)) | ||
raise ResourceNotFoundError(u"The policy with name '{0!s}' does not exist".format(name)) | ||
|
||
# Update the policy | ||
p = set_policy(name=name, active=enable) | ||
|
@@ -951,17 +951,14 @@ def enable_policy(name, enable=True): | |
@log_with(log) | ||
def delete_policy(name): | ||
""" | ||
Function to delete one named policy | ||
Function to delete one named policy. | ||
Raise ResourceNotFoundError if there is no such policy. | ||
|
||
:param name: the name of the policy to be deleted | ||
:return: the count of the deleted policies. | ||
:return: the ID of the deletec policy | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo :-) "deletec" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thx :) fixed! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
:rtype: int | ||
""" | ||
res = False | ||
p = Policy.query.filter_by(name=name).first() | ||
if p: | ||
res = p.delete() | ||
return res | ||
return fetch_one_resource(Policy, name=name).delete() | ||
|
||
|
||
@log_with(log) | ||
|
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.
{"success": True}
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.
Actually,
assign_token
either raises an error or returnsTrue
, so if this line is executed at all,res
will always be True.But maybe we should make this more explicit?
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.
I think it is more clear to revert it to "True" again.Thanks a lot.
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.
No problem, did this in dc564c7