-
Notifications
You must be signed in to change notification settings - Fork 48
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?
Changes from all commits
6ecb17f
2546546
c3a5920
482dd0a
cc7248b
0b41e77
e616d1c
019d772
aa27d79
6ea9202
f72ab14
edfa646
16684e6
7a12c49
c7938fa
8733408
38d8241
075db1a
97aa1d7
b2ec43a
d7862fc
07b864c
b07d76f
ff73a4c
110d027
54c87d4
0873183
4740e7a
934ac5f
a53db55
3e78412
70e3615
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -110,6 +110,7 @@ def permission_required_for_context( | |
| ctx_arg_name: str | None = None, | ||
| ctx_loader: Callable | None = None, | ||
| pass_ctx_to_loader: bool = False, | ||
| error_handler: Callable | None = None, | ||
| ): | ||
| """ | ||
| This decorator can be used to make sure that the current user has the necessary permission to access the context. | ||
|
|
@@ -118,6 +119,7 @@ def permission_required_for_context( | |
|
|
||
| A 403 response is raised if there is no principal for the required permission. | ||
| A 401 response is raised if the user is not authenticated at all. | ||
| A custom response can be generated by passing an error_handler, which should be a function that accepts the context, permission and a context origin. | ||
|
|
||
| We will now explain how to load a context, and give an example: | ||
|
|
||
|
|
@@ -144,7 +146,9 @@ def view(resource_id: int, the_resource: Resource): | |
|
|
||
| The ctx_loader: | ||
|
|
||
| The ctx_loader can be a function without arguments or it takes the context loaded from the arguments as input (using pass_ctx_to_loader=True). | ||
| The ctx_loader can be a function without arguments, or it takes the context loaded from the arguments as input (using pass_ctx_to_loader=True). | ||
| It should return the context, a list of contexts, or a list of (context, origin) tuples, | ||
| where an origin defines where (e.g. what API field) the context came from, to help with responding with more informative errors. | ||
| A special case is useful when the arguments contain the context ID (not the instance). | ||
| Then, the loader can be a subclass of AuthModelMixin, and this decorator will look up the instance. | ||
|
|
||
|
|
@@ -167,38 +171,64 @@ def post(self, resource_data: dict): | |
| def wrapper(fn): | ||
| @wraps(fn) | ||
| def decorated_view(*args, **kwargs): | ||
| # load & check context | ||
| context: AuthModelMixin = None | ||
|
|
||
| # first set context_from_args, if possible | ||
| context_from_args: AuthModelMixin = None | ||
| if ctx_arg_pos is not None and ctx_arg_name is not None: | ||
| context_from_args = args[ctx_arg_pos][ctx_arg_name] | ||
| elif ctx_arg_pos is not None: | ||
| context_from_args = args[ctx_arg_pos] | ||
| elif ctx_arg_name is not None: | ||
| context_from_args = kwargs[ctx_arg_name] | ||
| elif len(args) > 0: | ||
| context_from_args = args[0] | ||
|
|
||
| # if a loader is given, use that, otherwise fall back to context_from_args | ||
| if ctx_loader is not None: | ||
| if pass_ctx_to_loader: | ||
| if inspect.isclass(ctx_loader) and issubclass( | ||
| ctx_loader, AuthModelMixin | ||
| ): | ||
| context = db.session.get(ctx_loader, context_from_args) | ||
| else: | ||
| context = ctx_loader(context_from_args) | ||
| else: | ||
| context = ctx_loader() | ||
| else: | ||
| context = context_from_args | ||
| context = load_context( | ||
| ctx_arg_pos, ctx_arg_name, ctx_loader, pass_ctx_to_loader, args, kwargs | ||
| ) | ||
|
|
||
| check_access(context, permission) | ||
| # Check access for possibly multiple contexts | ||
| if not isinstance(context, list): | ||
| context = [context] | ||
| for ctx in context: | ||
| if isinstance(ctx, tuple): | ||
| c = ctx[0] # c[0] is the context, c[1] is its origin | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| # the context loader may narrow down the origin of the context (e.g. a nested field rather than a function argument) | ||
| origin = ctx[1] | ||
| else: | ||
| c = ctx | ||
| origin = ctx_arg_name | ||
| try: | ||
| check_access(c, permission) | ||
| except Exception as e: # noqa: B902 | ||
| if error_handler: | ||
| return error_handler(c, permission, origin) | ||
| raise e | ||
|
|
||
| return fn(*args, **kwargs) | ||
|
|
||
| return decorated_view | ||
|
|
||
| return wrapper | ||
|
|
||
|
|
||
| def load_context( | ||
| ctx_arg_pos, ctx_arg_name, ctx_loader, pass_ctx_to_loader, args, kwargs | ||
| ) -> AuthModelMixin | None: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
|
|
||
| # first set context_from_args, if possible | ||
| context_from_args: AuthModelMixin | None = None | ||
| if ctx_arg_pos is not None and ctx_arg_name is not None: | ||
| context_from_args = args[ctx_arg_pos][ctx_arg_name] | ||
| elif ctx_arg_pos is not None: | ||
| context_from_args = args[ctx_arg_pos] | ||
| elif ctx_arg_name is not None: | ||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I (still) believe this should be a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we not check that I.e. if Simply put, if 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 But the interpretation has to fit.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| elif len(args) > 0: | ||
| context_from_args = args[0] | ||
|
|
||
| # if a loader is given, use that, otherwise fall back to context_from_args | ||
| if ctx_loader is not None: | ||
| if pass_ctx_to_loader: | ||
| if inspect.isclass(ctx_loader) and issubclass(ctx_loader, AuthModelMixin): | ||
| context = db.session.get(ctx_loader, context_from_args) | ||
| else: | ||
| context = ctx_loader(context_from_args) | ||
| else: | ||
| context = ctx_loader() | ||
| else: | ||
| context = context_from_args | ||
| if context is None: | ||
| raise LookupError(f"No context could be loaded from {context_from_args}.") | ||
| return context | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I just checked, and |
||
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"