From 71fd2cb30e244ab8db594fb9393d587c0159c512 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A5kon=20V=2E=20Treider?= Date: Fri, 17 Nov 2023 00:56:37 +0100 Subject: [PATCH 1/5] fix database scope for raw Acl --- cognite/client/_api/iam.py | 33 +++++++++++----- cognite/client/data_classes/capabilities.py | 8 ++-- .../test_data_classes/test_capabilities.py | 38 +++++++++++++++++++ 3 files changed, 66 insertions(+), 13 deletions(-) diff --git a/cognite/client/_api/iam.py b/cognite/client/_api/iam.py index 59fd4a8ecb..8a20c2a5af 100644 --- a/cognite/client/_api/iam.py +++ b/cognite/client/_api/iam.py @@ -22,9 +22,11 @@ SessionList, ) from cognite.client.data_classes.capabilities import ( + AllScope, Capability, ProjectCapability, ProjectCapabilityList, + RawAcl, ) from cognite.client.data_classes.iam import TokenInspection from cognite.client.utils._identifier import IdentifierSequence @@ -46,8 +48,6 @@ def _convert_capability_to_tuples(capabilities: ComparableCapability, project: str | None = None) -> set[tuple]: - from cognite.client.data_classes import Group, GroupList - if isinstance(capabilities, ProjectCapability): return ProjectCapabilityList([capabilities]).as_tuples(project) if isinstance(capabilities, ProjectCapabilityList): @@ -59,7 +59,6 @@ def _convert_capability_to_tuples(capabilities: ComparableCapability, project: s capabilities = capabilities.capabilities or [] elif isinstance(capabilities, GroupList): capabilities = [cap for grp in capabilities for cap in grp.capabilities or []] - if isinstance(capabilities, Sequence): tpls: set[tuple] = set() for cap in capabilities: @@ -100,8 +99,10 @@ def compare_capabilities( project (str | None): If a ProjectCapability or ProjectCapabilityList is passed, we need to know which CDF project to pull capabilities from (existing might be from several). If project is not passed, and ProjectCapabilityList is used, it will be inferred from the CogniteClient used to call retrieve it via token/inspect. - ignore_allscope_meaning (bool): Option on how to treat allScopes. When True, this function will return - e.g. an Acl scoped to a dataset even if the user have the same Acl scoped to all. Defaults to False. + ignore_allscope_meaning (bool): Option on how to treat scopes that encompass other scopes, like allScope. When True, + this function will return e.g. an Acl scoped to a dataset even if the user have the same Acl scoped to all. The + same logic applies to RawAcl scoped to a specific database->table, even when the user have access to all tables + in that database. Defaults to False. Returns: list[Capability]: A flattened list of the missing capabilities, meaning they each have exactly 1 action, 1 scope, 1 id etc. @@ -147,18 +148,30 @@ def compare_capabilities( to_check = _convert_capability_to_tuples(desired_capabilities, project) missing = to_check - has_capabilties - if ignore_allscope_meaning: - return [Capability.from_tuple(tpl) for tpl in missing] - has_capabilties_lookup = {k: set(grp) for k, grp in groupby(sorted(has_capabilties), key=itemgetter(slice(2)))} to_check_lookup = {k: set(grp) for k, grp in groupby(sorted(missing), key=itemgetter(slice(2)))} missing.clear() + raw_group, raw_check_grp = set(), set() for key, check_grp in to_check_lookup.items(): group = has_capabilties_lookup.get(key, set()) - # If allScope exists for capability, we skip the missing: - if not any(grp[2] == "all" for grp in group): + if any(AllScope._scope_name == grp[2] for grp in group): + continue # If allScope exists for capability, we safely skip ahead: + elif RawAcl._capability_name == next(iter(check_grp))[0]: + raw_group.update(group) + raw_check_grp.update(check_grp) + else: missing.update(check_grp) + + # Special handling of rawAcl which has a "hidden" database scope between "all" and "tables": + raw_to_check = {k: sorted(grp) for k, grp in groupby(sorted(raw_check_grp), key=itemgetter(slice(4)))} + raw_has_capabs = {k: sorted(grp) for k, grp in groupby(sorted(raw_group), key=itemgetter(slice(4)))} + for key, check_db_grp in raw_to_check.items(): + if (db_group := raw_has_capabs.get(key)) and not db_group[0][-1]: + # [0] because empty string sorts first; [-1] is table; if no table -> db scope -> skip ahead + continue + missing.update(check_db_grp) + return [Capability.from_tuple(tpl) for tpl in missing] def verify_capabilities( diff --git a/cognite/client/data_classes/capabilities.py b/cognite/client/data_classes/capabilities.py index 46b0326e12..a63f0bcb43 100644 --- a/cognite/client/data_classes/capabilities.py +++ b/cognite/client/data_classes/capabilities.py @@ -113,9 +113,9 @@ def from_tuple(cls, tpl: tuple) -> Self: scope = scope_cls(scope_params) # type: ignore [call-arg] elif len(scope_params) == 2 and scope_cls is TableScope: db, tbl = scope_params - scope = scope_cls({db: [tbl]}) # type: ignore [call-arg] + scope = scope_cls({db: [tbl] if tbl else []}) # type: ignore [call-arg] else: - raise ValueError(f"tuple not understood ({tpl})") + raise ValueError(f"tuple not understood as capability ({tpl})") return cast(Self, capability_cls(actions=[capability_cls.Action(action)], scope=scope)) @@ -357,7 +357,9 @@ def dump(self, camel_case: bool = True) -> dict[str, Any]: return {self._scope_name: {key: {k: {"tables": v} for k, v in self.dbs_to_tables.items()}}} def as_tuples(self) -> set[tuple]: - return {(self._scope_name, db, tbl) for db, tables in self.dbs_to_tables.items() for tbl in tables} + # When the scope contains no tables, it means all tables... since database name must be at least 1 + # character, we represent this internally with the empty string: + return {(self._scope_name, db, tbl) for db, tables in self.dbs_to_tables.items() for tbl in tables or [""]} def is_within(self, other: Self) -> bool: if isinstance(other, AllScope): diff --git a/tests/tests_unit/test_data_classes/test_capabilities.py b/tests/tests_unit/test_data_classes/test_capabilities.py index 251ae43c6a..13692ea432 100644 --- a/tests/tests_unit/test_data_classes/test_capabilities.py +++ b/tests/tests_unit/test_data_classes/test_capabilities.py @@ -19,6 +19,7 @@ ProjectCapabilityList, ProjectsAcl, RawAcl, + TableScope, UnknownAcl, UnknownScope, ) @@ -301,6 +302,8 @@ def test_project_scope_is_all_projects(self, proj_cap_allprojects_dct): loaded = ProjectCapabilityList.load([proj_cap_allprojects_dct]) assert type(loaded[0].project_scope) is AllProjectsScope + +class TestIAMCompareCapabilities: @pytest.mark.parametrize( "capability", [ @@ -347,6 +350,41 @@ def test_partly_missing_capabilities( ) assert missing_acls == [has_not] + def test_raw_acl_database_scope_only(self, cognite_client): + # Would fail with: 'ValueError: No capabilities given' prior to 7.2.1 due to a bug in 'as_tuples'. + has_all_scope = RawAcl([RawAcl.Action.Read], AllScope) + has_db_scope = RawAcl([RawAcl.Action.Read], TableScope(dbs_to_tables={"db1": []})) + assert not cognite_client.iam.compare_capabilities(has_all_scope, has_db_scope) + + @pytest.mark.parametrize( + "extra_existing, no_read_missing", + [ + ([], False), + ([RawAcl(actions=[RawAcl.Action.Read], scope=AllScope)], True), + ([RawAcl(actions=[RawAcl.Action.Read], scope=TableScope({"db1": []}))], True), + ([RawAcl(actions=[RawAcl.Action.Read], scope=TableScope({"db1": {"tables": []}}))], True), + ], + ) + def test_raw_acl_database_scope(self, cognite_client, extra_existing, no_read_missing): + existing = [ + RawAcl([RawAcl.Action.Read], RawAcl.Scope.Table({"db1": ["t1"]})), + RawAcl([RawAcl.Action.Read], RawAcl.Scope.Table({"db1": ["t1", "t2"]})), + RawAcl([RawAcl.Action.Read], RawAcl.Scope.Table({"db2": ["t1", "t2"]})), + *extra_existing, + ] + desired = [ + RawAcl([RawAcl.Action.Read], RawAcl.Scope.Table({"db1": ["t1", "t2", "t3"]})), + RawAcl([RawAcl.Action.Write], RawAcl.Scope.Table({"db1": ["t1"]})), + ] + missing = cognite_client.iam.compare_capabilities(existing, desired) + if no_read_missing: + assert missing == [RawAcl([RawAcl.Action.Write], RawAcl.Scope.Table(dbs_to_tables={"db1": ["t1"]}))] + else: + assert missing == [ + RawAcl([RawAcl.Action.Read], RawAcl.Scope.Table(dbs_to_tables={"db1": ["t3"]})), + RawAcl([RawAcl.Action.Write], RawAcl.Scope.Table(dbs_to_tables={"db1": ["t1"]})), + ] + @pytest.mark.parametrize( "dct", From d165f42c1096031b5a07d2ae5e7bc69008d39d8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A5kon=20V=2E=20Treider?= Date: Fri, 17 Nov 2023 01:18:39 +0100 Subject: [PATCH 2/5] changelog and version --- CHANGELOG.md | 11 ++++++++--- cognite/client/_version.py | 2 +- pyproject.toml | 2 +- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e3191231f..2bbd8ffa2c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,11 @@ Changes are grouped as follows - `Fixed` for any bug fixes. - `Security` in case of vulnerabilities. +## [7.2.1] - 2023-11-17 +### Fixed +- The new compare methods for capabilities in major version 7, `IAMAPI.verify_capabilities` and `IAMAPI.compare_capabilities` + now works correctly for rawAcl with database scope ("all tables"). + ## [7.2.0] - 2023-11-16 ### Added - The `trigger` method of the Workflow Execution API, now accepts a `client_credentials` to allow specifying specific @@ -169,9 +174,9 @@ with no easy way to add a prefix. Also, it no longer expands metadata by default ## [6.39.6] - 2023-11-13 ## Fixed - HTTP status code retry strategy for RAW and labels. `/rows/insert` and `/rows/delete` will now -be retried for all status codes in `config.status_forcelist` (default 429, 502, 503, 504), while -`/dbs/{db}` and `/tables/{table}` will now only be retried for 429s and connection errors as those -endpoints are not idempotent. + be retried for all status codes in `config.status_forcelist` (default 429, 502, 503, 504), while + `/dbs/{db}` and `/tables/{table}` will now only be retried for 429s and connection errors as those + endpoints are not idempotent. - Also, `labels/list` will now also be retried. ## [6.39.5] - 2023-11-12 diff --git a/cognite/client/_version.py b/cognite/client/_version.py index 318fcef52c..218d5db77a 100644 --- a/cognite/client/_version.py +++ b/cognite/client/_version.py @@ -1,4 +1,4 @@ from __future__ import annotations -__version__ = "7.2.0" +__version__ = "7.2.1" __api_subversion__ = "V20220125" diff --git a/pyproject.toml b/pyproject.toml index 52ddac8e74..2627867de8 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,7 +1,7 @@ [tool.poetry] name = "cognite-sdk" -version = "7.2.0" +version = "7.2.1" description = "Cognite Python SDK" readme = "README.md" documentation = "https://cognite-sdk-python.readthedocs-hosted.com" From d39175b90dc21f432958ec8b1a688fa48a9152cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A5kon=20V=2E=20Treider?= Date: Fri, 17 Nov 2023 01:29:47 +0100 Subject: [PATCH 3/5] simplify test parametrize args --- .../test_data_classes/test_capabilities.py | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/tests_unit/test_data_classes/test_capabilities.py b/tests/tests_unit/test_data_classes/test_capabilities.py index 13692ea432..e49dbcdd4f 100644 --- a/tests/tests_unit/test_data_classes/test_capabilities.py +++ b/tests/tests_unit/test_data_classes/test_capabilities.py @@ -357,15 +357,15 @@ def test_raw_acl_database_scope_only(self, cognite_client): assert not cognite_client.iam.compare_capabilities(has_all_scope, has_db_scope) @pytest.mark.parametrize( - "extra_existing, no_read_missing", + "extra_existing", [ - ([], False), - ([RawAcl(actions=[RawAcl.Action.Read], scope=AllScope)], True), - ([RawAcl(actions=[RawAcl.Action.Read], scope=TableScope({"db1": []}))], True), - ([RawAcl(actions=[RawAcl.Action.Read], scope=TableScope({"db1": {"tables": []}}))], True), + [], + [RawAcl(actions=[RawAcl.Action.Read], scope=AllScope)], + [RawAcl(actions=[RawAcl.Action.Read], scope=TableScope({"db1": []}))], + [RawAcl(actions=[RawAcl.Action.Read], scope=TableScope({"db1": {"tables": []}}))], ], ) - def test_raw_acl_database_scope(self, cognite_client, extra_existing, no_read_missing): + def test_raw_acl_database_scope(self, cognite_client, extra_existing): existing = [ RawAcl([RawAcl.Action.Read], RawAcl.Scope.Table({"db1": ["t1"]})), RawAcl([RawAcl.Action.Read], RawAcl.Scope.Table({"db1": ["t1", "t2"]})), @@ -377,12 +377,12 @@ def test_raw_acl_database_scope(self, cognite_client, extra_existing, no_read_mi RawAcl([RawAcl.Action.Write], RawAcl.Scope.Table({"db1": ["t1"]})), ] missing = cognite_client.iam.compare_capabilities(existing, desired) - if no_read_missing: - assert missing == [RawAcl([RawAcl.Action.Write], RawAcl.Scope.Table(dbs_to_tables={"db1": ["t1"]}))] + if extra_existing: + assert missing == [RawAcl([RawAcl.Action.Write], RawAcl.Scope.Table({"db1": ["t1"]}))] else: assert missing == [ - RawAcl([RawAcl.Action.Read], RawAcl.Scope.Table(dbs_to_tables={"db1": ["t3"]})), - RawAcl([RawAcl.Action.Write], RawAcl.Scope.Table(dbs_to_tables={"db1": ["t1"]})), + RawAcl([RawAcl.Action.Read], RawAcl.Scope.Table({"db1": ["t3"]})), + RawAcl([RawAcl.Action.Write], RawAcl.Scope.Table({"db1": ["t1"]})), ] From 223221e9850ef9c7443f971b11a399e21f84c522 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A5kon=20V=2E=20Treider?= Date: Fri, 17 Nov 2023 01:30:29 +0100 Subject: [PATCH 4/5] remove is_within from scopes and has_capability from capabilities --- CHANGELOG.md | 3 + cognite/client/data_classes/capabilities.py | 61 +-------------------- 2 files changed, 4 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2bbd8ffa2c..774b940ffc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,9 @@ Changes are grouped as follows ### Fixed - The new compare methods for capabilities in major version 7, `IAMAPI.verify_capabilities` and `IAMAPI.compare_capabilities` now works correctly for rawAcl with database scope ("all tables"). +### Removed +- Capability scopes no longer have the `is_within` method, and capabilities no longer have `has_capability`. Use the more + general `IAMAPI.compare_capabilities` instead. ## [7.2.0] - 2023-11-16 ### Added diff --git a/cognite/client/data_classes/capabilities.py b/cognite/client/data_classes/capabilities.py index a63f0bcb43..7d75740cb4 100644 --- a/cognite/client/data_classes/capabilities.py +++ b/cognite/client/data_classes/capabilities.py @@ -98,9 +98,6 @@ def as_tuples(self) -> set[tuple]: # Basic implementation for all simple Scopes (e.g. all or currentuser) return {(self._scope_name,)} - def is_within(self, other: Self) -> bool: - raise NotImplementedError - @classmethod def from_tuple(cls, tpl: tuple) -> Self: acl_name, action, scope_name, *scope_params = tpl @@ -115,7 +112,7 @@ def from_tuple(cls, tpl: tuple) -> Self: db, tbl = scope_params scope = scope_cls({db: [tbl] if tbl else []}) # type: ignore [call-arg] else: - raise ValueError(f"tuple not understood as capability ({tpl})") + raise ValueError(f"tuple not understood as capability: {tpl}") return cast(Self, capability_cls(actions=[capability_cls.Action(action)], scope=scope)) @@ -162,13 +159,6 @@ def dump(self, camel_case: bool = True) -> dict[str, Any]: capability_name = self._capability_name return {to_camel_case(capability_name) if camel_case else to_snake_case(capability_name): data} - def has_capability(self, other: Capability) -> bool: - if not isinstance(self, type(other)): - return False - if not other.scope.is_within(self.scope): - return False - return not set(other.actions) - set(self.actions) - def as_tuples(self) -> set[tuple]: return set( (acl, action, *scope_tpl) @@ -262,17 +252,11 @@ def as_tuples(self, project: str | None = None) -> set[tuple]: class AllScope(Capability.Scope): _scope_name = "all" - def is_within(self, other: Self) -> bool: - return isinstance(other, AllScope) - @dataclass(frozen=True) class CurrentUserScope(Capability.Scope): _scope_name = "currentuserscope" - def is_within(self, other: Self) -> bool: - return isinstance(other, (AllScope, CurrentUserScope)) - @dataclass(frozen=True) class IDScope(Capability.Scope): @@ -285,9 +269,6 @@ def __post_init__(self) -> None: def as_tuples(self) -> set[tuple]: return {(self._scope_name, i) for i in self.ids} - def is_within(self, other: Self) -> bool: - return isinstance(other, AllScope) or type(self) is type(other) and set(self.ids).issubset(other.ids) - @dataclass(frozen=True) class IDScopeLowerCase(Capability.Scope): @@ -302,9 +283,6 @@ def __post_init__(self) -> None: def as_tuples(self) -> set[tuple]: return {(self._scope_name, i) for i in self.ids} - def is_within(self, other: Self) -> bool: - return isinstance(other, AllScope) or type(self) is type(other) and set(self.ids).issubset(other.ids) - @dataclass(frozen=True) class ExtractionPipelineScope(Capability.Scope): @@ -317,9 +295,6 @@ def __post_init__(self) -> None: def as_tuples(self) -> set[tuple]: return {(self._scope_name, i) for i in self.ids} - def is_within(self, other: Self) -> bool: - return isinstance(other, AllScope) or type(self) is type(other) and set(self.ids).issubset(other.ids) - @dataclass(frozen=True) class DataSetScope(Capability.Scope): @@ -332,9 +307,6 @@ def __post_init__(self) -> None: def as_tuples(self) -> set[tuple]: return {(self._scope_name, i) for i in self.ids} - def is_within(self, other: Self) -> bool: - return isinstance(other, AllScope) or type(self) is type(other) and set(self.ids).issubset(other.ids) - @dataclass(frozen=True) class TableScope(Capability.Scope): @@ -361,19 +333,6 @@ def as_tuples(self) -> set[tuple]: # character, we represent this internally with the empty string: return {(self._scope_name, db, tbl) for db, tables in self.dbs_to_tables.items() for tbl in tables or [""]} - def is_within(self, other: Self) -> bool: - if isinstance(other, AllScope): - return True - if not isinstance(other, TableScope): - return False - - for db_name, tables in self.dbs_to_tables.items(): - if (other_tables := other.dbs_to_tables.get(db_name)) is None: - return False - if not set(tables).issubset(other_tables): - return False - return True - @dataclass(frozen=True) class AssetRootIDScope(Capability.Scope): @@ -386,9 +345,6 @@ def __post_init__(self) -> None: def as_tuples(self) -> set[tuple]: return {(self._scope_name, i) for i in self.root_ids} - def is_within(self, other: Self) -> bool: - return isinstance(other, AllScope) or type(self) is type(other) and set(self.root_ids).issubset(other.root_ids) - @dataclass(frozen=True) class ExperimentsScope(Capability.Scope): @@ -398,13 +354,6 @@ class ExperimentsScope(Capability.Scope): def as_tuples(self) -> set[tuple]: return {(self._scope_name, s) for s in self.experiments} - def is_within(self, other: Self) -> bool: - return ( - isinstance(other, AllScope) - or type(self) is type(other) - and set(self.experiments).issubset(other.experiments) - ) - @dataclass(frozen=True) class SpaceIDScope(Capability.Scope): @@ -414,11 +363,6 @@ class SpaceIDScope(Capability.Scope): def as_tuples(self) -> set[tuple]: return {(self._scope_name, s) for s in self.space_ids} - def is_within(self, other: Self) -> bool: - return ( - isinstance(other, AllScope) or type(self) is type(other) and set(self.space_ids).issubset(other.space_ids) - ) - @dataclass(frozen=True) class UnknownScope(Capability.Scope): @@ -436,9 +380,6 @@ def __getitem__(self, item: str) -> Any: def as_tuples(self) -> set[tuple]: raise NotImplementedError("Unknown scope cannot be converted to tuples (needed for comparisons)") - def is_within(self, other: Self) -> bool: - raise NotImplementedError("Unknown scope cannot be compared") - _SCOPE_CLASS_BY_NAME: MappingProxyType[str, type[Capability.Scope]] = MappingProxyType( {c._scope_name: c for c in Capability.Scope.__subclasses__() if not issubclass(c, UnknownScope)} From 5e269af38c851f35049d566d3501e4d3808fa71a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?H=C3=A5kon=20V=2E=20Treider?= Date: Fri, 17 Nov 2023 01:56:04 +0100 Subject: [PATCH 5/5] fix flaky test --- cognite/client/_api/iam.py | 2 +- tests/tests_unit/test_data_classes/test_capabilities.py | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/cognite/client/_api/iam.py b/cognite/client/_api/iam.py index 8a20c2a5af..0464a8a8e5 100644 --- a/cognite/client/_api/iam.py +++ b/cognite/client/_api/iam.py @@ -156,7 +156,7 @@ def compare_capabilities( for key, check_grp in to_check_lookup.items(): group = has_capabilties_lookup.get(key, set()) if any(AllScope._scope_name == grp[2] for grp in group): - continue # If allScope exists for capability, we safely skip ahead: + continue # If allScope exists for capability, we safely skip ahead elif RawAcl._capability_name == next(iter(check_grp))[0]: raw_group.update(group) raw_check_grp.update(check_grp) diff --git a/tests/tests_unit/test_data_classes/test_capabilities.py b/tests/tests_unit/test_data_classes/test_capabilities.py index e49dbcdd4f..0ffa49d46d 100644 --- a/tests/tests_unit/test_data_classes/test_capabilities.py +++ b/tests/tests_unit/test_data_classes/test_capabilities.py @@ -380,10 +380,9 @@ def test_raw_acl_database_scope(self, cognite_client, extra_existing): if extra_existing: assert missing == [RawAcl([RawAcl.Action.Write], RawAcl.Scope.Table({"db1": ["t1"]}))] else: - assert missing == [ - RawAcl([RawAcl.Action.Read], RawAcl.Scope.Table({"db1": ["t3"]})), - RawAcl([RawAcl.Action.Write], RawAcl.Scope.Table({"db1": ["t1"]})), - ] + assert len(missing) == 2 + assert RawAcl([RawAcl.Action.Read], RawAcl.Scope.Table({"db1": ["t3"]})) in missing + assert RawAcl([RawAcl.Action.Write], RawAcl.Scope.Table({"db1": ["t1"]})) in missing @pytest.mark.parametrize(