Introduce decorators for all permission types - #5034
Draft
bruntib wants to merge 4 commits into
Draft
Conversation
Introduces a @requires_view decorator in api/common.py that wraps a Thrift API method's view-permission check into the method's decorator chain, so the permission requirement is visible in the method's signature rather than buried in the first line of the body. The decorator looks up the handler's existing __require_view() helper via getattr on the name-mangled attribute, so the helper does not need to be renamed and the change is purely additive in report_server.py. This commit applies the decorator to ThriftRequestHandler.getRunData as a design proof. A follow-up commit will migrate the remaining self.__require_view() call sites in report_server.py to use the decorator.
Migrates the remaining 42 Thrift API method bodies in ThriftRequestHandler from starting with a literal self.__require_view() call to using the @requires_view decorator introduced in the prior commit. The decorator inserts the permission check before the method body executes, so behavior is unchanged but the permission requirement is now visible at the method's signature. Performed mechanically: each call site was located, the enclosing def was identified, @requires_view was inserted above the def at matching indentation, and the call line was removed. Net diff is symmetric (42 insertions, 42 deletions). All 31 server unit tests still pass.
Adds a static check that every method declared in the Thrift service
codeCheckerDBAccess_v6 has a detectable permission check on the
implementing ThriftRequestHandler. A method is considered protected if
any of the following holds:
* It has the @requires_view decorator (introduced earlier in this
branch).
* One of its first few statements calls a self.__require_*() or
self._require_*() permission helper (admin, access, store,
permission, etc.).
* It is listed in _DELEGATED_PROTECTION, in which case the check
verifies that the named delegate helper itself satisfies one of
the above conditions. This handles methods such as massStoreRun
that pass the request through __massStoreRun_common, which in
turn calls self.__require_store().
The check is wired into two places, both using the same logic so they
cannot disagree:
* As a unit test (test_thrift_permission_coverage.py), so a PR that
introduces an unprotected method fails CI before being merged.
* As a startup assertion in start_server(), so even if such a PR
landed in master, the server would refuse to start instead of
serving an exploitable endpoint.
Thrift method names are discovered by introspecting the generated
Iface class in codechecker_api.codeCheckerDBAccess_v6, which is
present in both development and deployed installations -- no
dependency on the .thrift source files.
The __require_view helper on ThriftRequestHandler is renamed to
_require_view (single underscore). The @requires_view decorator now
calls self._require_view() directly. Pylint's unused-private-member
checker cannot see through the previous getattr indirection that
worked around Python's name mangling, so the original double-
underscore form was flagged as dead code despite being live.
In a previous commit the __requires_view() permission check has been implemented by a decorator function. In this commit __requires_store, __requires_access, __requires_admin are all transformed to decorators. Also, there is @requires_premission for custom permission levels. These decorators are used in report_server.py in a uniform way.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In a previous commit the __requires_view() permission check has been implemented by a decorator function.
In this commit __requires_store, __requires_access, __requires_admin are all transformed to decorators. Also, there is @requires_premission for custom permission levels.
These decorators are used in report_server.py in a uniform way.