-
Notifications
You must be signed in to change notification settings - Fork 38
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
904 read permissions of entities within flex model or flex context #1071
base: main
Are you sure you want to change the base?
Changes from 17 commits
6ecb17f
2546546
c3a5920
482dd0a
cc7248b
0b41e77
e616d1c
019d772
aa27d79
6ea9202
f72ab14
edfa646
16684e6
7a12c49
c7938fa
8733408
38d8241
075db1a
97aa1d7
b2ec43a
d7862fc
07b864c
b07d76f
ff73a4c
110d027
54c87d4
0873183
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 |
---|---|---|
|
@@ -84,7 +84,7 @@ def test_trigger_schedule_with_invalid_flexmodel( | |
) | ||
print("Server responded with:\n%s" % trigger_schedule_response.json) | ||
check_deprecation(trigger_schedule_response, deprecation=None, sunset=None) | ||
assert trigger_schedule_response.status_code == 422 | ||
assert trigger_schedule_response.status_code == 422 # Unprocessable entity | ||
assert field in trigger_schedule_response.json["message"]["json"] | ||
if isinstance(trigger_schedule_response.json["message"]["json"], str): | ||
# ValueError | ||
|
@@ -422,3 +422,76 @@ def test_get_schedule_fallback_not_redirect( | |
assert schedule["scheduler_info"]["scheduler"] == "StorageFallbackScheduler" | ||
|
||
app.config["FLEXMEASURES_FALLBACK_REDIRECT"] = False | ||
|
||
|
||
@pytest.mark.parametrize( | ||
"message, flex_config, field, err_msg", | ||
[ | ||
( | ||
message_for_trigger_schedule(), | ||
"flex-context", | ||
"site-consumption-capacity", | ||
"no read authorization", | ||
), | ||
( | ||
message_for_trigger_schedule(), | ||
"flex-model", | ||
"site-consumption-capacity", | ||
"no read authorization", | ||
), | ||
], | ||
) | ||
@pytest.mark.parametrize( | ||
"requesting_user", ["[email protected]"], indirect=True | ||
) | ||
def test_trigger_schedule_with_unauthorized_sensor( | ||
app, | ||
add_battery_assets, | ||
setup_capacity_sensor_on_asset_in_supplier_account, | ||
keep_scheduling_queue_empty, | ||
message, | ||
flex_config, | ||
field, | ||
err_msg, | ||
requesting_user, | ||
): | ||
"""Test triggering a schedule using a flex config that refers to a capacity sensor from a different account. | ||
|
||
The user is not authorized to read sensors from the other account, | ||
so we expect a 422 (Unprocessable entity) response referring to the relevant flex-config field. | ||
nhoening marked this conversation as resolved.
Show resolved
Hide resolved
|
||
""" | ||
sensor = add_battery_assets["Test battery"].sensors[0] | ||
with app.test_client() as client: | ||
if flex_config not in message: | ||
message[flex_config] = {} | ||
sensor_id = setup_capacity_sensor_on_asset_in_supplier_account.id | ||
message[flex_config][field] = {"sensor": sensor_id} | ||
|
||
trigger_schedule_response = client.post( | ||
url_for("SensorAPI:trigger_schedule", id=sensor.id), | ||
json=message, | ||
) | ||
print("Server responded with:\n%s" % trigger_schedule_response.json) | ||
assert trigger_schedule_response.status_code == 422 # Unprocessable entity | ||
assert ( | ||
f"{flex_config}.{field}.sensor" in trigger_schedule_response.json["message"] | ||
) | ||
if isinstance( | ||
trigger_schedule_response.json["message"][f"{flex_config}.{field}.sensor"], | ||
str, | ||
): | ||
# ValueError | ||
assert ( | ||
err_msg | ||
in trigger_schedule_response.json["message"][ | ||
f"{flex_config}.{field}.sensor" | ||
] | ||
) | ||
else: | ||
# ValidationError (marshmallow) | ||
assert ( | ||
err_msg | ||
in trigger_schedule_response.json["message"][ | ||
f"{flex_config}.{field}.sensor" | ||
][field][0] | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -111,6 +111,7 @@ def permission_required_for_context( | |
ctx_arg_name: str | None = None, | ||
ctx_loader: Callable | None = None, | ||
pass_ctx_to_loader: bool = False, | ||
error_handler: Callable | None = None, | ||
): | ||
""" | ||
This decorator can be used to make sure that the current user has the necessary permission to access the context. | ||
|
@@ -119,6 +120,7 @@ def permission_required_for_context( | |
|
||
A 403 response is raised if there is no principal for the required permission. | ||
A 401 response is raised if the user is not authenticated at all. | ||
A custom response can be generated by passing an error_handler, which should be a function that accepts the context, permission and a context origin. | ||
|
||
We will now explain how to load a context, and give an example: | ||
|
||
|
@@ -145,7 +147,8 @@ def view(resource_id: int, the_resource: Resource): | |
|
||
The ctx_loader: | ||
|
||
The ctx_loader can be a function without arguments or it takes the context loaded from the arguments as input (using pass_ctx_to_loader=True). | ||
The ctx_loader can be a function without arguments, or it takes the context loaded from the arguments as input (using pass_ctx_to_loader=True). | ||
It should return the context or a list of contexts. | ||
Flix6x marked this conversation as resolved.
Show resolved
Hide resolved
|
||
A special case is useful when the arguments contain the context ID (not the instance). | ||
Then, the loader can be a subclass of AuthModelMixin, and this decorator will look up the instance. | ||
|
||
|
@@ -168,38 +171,66 @@ def post(self, resource_data: dict): | |
def wrapper(fn): | ||
@wraps(fn) | ||
def decorated_view(*args, **kwargs): | ||
# load & check context | ||
context: AuthModelMixin = None | ||
|
||
# first set context_from_args, if possible | ||
context_from_args: AuthModelMixin = None | ||
if ctx_arg_pos is not None and ctx_arg_name is not None: | ||
context_from_args = args[ctx_arg_pos][ctx_arg_name] | ||
elif ctx_arg_pos is not None: | ||
context_from_args = args[ctx_arg_pos] | ||
elif ctx_arg_name is not None: | ||
context_from_args = kwargs[ctx_arg_name] | ||
elif len(args) > 0: | ||
context_from_args = args[0] | ||
|
||
# if a loader is given, use that, otherwise fall back to context_from_args | ||
if ctx_loader is not None: | ||
if pass_ctx_to_loader: | ||
if inspect.isclass(ctx_loader) and issubclass( | ||
ctx_loader, AuthModelMixin | ||
): | ||
context = db.session.get(ctx_loader, context_from_args) | ||
else: | ||
context = ctx_loader(context_from_args) | ||
else: | ||
context = ctx_loader() | ||
else: | ||
context = context_from_args | ||
context = load_context( | ||
ctx_arg_pos, ctx_arg_name, ctx_loader, pass_ctx_to_loader, args, kwargs | ||
) | ||
|
||
check_access(context, permission) | ||
# skip check in case (optional) argument was not passed | ||
if context is None: | ||
Flix6x marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return fn(*args, **kwargs) | ||
|
||
# Check access for possibly multiple contexts | ||
if not isinstance(context, list): | ||
context = [context] | ||
for ctx in context: | ||
if isinstance(ctx, tuple): | ||
c = ctx[0] # c[0] is the context, c[1] is its origin | ||
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. Let's mention for ease of reading that the origin is just a string. |
||
# the context loader may narrow down the origin of the context (e.g. a nested field rather than a function argument) | ||
origin = ctx[1] | ||
else: | ||
c = ctx | ||
origin = ctx_arg_name | ||
try: | ||
check_access(c, permission) | ||
except Exception as e: # noqa: B902 | ||
if error_handler: | ||
return error_handler(c, permission, origin) | ||
raise e | ||
|
||
return fn(*args, **kwargs) | ||
|
||
return decorated_view | ||
|
||
return wrapper | ||
|
||
|
||
def load_context( | ||
ctx_arg_pos, ctx_arg_name, ctx_loader, pass_ctx_to_loader, args, kwargs | ||
) -> AuthModelMixin | None: | ||
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. Is this return type still valid? It can be a list or tuples with origins, right? |
||
|
||
# first set context_from_args, if possible | ||
context_from_args: AuthModelMixin | None = None | ||
if ctx_arg_pos is not None and ctx_arg_name is not None: | ||
context_from_args = args[ctx_arg_pos][ctx_arg_name] | ||
elif ctx_arg_pos is not None: | ||
context_from_args = args[ctx_arg_pos] | ||
elif ctx_arg_name is not None: | ||
context_from_args = kwargs.get(ctx_arg_name) | ||
# skip check in case (optional) argument was not passed | ||
if context_from_args is None: | ||
return None | ||
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 (still) believe this should be a
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 this is the major problem blocking my progress on this PR. How do you propose I deal with checking authorization on an optional argument (in this case coming from an optional API field)? Should I add a boolean argument 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. Can we not check that I.e. if Simply put, if Why did you change the logic so that it could be granted? The field was optional before. Maybe it is because the flex context can contain no sensors to check? That is a case I can imagine, and maybe we could add a parameter to But the interpretation has to fit. |
||
elif len(args) > 0: | ||
context_from_args = args[0] | ||
|
||
# if a loader is given, use that, otherwise fall back to context_from_args | ||
if ctx_loader is not None: | ||
if pass_ctx_to_loader: | ||
if inspect.isclass(ctx_loader) and issubclass(ctx_loader, AuthModelMixin): | ||
context = db.session.get(ctx_loader, context_from_args) | ||
else: | ||
context = ctx_loader(context_from_args) | ||
else: | ||
context = ctx_loader() | ||
else: | ||
context = context_from_args | ||
return context | ||
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. Another thought: We provide several means of looking up context(s). Should we maybe check here that all context(s) are actually a subclass of I just checked, and |
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. @nhoening I need advice on where to move these new util functions, for example, to:
I saw other ctx_loaders were pointing to various things, such as:
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. These have nothing to do with auth directly, but are lookup functions that rely on our planning data structure. So I guess this location isn't bad, really. I don't see why 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'm not sure what to do with this. Am I using |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
from __future__ import annotations | ||
|
||
from functools import partial | ||
from packaging import version | ||
from datetime import date, datetime, timedelta | ||
|
||
|
@@ -441,3 +442,82 @@ def nanmin_of_series_and_value(s: pd.Series, value: float | pd.Series) -> pd.Ser | |
# [right]: datetime64[ns, UTC] | ||
value = value.tz_convert("UTC") | ||
return s.fillna(value).clip(upper=value) | ||
|
||
|
||
def sensor_loader(data, parent_key: str) -> list[tuple[Sensor, str]]: | ||
"""Load all sensors referenced by their ID in a nested dict or list, along with the fields referring to them. | ||
|
||
:param data: nested dict or list | ||
:param parent_key: 'flex-model' or 'flex-context' | ||
:returns: list of sensor-field tuples | ||
""" | ||
sensor_ids = find_sensor_ids(data, parent_key) | ||
sensors = [ | ||
(db.session.get(Sensor, sensor_id), field_name) | ||
for sensor_id, field_name in sensor_ids | ||
] | ||
return sensors | ||
|
||
|
||
flex_model_loader = partial(sensor_loader, parent_key="flex-model") | ||
flex_context_loader = partial(sensor_loader, parent_key="flex-context") | ||
|
||
|
||
def find_sensor_ids(data, parent_key="") -> list[tuple[int, str]]: | ||
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. Type hint |
||
""" | ||
Recursively find all sensor IDs in a nested dictionary or list along with the fields referring to them. | ||
|
||
Args: | ||
data (dict or list): The input data which can be a dictionary or a list containing nested dictionaries and lists. | ||
parent_key (str): The key of the parent element in the recursion, used to track the referring fields. | ||
|
||
Returns: | ||
list: A list of tuples, each containing a sensor ID and the referring field. | ||
|
||
Example: | ||
nested_dict = { | ||
"flex-model": [ | ||
{ | ||
"sensor": 931, | ||
"soc-at-start": 12.1, | ||
"soc-unit": "kWh", | ||
"soc-targets": [ | ||
{ | ||
"value": 25, | ||
"datetime": "2015-06-02T16:00:00+00:00" | ||
}, | ||
], | ||
"soc-minima": {"sensor": 300}, | ||
"soc-min": 10, | ||
"soc-max": 25, | ||
"charging-efficiency": "120%", | ||
"discharging-efficiency": {"sensor": 98}, | ||
"storage-efficiency": 0.9999, | ||
"power-capacity": "25kW", | ||
"consumption-capacity": {"sensor": 42}, | ||
"production-capacity": "30 kW" | ||
}, | ||
], | ||
} | ||
|
||
sensor_ids = find_sensor_ids(nested_dict) | ||
print(sensor_ids) # Output: [(931, 'sensor'), (300, 'soc-minima.sensor'), (98, 'discharging-efficiency.sensor'), (42, 'consumption-capacity.sensor')] | ||
""" | ||
sensor_ids = [] | ||
|
||
if isinstance(data, dict): | ||
for key, value in data.items(): | ||
new_parent_key = f"{parent_key}.{key}" if parent_key else key | ||
if key[-6:] == "sensor": | ||
sensor_ids.append((value, new_parent_key)) | ||
elif key[-7:] == "sensors": | ||
for v in value: | ||
sensor_ids.append((v, new_parent_key)) | ||
else: | ||
sensor_ids.extend(find_sensor_ids(value, new_parent_key)) | ||
elif isinstance(data, list): | ||
for index, item in enumerate(data): | ||
new_parent_key = f"{parent_key}[{index}]" | ||
sensor_ids.extend(find_sensor_ids(item, new_parent_key)) | ||
|
||
return sensor_ids |
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'd add a comment: "This extracts sensors from the flex model parameter - user needs read access to them"