Fix rawAcl database scope
#1514
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1514 +/- ##
==========================================
+ Coverage 91.56% 91.75% +0.18%
==========================================
Files 120 120
Lines 15611 15582 -29
==========================================
+ Hits 14294 14297 +3
+ Misses 1317 1285 -32
|
haakonvt
force-pushed
the
fix-raw-acl-db-scope
branch
from
290adbf to
5e269af
Compare
| @@ -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, | |||
There was a problem hiding this comment.
When would you want this set to True?
There was a problem hiding this comment.
That is the most logical to me. You have access to all data sets when you have all, which would be the most common use case.
There was a problem hiding this comment.
If you want two groups of capabilities to be equal, and they differ in scopes "not all" - but you still have some all-scopes here and there, then this option will tell you exactly what those differences are. For Peter's bootstrap CLI (and maybe others...) it means the manual work of removing any possible all-scopes or raw-database-scopes before comparing is not needed.
| @@ -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: | |||
There was a problem hiding this comment.
Surprised that these slipped through the reviews.
There was a problem hiding this comment.
I actually kept it as a "simple 1:1 check", but decided now with the raw-database changes to not duplicate the error-prone scope logic.
| [RawAcl(actions=[RawAcl.Action.Read], scope=TableScope({"db1": []}))], | ||
| [RawAcl(actions=[RawAcl.Action.Read], scope=TableScope({"db1": {"tables": []}}))], |
There was a problem hiding this comment.
These are confusing, but if I understand it correctly, it means all in db1? Both of them are equal?
There was a problem hiding this comment.
This means access is granted to all current and all future tables in the database "db1":
TableScope({"db1": []})
I don't like the syntax one bit.
There was a problem hiding this comment.
However, that's how the API does it, and hence we shouldn't add our own DataBaseScope imo
Description
[7.2.1] - 2023-11-17
Fixed
IAMAPI.verify_capabilitiesandIAMAPI.compare_capabilitiesnow works correctly for rawAcl with database scope ("all tables").Removed
is_withinmethod, and capabilities no longer havehas_capability. Use the more generalIAMAPI.compare_capabilitiesinstead.Checklist:
If a new method has been added it should be referenced in cognite.rst in order to generate docs based on its docstring.