904 read permissions of entities within flex model or flex context #1071
Conversation
Signed-off-by: F.N. Claessen <[email protected]>
Signed-off-by: F.N. Claessen <[email protected]>
Signed-off-by: F.N. Claessen <[email protected]>
…ntext Signed-off-by: F.N. Claessen <[email protected]>
Signed-off-by: F.N. Claessen <[email protected]>
Signed-off-by: F.N. Claessen <[email protected]>
Signed-off-by: F.N. Claessen <[email protected]>
Signed-off-by: F.N. Claessen <[email protected]>
Signed-off-by: F.N. Claessen <[email protected]>
Signed-off-by: F.N. Claessen <[email protected]>
Signed-off-by: F.N. Claessen <[email protected]>
Signed-off-by: F.N. Claessen <[email protected]>
Signed-off-by: F.N. Claessen <[email protected]>
Signed-off-by: F.N. Claessen <[email protected]>
There was a problem hiding this comment.
@nhoening I need advice on where to move these new util functions, for example, to:
- api/common/utils/args_parsing.py
- auth/loaders.py
- or elsewhere, maybe even becoming a new acl method on some class? I'm out of my league here.
I saw other ctx_loaders were pointing to various things, such as:
ctx_loader=lambda bdf: bdf.sensorctx_loader=GenericAssetctx_loader=AccountIdField.load_currentctx_loader=AuditLog.account_table_aclctx_loader=AuditLog.user_table_acl
There was a problem hiding this comment.
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 ctx_loader should be used, as no AuthContext is touched.
There was a problem hiding this comment.
I don't see why ctx_loader should be used, as no AuthContext is touched.
I'm not sure what to do with this. Am I using ctx_loader incorrectly?
Signed-off-by: F.N. Claessen <[email protected]>
Signed-off-by: F.N. Claessen <[email protected]>
Signed-off-by: F.N. Claessen <[email protected]>
There was a problem hiding this comment.
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 ctx_loader should be used, as no AuthContext is touched.
…-permissions-of-entities-within-flex-model-or-flex-context
Signed-off-by: F.N. Claessen <[email protected]>
…nly. Signed-off-by: F.N. Claessen <[email protected]>
Signed-off-by: F.N. Claessen <[email protected]>
Signed-off-by: F.N. Claessen <[email protected]>
…uments only." This reverts commit b2ec43a.
…ontext loaders loads a None Signed-off-by: F.N. Claessen <[email protected]>
Signed-off-by: F.N. Claessen <[email protected]>
…-model-or-flex-context
…-model-or-flex-context
| 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.
I (still) believe this should be a LookupError. Above, you skip the auth check if this looking up of a context by argument name did not work out (which I also am in favor of not doing). This could quite easily happen by programmer mistake, and could easily be overlooked during testing.
ctx_arg_name is an optional attribute, but if it is given, we need to actually use it.
There was a problem hiding this comment.
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 permission_required_for_context telling the argument is optional (i.e. skip if context is None) or required (i.e. raise if context is None)?
There was a problem hiding this comment.
Can we not check that context_from_args has to be something if the developer gave information to look it up?
I.e. if ctx_arg_name is not None, then we expect that context_from_args is also not None, otherwise we got wrong parameterisation.
Simply put, if permission_required_for_context() has no context, we cannot say the permission is granted.
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 permission_required_for_context() that says pass_if_no_context_found or similar.
But the interpretation has to fit.
| 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.
Let's mention for ease of reading that the origin is just a string.
|
|
||
| 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.
Is this return type still valid? It can be a list or tuples with origins, right?
| context = context_from_args | ||
| if context is None: | ||
| raise LookupError(f"No context could be loaded from {context_from_args}.") | ||
| return context |
There was a problem hiding this comment.
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 AuthModelMixin and throw a meaningful LookupError if they are not?
I just checked, and check_access does this check. So we can not do it here. But it could help debugging problems.
| flex_context_loader = partial(sensor_loader, parent_key="flex-context") | ||
|
|
||
|
|
||
| def find_sensor_ids(data, parent_key="") -> list[tuple[int, str]]: |
| @permission_required_for_context( | ||
| "read", | ||
| ctx_arg_name="flex_model", | ||
| ctx_loader=flex_model_loader, |
There was a problem hiding this comment.
I'd add a comment: "This extracts sensors from the flex model parameter - user needs read access to them"
Closes #904.