[uss_qualifier] Add auth adapter expectations to authorization verification - #1654
Conversation
|
|
||
| def _has_attr(obj: Any, attr_name: str) -> bool: | ||
| if "." in attr_name: | ||
| levels = attr_name.split(".") |
There was a problem hiding this comment.
nit: you can base, rest = attr_name.split(".", 2) and avoid the array addressing and join.
There was a problem hiding this comment.
Looks like it needs to be .split(".", 1), but done.
| return hasattr(obj, attr_name) | ||
|
|
||
|
|
||
| def _get_attr_value(obj: Any, attr_name: str) -> Any: |
There was a problem hiding this comment.
This intends to do the same as:
Consider deduplicating
There was a problem hiding this comment.
This function does have similar behavior to the content added to inspection.py in this PR, but it's not identical (e.g., _dotted_get returns None when keys can't be found whereas _get_attr_value errors). I'm not sure deduplicating this particular function would be worth it after the changes necessary to expose _get_attr_value publicly and change its functionality to be polymorphic (raise an AttributeError or return None), especially since it's only a few lines long in both places. If we did want to harmonize the two functions, I think that would probably be in a follow-up PR since both functions are currently private, so the harmonization would be a non-trivial addition to this PR. If there was already something available in the codebase to perform the dotted-get task added in this PR, I agree we wouldn't want to merge this PR until we avoided introducing unnecessary duplication. However, I don't think that's the case here with this _dotted_get, especially since it is not currently in a position to be reused (in common_dictionary_evaluator.py rather than somewhere in monitorlib or similar).
| failures.append( | ||
| f"Attribute '{attr_name}' expected to be numeric, but observed type '{type(actual_val).__name__}'." | ||
| ) | ||
| elif actual_val != pair.equals_number_value: |
There was a problem hiding this comment.
Consider comparison with delta to avoid float precision issues
There was a problem hiding this comment.
I agree this is a valid concern as exact comparison of float numbers is often not best accomplished with exact equality. I considered switching to math.isclose, but then thought that a field labeled "equals_number_value" instead evaluating whether the numbers were close could be surprising behavior for the user. I think a better approach would be to add an "is_close_to_number_value" field for the float comparison so the user's expectations are more precisely aligned. I've added a clarification in the documentation recommending against equals_number_value for float comparisons.
This PR implements the second part of #1629 by adding the concept of AuthAdapterExpectations that can check what type of auth adapter was actually provided to the scenario and key attributes of the auth adapter.
I used a custom implementation of exact-value-of-attribute for AuthAdapter checks rather than an ASTExpression to reduce the potential for AST script attacks using the sensitive AuthAdapter object.