From 6412ce8adf388ced360a53b5aba78ea584e62f83 Mon Sep 17 00:00:00 2001 From: "F.N. Claessen" Date: Wed, 30 Mar 2022 16:56:41 +0200 Subject: [PATCH 1/6] Mask inaccessible assets (workaround for issue #200) Signed-off-by: F.N. Claessen --- flexmeasures/data/services/asset_grouping.py | 29 ++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/flexmeasures/data/services/asset_grouping.py b/flexmeasures/data/services/asset_grouping.py index d6a73c756..36b68090d 100644 --- a/flexmeasures/data/services/asset_grouping.py +++ b/flexmeasures/data/services/asset_grouping.py @@ -4,11 +4,13 @@ """ from __future__ import annotations -from typing import List, Dict, Optional +from typing import List, Dict, Optional, Union import inflect +from flask_security.core import current_user from sqlalchemy.orm import Query +from flexmeasures.auth.policy import ADMIN_ROLE from flexmeasures.utils.flexmeasures_inflection import parameterize, pluralize from flexmeasures.data.models.generic_assets import ( GenericAssetType, @@ -28,7 +30,7 @@ def get_asset_group_queries( group_by_location: bool = False, ) -> Dict[str, Query]: """ - An asset group is defined by Asset queries. Each query has a name, and we prefer pluralised names. + An asset group is defined by Asset queries. Each query has a name, and we prefer pluralised names. They still need an executive call, like all(), count() or first(). This function limits the assets to be queried to the current user's account, @@ -56,6 +58,8 @@ def get_asset_group_queries( if group_by_location: asset_queries.update(get_location_queries()) + asset_queries = mask_inaccessible_assets(asset_queries) + return asset_queries @@ -143,3 +147,24 @@ def parameterized_name(self) -> str: def __str__(self): return self.display_name + + +def mask_inaccessible_assets( + asset_queries: Union[Query, Dict[str, Query]] +) -> Union[Query, Dict[str, Query]]: + """Filter out any assets that the user should not be able to access. + + We do not explicitly check user authentication here, because non-authenticated users are not admins + and have no asset ownership, so applying this filter for non-admins masks all assets. + """ + if not current_user.has_role(ADMIN_ROLE): + if isinstance(asset_queries, dict): + for name, query in asset_queries.items(): + asset_queries[name] = query.filter( + GenericAsset.owner == current_user.account + ) + else: + asset_queries = asset_queries.filter( + GenericAsset.owner == current_user.account + ) + return asset_queries From c3251343800b12c45132b78c791f1209198f001e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20H=C3=B6ning?= Date: Wed, 30 Mar 2022 23:46:40 +0200 Subject: [PATCH 2/6] fix limiting query to current account MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Nicolas Höning --- flexmeasures/cli/__init__.py | 14 ++++++++++ flexmeasures/data/queries/utils.py | 14 +++++----- flexmeasures/data/services/asset_grouping.py | 27 +------------------- 3 files changed, 22 insertions(+), 33 deletions(-) diff --git a/flexmeasures/cli/__init__.py b/flexmeasures/cli/__init__.py index 8718348bb..cb3eb1d3c 100644 --- a/flexmeasures/cli/__init__.py +++ b/flexmeasures/cli/__init__.py @@ -1,3 +1,5 @@ +import sys + from flask import Flask @@ -13,3 +15,15 @@ def register_at(app: Flask): import flexmeasures.cli.data_delete import flexmeasures.cli.db_ops import flexmeasures.cli.testing # noqa: F401 + + +def is_running() -> bool: + """ + True if we are running one of the custom FlexMeasures CLI commands. + """ + cli_sets = ("add", "delete", "show", "monitor", "jobs", "db-ops") + command_line = " ".join(sys.argv) + for cli_set in cli_sets: + if f"flexmeasures {cli_set}" in command_line: + return True + return False diff --git a/flexmeasures/data/queries/utils.py b/flexmeasures/data/queries/utils.py index 0817debbb..a52f97631 100644 --- a/flexmeasures/data/queries/utils.py +++ b/flexmeasures/data/queries/utils.py @@ -1,18 +1,18 @@ from typing import List, Optional, Type, Tuple, Union from datetime import datetime, timedelta -from flask import current_app from flask_security import current_user from werkzeug.exceptions import Forbidden import pandas as pd import timely_beliefs as tb from sqlalchemy.orm import Query, Session -from sqlalchemy.sql.elements import BinaryExpression +from sqlalchemy.sql.elements import BinaryExpression, or_ from flexmeasures.data.config import db from flexmeasures.data.models.generic_assets import GenericAsset from flexmeasures.data.models.data_sources import DataSource from flexmeasures.utils import flexmeasures_inflection +from flexmeasures.cli import is_running as running_as_cli import flexmeasures.data.models.time_series as ts # noqa: F401 from flexmeasures.auth.policy import ADMIN_ROLE, ADMIN_READER_ROLE @@ -49,10 +49,10 @@ def potentially_limit_query_to_account_assets( :param account_id: if set, all assets that are not in the given account will be filtered out (only works for admins and CLI users). For querying public assets in particular, don't use this function. """ - if not current_app.cli and not current_user.is_authenticated: + if not running_as_cli() and not current_user.is_authenticated: raise Forbidden("Unauthenticated user cannot list assets.") user_is_admin = ( - current_app.cli + running_as_cli() or current_user.has_role(ADMIN_ROLE) or current_user.has_role(ADMIN_READER_ROLE) ) @@ -68,9 +68,9 @@ def potentially_limit_query_to_account_assets( account_id if account_id is not None else current_user.account_id ) return query.filter( - ( - GenericAsset.account_id == account_id_to_filter - or GenericAsset.account_id is None + or_( + GenericAsset.account_id == account_id_to_filter, + GenericAsset.account_id is None, ) ) diff --git a/flexmeasures/data/services/asset_grouping.py b/flexmeasures/data/services/asset_grouping.py index 36b68090d..b037f662c 100644 --- a/flexmeasures/data/services/asset_grouping.py +++ b/flexmeasures/data/services/asset_grouping.py @@ -4,13 +4,11 @@ """ from __future__ import annotations -from typing import List, Dict, Optional, Union +from typing import List, Dict, Optional import inflect -from flask_security.core import current_user from sqlalchemy.orm import Query -from flexmeasures.auth.policy import ADMIN_ROLE from flexmeasures.utils.flexmeasures_inflection import parameterize, pluralize from flexmeasures.data.models.generic_assets import ( GenericAssetType, @@ -58,8 +56,6 @@ def get_asset_group_queries( if group_by_location: asset_queries.update(get_location_queries()) - asset_queries = mask_inaccessible_assets(asset_queries) - return asset_queries @@ -147,24 +143,3 @@ def parameterized_name(self) -> str: def __str__(self): return self.display_name - - -def mask_inaccessible_assets( - asset_queries: Union[Query, Dict[str, Query]] -) -> Union[Query, Dict[str, Query]]: - """Filter out any assets that the user should not be able to access. - - We do not explicitly check user authentication here, because non-authenticated users are not admins - and have no asset ownership, so applying this filter for non-admins masks all assets. - """ - if not current_user.has_role(ADMIN_ROLE): - if isinstance(asset_queries, dict): - for name, query in asset_queries.items(): - asset_queries[name] = query.filter( - GenericAsset.owner == current_user.account - ) - else: - asset_queries = asset_queries.filter( - GenericAsset.owner == current_user.account - ) - return asset_queries From 19631cedff1eec9119c3fc8d3618b94e89390b94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20H=C3=B6ning?= Date: Thu, 31 Mar 2022 10:58:17 +0200 Subject: [PATCH 3/6] fix checking for ResponseTuples in API code (type() will say they are a tuple) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Nicolas Höning --- flexmeasures/api/common/responses.py | 13 +++++++++++++ flexmeasures/api/v1_1/implementations.py | 6 +++--- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/flexmeasures/api/common/responses.py b/flexmeasures/api/common/responses.py index 143512458..7b30465c4 100644 --- a/flexmeasures/api/common/responses.py +++ b/flexmeasures/api/common/responses.py @@ -11,6 +11,19 @@ ResponseTuple = Tuple[dict, int] +def is_response_tuple(value) -> bool: + """Check if an object qualifies as a ResponseTuple""" + if not isinstance(value, tuple): + return False + if not len(value) == 2: + return False + if not isinstance(value[0], dict): + return False + if not isinstance(value[1], int): + return False + return True + + class BaseMessage: """Set a base message to which extra info can be added by calling the wrapped function with additional string arguments. This is a decorator implemented as a class.""" diff --git a/flexmeasures/api/v1_1/implementations.py b/flexmeasures/api/v1_1/implementations.py index 0b94c382b..37a59f270 100644 --- a/flexmeasures/api/v1_1/implementations.py +++ b/flexmeasures/api/v1_1/implementations.py @@ -13,8 +13,8 @@ from flexmeasures.api.common.responses import ( invalid_domain, invalid_unit, - ResponseTuple, invalid_horizon, + is_response_tuple, ) from flexmeasures.api.common.utils.api_utils import ( save_and_enqueue, @@ -98,7 +98,7 @@ def post_price_data_response( # Look for the Sensor object sensor = get_sensor_by_unique_name(market_name, ["day_ahead", "tou_tariff"]) - if type(sensor) == ResponseTuple: + if is_response_tuple(sensor): # Error message telling the user what to do return sensor if unit != sensor.unit: @@ -185,7 +185,7 @@ def post_weather_data_response( # noqa: C901 sensor = get_sensor_by_generic_asset_type_and_location( weather_sensor_type_name, latitude, longitude ) - if type(sensor) == ResponseTuple: + if is_response_tuple(sensor): # Error message telling the user about the nearest weather sensor they can post to return sensor From 6c0bb2fcd37563ff64c36c0ea3c1376384eaed74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20H=C3=B6ning?= Date: Thu, 31 Mar 2022 11:24:45 +0200 Subject: [PATCH 4/6] fix querying for null values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Nicolas Höning --- flexmeasures/data/queries/utils.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/flexmeasures/data/queries/utils.py b/flexmeasures/data/queries/utils.py index a52f97631..5a7e7b9b6 100644 --- a/flexmeasures/data/queries/utils.py +++ b/flexmeasures/data/queries/utils.py @@ -7,6 +7,7 @@ import timely_beliefs as tb from sqlalchemy.orm import Query, Session from sqlalchemy.sql.elements import BinaryExpression, or_ +from sqlalchemy.sql.expression import null from flexmeasures.data.config import db from flexmeasures.data.models.generic_assets import GenericAsset @@ -70,7 +71,7 @@ def potentially_limit_query_to_account_assets( return query.filter( or_( GenericAsset.account_id == account_id_to_filter, - GenericAsset.account_id is None, + GenericAsset.account_id == null(), ) ) From 102f4155d471e9cfeae9394986545d332eb24bcd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicolas=20H=C3=B6ning?= Date: Thu, 31 Mar 2022 13:22:06 +0200 Subject: [PATCH 5/6] introduce a way for tests that test backend functionality to pretend they run via the CLI, so their querying is not hampered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Nicolas Höning --- flexmeasures/cli/__init__.py | 14 +++++++++++++- flexmeasures/conftest.py | 11 +++++++++++ flexmeasures/data/tests/test_forecasting_jobs.py | 14 ++++++++++---- .../data/tests/test_forecasting_jobs_fresh_db.py | 15 ++++++++++++--- flexmeasures/data/tests/test_sensor_queries.py | 2 +- 5 files changed, 47 insertions(+), 9 deletions(-) diff --git a/flexmeasures/cli/__init__.py b/flexmeasures/cli/__init__.py index cb3eb1d3c..20f98d921 100644 --- a/flexmeasures/cli/__init__.py +++ b/flexmeasures/cli/__init__.py @@ -1,6 +1,6 @@ import sys -from flask import Flask +from flask import Flask, current_app def register_at(app: Flask): @@ -20,10 +20,22 @@ def register_at(app: Flask): def is_running() -> bool: """ True if we are running one of the custom FlexMeasures CLI commands. + + We use this in combination with authorization logic, e.g. we assume that only sysadmins run commands there, + but also we consider forecasting & scheduling jobs to be in that realm, as well. + + This tooling might not live forever, as we could evolve into a more sophisticated auth model for these cases. + For instance, these jobs are queued by the system, but caused by user actions (sending data), and then they are run by the system. + + See also: the run_as_cli test fixture, which uses the (non-public) PRETEND_RUNNING_AS_CLI env setting. + + TODO: How can plugins add their CLI set here, should they need that? """ cli_sets = ("add", "delete", "show", "monitor", "jobs", "db-ops") command_line = " ".join(sys.argv) for cli_set in cli_sets: if f"flexmeasures {cli_set}" in command_line: return True + if current_app.config.get("PRETEND_RUNNING_AS_CLI", False): + return True return False diff --git a/flexmeasures/conftest.py b/flexmeasures/conftest.py index 29af4f134..8438cb1c6 100644 --- a/flexmeasures/conftest.py +++ b/flexmeasures/conftest.py @@ -739,6 +739,17 @@ def battery_soc_sensor(db: SQLAlchemy, setup_generic_assets): return soc_sensor +@pytest.fixture +def run_as_cli(app, monkeypatch): + """ + Use this to run your test as if it is run from the CLI. + This is useful where some auth restrictions (e.g. for querying) are in place. + FlexMeasures is more lenient with them if the CLI is running, as it considers + the user a sysadmin. + """ + monkeypatch.setitem(app.config, "PRETEND_RUNNING_AS_CLI", True) + + @pytest.fixture(scope="function") def clean_redis(app): failed = app.queues["forecasting"].failed_job_registry diff --git a/flexmeasures/data/tests/test_forecasting_jobs.py b/flexmeasures/data/tests/test_forecasting_jobs.py index a8815cd55..c664059c8 100644 --- a/flexmeasures/data/tests/test_forecasting_jobs.py +++ b/flexmeasures/data/tests/test_forecasting_jobs.py @@ -46,7 +46,7 @@ def check_aggregate(overall_expected: int, horizon: timedelta, sensor_id: int): assert all([not np.isnan(f.event_value) for f in all_forecasts]) -def test_forecasting_an_hour_of_wind(db, app, setup_test_data): +def test_forecasting_an_hour_of_wind(db, run_as_cli, app, setup_test_data): """Test one clean run of one job: - data source was made, - forecasts have been made @@ -84,7 +84,9 @@ def test_forecasting_an_hour_of_wind(db, app, setup_test_data): check_aggregate(4, horizon, wind_device_1.id) -def test_forecasting_two_hours_of_solar_at_edge_of_data_set(db, app, setup_test_data): +def test_forecasting_two_hours_of_solar_at_edge_of_data_set( + db, run_as_cli, app, setup_test_data +): solar_device1: Sensor = Sensor.query.filter_by(name="solar-asset-1").one_or_none() last_power_datetime = ( @@ -168,7 +170,9 @@ def check_failures( assert job.meta["model_identifier"] == model_identifiers[job_idx] -def test_failed_forecasting_insufficient_data(app, clean_redis, setup_test_data): +def test_failed_forecasting_insufficient_data( + app, run_as_cli, clean_redis, setup_test_data +): """This one (as well as the fallback) should fail as there is no underlying data. (Power data is in 2015)""" solar_device1: Sensor = Sensor.query.filter_by(name="solar-asset-1").one_or_none() @@ -183,7 +187,9 @@ def test_failed_forecasting_insufficient_data(app, clean_redis, setup_test_data) check_failures(app.queues["forecasting"], 2 * ["NotEnoughDataException"]) -def test_failed_forecasting_invalid_horizon(app, clean_redis, setup_test_data): +def test_failed_forecasting_invalid_horizon( + app, run_as_cli, clean_redis, setup_test_data +): """This one (as well as the fallback) should fail as the horizon is invalid.""" solar_device1: Sensor = Sensor.query.filter_by(name="solar-asset-1").one_or_none() create_forecasting_jobs( diff --git a/flexmeasures/data/tests/test_forecasting_jobs_fresh_db.py b/flexmeasures/data/tests/test_forecasting_jobs_fresh_db.py index b1660b334..a01b1cbfc 100644 --- a/flexmeasures/data/tests/test_forecasting_jobs_fresh_db.py +++ b/flexmeasures/data/tests/test_forecasting_jobs_fresh_db.py @@ -18,7 +18,9 @@ from flexmeasures.utils.time_utils import as_server_time -def test_forecasting_three_hours_of_wind(app, setup_fresh_test_data, clean_redis): +def test_forecasting_three_hours_of_wind( + app, run_as_cli, setup_fresh_test_data, clean_redis +): wind_device2: Sensor = Sensor.query.filter_by(name="wind-asset-2").one_or_none() # makes 12 forecasts @@ -47,7 +49,9 @@ def test_forecasting_three_hours_of_wind(app, setup_fresh_test_data, clean_redis check_aggregate(12, horizon, wind_device2.id) -def test_forecasting_two_hours_of_solar(app, setup_fresh_test_data, clean_redis): +def test_forecasting_two_hours_of_solar( + app, run_as_cli, setup_fresh_test_data, clean_redis +): solar_device1: Sensor = Sensor.query.filter_by(name="solar-asset-1").one_or_none() wind_device2: Sensor = Sensor.query.filter_by(name="wind-asset-2").one_or_none() print(solar_device1) @@ -82,7 +86,12 @@ def test_forecasting_two_hours_of_solar(app, setup_fresh_test_data, clean_redis) "model_to_start_with, model_version", [("failing-test", 1), ("linear-OLS", 2)] ) def test_failed_model_with_too_much_training_then_succeed_with_fallback( - setup_fresh_test_data, app, clean_redis, model_to_start_with, model_version + app, + run_as_cli, + clean_redis, + setup_fresh_test_data, + model_to_start_with, + model_version, ): """ Here we fail once - because we start with a model that needs too much training. diff --git a/flexmeasures/data/tests/test_sensor_queries.py b/flexmeasures/data/tests/test_sensor_queries.py index 241bb0bf9..0cdedd4ee 100644 --- a/flexmeasures/data/tests/test_sensor_queries.py +++ b/flexmeasures/data/tests/test_sensor_queries.py @@ -1,7 +1,7 @@ from flexmeasures.data.models.time_series import Sensor -def test_closest_sensor(add_nearby_weather_sensors): +def test_closest_sensor(run_as_cli, add_nearby_weather_sensors): """Check that the closest temperature sensor to our wind sensor returns the one that is on the same spot as the wind sensor itself. (That's where we set it up in our conftest.) From 8e3e41a6a4dbf5bc8c58f930a9fc7553a21115b5 Mon Sep 17 00:00:00 2001 From: "F.N. Claessen" Date: Thu, 31 Mar 2022 15:31:25 +0200 Subject: [PATCH 6/6] Changelog entry Signed-off-by: F.N. Claessen --- documentation/changelog.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/documentation/changelog.rst b/documentation/changelog.rst index f1e2d53c9..6bb5911ee 100644 --- a/documentation/changelog.rst +++ b/documentation/changelog.rst @@ -16,6 +16,14 @@ Infrastructure / Support ---------------------- +v0.9.1 | March 31, 2022 +=========================== + +Bugfixes +-------- +* Fix auth bug not masking locations of inaccessible assets on map [see `PR #409 `_] + + v0.9.0 | March 25, 2022 ===========================