-
Notifications
You must be signed in to change notification settings - Fork 45
904 read permissions of entities within flex model or flex context #1071
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
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The 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:
- 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.
Choose a reason for hiding this comment
The 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 ctx_loader should be used, as no AuthContext is touched.
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 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.
Choose a reason for hiding this comment
The 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 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
nhoening
left a comment
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 see the improvements, this is good.
I disagree with one potential security problem in particular, like in the last review.
| 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 comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The 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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
It was because flex_model and flex_context could be None, as they are optional parameters. In that case, the context is None, too, and we should pass the permissions check: i.e. the fields contain no sensor references, so there is nothing to check. I still don't really understand why we shouldn't grant permission on a None context, but by now I did find an alternative fix, by setting load defaults for the flex_context and flex_model. So now they are no longer None, but empty dicts instead.
| 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 comment
The 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.
|
|
||
| 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 comment
The 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?
| 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.
Choose a reason for hiding this comment
The 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 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.
| @permission_required_for_context( | ||
| "read", | ||
| ctx_arg_name="flex_model", | ||
| ctx_loader=flex_model_loader, |
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"
…-permissions-of-entities-within-flex-model-or-flex-context
Signed-off-by: F.N. Claessen <[email protected]>
…so now permission_required_for_context still doesn't work with optional contexts Signed-off-by: F.N. Claessen <[email protected]>
Documentation build overview
Show files changed (7 files in total): 📝 7 modified | ➕ 0 added | ➖ 0 deleted
|
Signed-off-by: F.N. Claessen <[email protected]>
Signed-off-by: F.N. Claessen <[email protected]>
Closes #904.