Skip to content

[uss_qualifier] Add auth adapter expectations to authorization verification - #1654

Merged
BenjaminPelletier merged 6 commits into
interuss:mainfrom
BenjaminPelletier:auth-adapter-expectations
Aug 25, 2026
Merged

[uss_qualifier] Add auth adapter expectations to authorization verification#1654
BenjaminPelletier merged 6 commits into
interuss:mainfrom
BenjaminPelletier:auth-adapter-expectations

Conversation

@BenjaminPelletier

Copy link
Copy Markdown
Member

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.

@BenjaminPelletier
BenjaminPelletier marked this pull request as ready for review August 20, 2026 19:23

def _has_attr(obj: Any, attr_name: str) -> bool:
if "." in attr_name:
levels = attr_name.split(".")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you can base, rest = attr_name.split(".", 2) and avoid the array addressing and join.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it needs to be .split(".", 1), but done.

@mickmis mickmis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

return hasattr(obj, attr_name)


def _get_attr_value(obj: Any, attr_name: str) -> Any:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This intends to do the same as:

def _dotted_get(obj: Any, key: str) -> T | None:
val: Any = obj
for k in key.split("."):
if val is None:
return val
if isinstance(val, dict) and k in val:
val = val[k]
else:
val = getattr(val, k, None)
return val

Consider deduplicating

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider comparison with delta to avoid float precision issues

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@BenjaminPelletier
BenjaminPelletier merged commit 7bde93e into interuss:main Aug 25, 2026
24 checks passed
@BenjaminPelletier
BenjaminPelletier deleted the auth-adapter-expectations branch August 25, 2026 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants