From 2a7183bad6b412fe97f464679b2a49eb46b47eec Mon Sep 17 00:00:00 2001 From: Mark Benvenuto Date: Mon, 8 Mar 2021 12:03:40 -0500 Subject: SERVER-54521 Extend access_check for simple and privileges --- buildscripts/idl/idl/ast.py | 14 ++++ buildscripts/idl/idl/binder.py | 67 +++++++++++++++---- buildscripts/idl/idl/errors.py | 17 +++++ buildscripts/idl/idl/generator.py | 11 +++- buildscripts/idl/idl/parser.py | 32 +++++++-- buildscripts/idl/idl/syntax.py | 14 ++++ buildscripts/idl/tests/test_binder.py | 118 ++++++++++++++++++++++++++++++++++ buildscripts/idl/tests/test_parser.py | 59 +++++++++++++++-- src/mongo/db/auth/resource_pattern.h | 7 ++ src/mongo/idl/idl_test.cpp | 8 +++ src/mongo/idl/unittest.idl | 13 ++++ 11 files changed, 336 insertions(+), 24 deletions(-) diff --git a/buildscripts/idl/idl/ast.py b/buildscripts/idl/idl/ast.py index 915d04c206c..26238ed53d1 100644 --- a/buildscripts/idl/idl/ast.py +++ b/buildscripts/idl/idl/ast.py @@ -225,6 +225,19 @@ class Field(common.SourceLocation): super(Field, self).__init__(file_name, line, column) +class Privilege(common.SourceLocation): + """IDL privilege information.""" + + def __init__(self, file_name, line, column): + # type: (str, int, int) -> None + """Construct an Privilege.""" + + self.resource_pattern = None # type: str + self.action_type = None # type: List[str] + + super(Privilege, self).__init__(file_name, line, column) + + class AccessCheck(common.SourceLocation): """IDL commmand access check information.""" @@ -233,6 +246,7 @@ class AccessCheck(common.SourceLocation): """Construct an AccessCheck.""" self.check = None # type: str + self.privilege = None # type: Privilege super(AccessCheck, self).__init__(file_name, line, column) diff --git a/buildscripts/idl/idl/binder.py b/buildscripts/idl/idl/binder.py index 2dd71a45cf3..2f0de9b3382 100644 --- a/buildscripts/idl/idl/binder.py +++ b/buildscripts/idl/idl/binder.py @@ -557,35 +557,74 @@ def resolve_enum_value(ctxt, syntax_enum, name): if value.value == name: return value - ctxt.add_unknown_enum_value(syntax_enum, "AccessCheck", name) + ctxt.add_unknown_enum_value(syntax_enum, syntax_enum.name, name) return None -def _bind_single_check(ctxt, parsed_spec, access_check): - # type: (errors.ParserContext, syntax.IDLSpec, syntax.AccessCheck) -> ast.AccessCheck - """Bind a single access_check.""" - - ast_access_check = ast.AccessCheck(access_check.file_name, access_check.line, - access_check.column) +def _bind_enum_value(ctxt, parsed_spec, location, enum_name, enum_value): + # type: (errors.ParserContext, syntax.IDLSpec, common.SourceLocation, str, str) -> str - # Look up the enum for "AccessCheck" in the symbol table - access_check_enum = parsed_spec.symbols.resolve_type_from_name( - ctxt, access_check, "access_check.simple", "AccessCheck") + # Look up the enum for "enum_name" in the symbol table + access_check_enum = parsed_spec.symbols.resolve_type_from_name(ctxt, location, "access_check", + enum_name) if access_check_enum is None: # Resolution failed, we've recorded an error. return None if not isinstance(access_check_enum, syntax.Enum): - ctxt.add_unknown_type_error(access_check, "AccessCheck", "enum") + ctxt.add_unknown_type_error(location, enum_name, "enum") return None - enum_value = resolve_enum_value(ctxt, cast(syntax.Enum, access_check_enum), access_check.check) - if not enum_value: + syntax_enum = resolve_enum_value(ctxt, cast(syntax.Enum, access_check_enum), enum_value) + if not syntax_enum: return None - ast_access_check.check = enum_value.name + return syntax_enum.name + + +def _bind_single_check(ctxt, parsed_spec, access_check): + # type: (errors.ParserContext, syntax.IDLSpec, syntax.AccessCheck) -> ast.AccessCheck + """Bind a single access_check.""" + + ast_access_check = ast.AccessCheck(access_check.file_name, access_check.line, + access_check.column) + + assert bool(access_check.check) != bool(access_check.privilege) + + if access_check.check: + ast_access_check.check = _bind_enum_value(ctxt, parsed_spec, access_check, "AccessCheck", + access_check.check) + if not ast_access_check.check: + return None + else: + privilege = access_check.privilege + ast_privilege = ast.Privilege(privilege.file_name, privilege.line, privilege.column) + + ast_privilege.resource_pattern = _bind_enum_value(ctxt, parsed_spec, privilege, "MatchType", + privilege.resource_pattern) + if not ast_privilege.resource_pattern: + return None + + ast_privilege.action_type = [] + at_names = [] + for at in privilege.action_type: + at_names.append(at) + bound_at = _bind_enum_value(ctxt, parsed_spec, privilege, "ActionType", at) + if not bound_at: + return None + + ast_privilege.action_type.append(bound_at) + + at_names_set = set(at_names) + if len(at_names_set) != len(at_names): + for name in at_names_set: + if at_names.count(name) > 1: + ctxt.add_duplicate_action_types(ast_privilege, name) + return None + + ast_access_check.privilege = ast_privilege return ast_access_check diff --git a/buildscripts/idl/idl/errors.py b/buildscripts/idl/idl/errors.py index 2ac6565f04b..fb8023e6431 100644 --- a/buildscripts/idl/idl/errors.py +++ b/buildscripts/idl/idl/errors.py @@ -124,6 +124,8 @@ ERROR_ID_VARIANT_STRUCTS = "ID0081" ERROR_ID_NO_VARIANT_ENUM = "ID0082" ERROR_ID_COMMAND_DUPLICATES_NAME_AND_ALIAS = "ID0083" ERROR_ID_UNKOWN_ENUM_VALUE = "ID0084" +ERROR_ID_EITHER_CHECK_OR_PRIVILEGE = "ID0085" +ERROR_ID_DUPLICATE_ACTION_TYPE = "ID0086" class IDLError(Exception): @@ -923,6 +925,21 @@ class ParserContext(object): self._add_error(location, ERROR_ID_UNKOWN_ENUM_VALUE, "Cannot find enum value '%s' in enum '%s'." % (enum_value, enum_name)) + def add_either_check_or_privilege(self, location): + # type: (common.SourceLocation) -> None + """Add an error about specifing both a check and a privilege or neither.""" + # pylint: disable=invalid-name + self._add_error( + location, ERROR_ID_EITHER_CHECK_OR_PRIVILEGE, + "Must specify either a 'check' and a 'privilege' in an access_check, not both.") + + def add_duplicate_action_types(self, location, name): + # type: (common.SourceLocation, str) -> None + """Add an error about specifying an action type twice in the same list.""" + # pylint: disable=invalid-name + self._add_error(location, ERROR_ID_DUPLICATE_ACTION_TYPE, + "Cannot specify an action_type '%s' more then once" % (name)) + def _assert_unique_error_messages(): # type: () -> None diff --git a/buildscripts/idl/idl/generator.py b/buildscripts/idl/idl/generator.py index 0fc2962b089..ffa143909f0 100644 --- a/buildscripts/idl/idl/generator.py +++ b/buildscripts/idl/idl/generator.py @@ -2244,9 +2244,16 @@ class _CppSourceFileWriter(_CppFileWriterBase): checks = ",".join( [("AccessCheckEnum::" + ac.check) for ac in struct.access_checks if ac.check]) + privilege_list = [ac.privilege for ac in struct.access_checks if ac.privilege] + privileges = ",".join([ + "Privilege(ResourcePattern::forAuthorizationContract(MatchTypeEnum::%s), ActionSet{%s})" + % (p.resource_pattern, ",".join(["ActionType::" + at for at in p.action_type])) + for p in privilege_list + ]) + self._writer.write_line( - 'mongo::AuthorizationContract %s::kAuthorizationContract = AuthorizationContract(std::initializer_list{%s}, std::initializer_list{});' - % (common.title_case(struct.cpp_name), checks)) + 'mongo::AuthorizationContract %s::kAuthorizationContract = AuthorizationContract(std::initializer_list{%s}, std::initializer_list{%s});' + % (common.title_case(struct.cpp_name), checks, privileges)) self._writer.write_empty_line() diff --git a/buildscripts/idl/idl/parser.py b/buildscripts/idl/idl/parser.py index f3bd125154c..270feb16cdb 100644 --- a/buildscripts/idl/idl/parser.py +++ b/buildscripts/idl/idl/parser.py @@ -682,17 +682,41 @@ def _parse_enum(ctxt, spec, name, node): spec.symbols.add_enum(ctxt, idl_enum) +def _parse_privilege(ctxt, node): + # type: (errors.ParserContext, yaml.nodes.MappingNode) -> syntax.Privilege + """Parse a access check section in a struct in the IDL file.""" + + if not ctxt.is_mapping_node(node, "privilege"): + return None + + privilege = syntax.Privilege(ctxt.file_name, node.start_mark.line, node.start_mark.column) + + _generic_parser( + ctxt, node, "privilege", privilege, { + "resource_pattern": _RuleDesc('scalar', _RuleDesc.REQUIRED), + "action_type": _RuleDesc('scalar_or_sequence', _RuleDesc.REQUIRED), + }) + + return privilege + + def _parse_privilege_or_check(ctxt, node): # type: (errors.ParserContext, yaml.nodes.MappingNode) -> syntax.AccessCheck """Parse a access check section in a struct in the IDL file.""" access_check = syntax.AccessCheck(ctxt.file_name, node.start_mark.line, node.start_mark.column) - _generic_parser(ctxt, node, "privilege_or_check", access_check, { - "check": _RuleDesc('scalar'), - }) + _generic_parser( + ctxt, node, "privilege_or_check", access_check, { + "check": _RuleDesc('scalar'), + "privilege": _RuleDesc('mapping', mapping_parser_func=_parse_privilege), + }) - # TODO (SERVER-54521) - validate only one of check or privilege + if (access_check.check is None + and access_check.privilege is None) or (access_check.check is not None + and access_check.privilege is not None): + ctxt.add_either_check_or_privilege(access_check) + return None return access_check diff --git a/buildscripts/idl/idl/syntax.py b/buildscripts/idl/idl/syntax.py index 23d4975710c..4d10a3ef38e 100644 --- a/buildscripts/idl/idl/syntax.py +++ b/buildscripts/idl/idl/syntax.py @@ -527,6 +527,19 @@ class Struct(common.SourceLocation): super(Struct, self).__init__(file_name, line, column) +class Privilege(common.SourceLocation): + """IDL privilege information.""" + + def __init__(self, file_name, line, column): + # type: (str, int, int) -> None + """Construct an Privilege.""" + + self.resource_pattern = None # type: str + self.action_type = None # type: List[str] + + super(Privilege, self).__init__(file_name, line, column) + + class AccessCheck(common.SourceLocation): """IDL access check information.""" @@ -535,6 +548,7 @@ class AccessCheck(common.SourceLocation): """Construct an AccessCheck.""" self.check = None # type: str + self.privilege = None # type: Privilege super(AccessCheck, self).__init__(file_name, line, column) diff --git a/buildscripts/idl/tests/test_binder.py b/buildscripts/idl/tests/test_binder.py index cb3861898e7..48369e63a4f 100644 --- a/buildscripts/idl/tests/test_binder.py +++ b/buildscripts/idl/tests/test_binder.py @@ -2384,6 +2384,19 @@ class TestBinder(testcase.IDLTestcase): kIsAuthenticated : "is_authenticated" kIsCoAuthorized : "is_coauthorized" + ActionType: + description: "test" + type: string + values: + addShard : "addShard" + serverStatus : "serverStatus" + + MatchType: + description: "test" + type: string + values: + matchClusterResource : "cluster" + structs: reply: description: foo @@ -2422,6 +2435,24 @@ class TestBinder(testcase.IDLTestcase): reply_type: reply """)) + # Test simple with privilege + self.assert_bind(test_preamble + textwrap.dedent(""" + commands: + test1: + description: foo + command_name: foo + api_version: "" + namespace: ignored + access_check: + simple: + privilege: + resource_pattern: cluster + action_type: addShard + fields: + foo: string + reply_type: reply + """)) + def test_access_check_negative(self): # type: () -> None """Negative access check tests.""" @@ -2443,6 +2474,18 @@ class TestBinder(testcase.IDLTestcase): kIsAuthenticated : "is_authenticated" kIsCoAuthorized : "is_coauthorized" + ActionType: + description: "test" + type: string + values: + addShard : "addShard" + serverStatus : "serverStatus" + + MatchType: + description: "test" + type: string + values: + matchClusterResource : "cluster" structs: reply: description: foo @@ -2467,6 +2510,81 @@ class TestBinder(testcase.IDLTestcase): reply_type: reply """), idl.errors.ERROR_ID_UNKOWN_ENUM_VALUE) + # Test simple with bad access check with privilege + self.assert_bind_fail( + test_preamble + textwrap.dedent(""" + commands: + test1: + description: foo + command_name: foo + api_version: "" + namespace: ignored + access_check: + simple: + privilege: + resource_pattern: foo + action_type: addShard + fields: + foo: string + reply_type: reply + """), idl.errors.ERROR_ID_UNKOWN_ENUM_VALUE) + + # Test simple with bad access check with privilege + self.assert_bind_fail( + test_preamble + textwrap.dedent(""" + commands: + test1: + description: foo + command_name: foo + api_version: "" + namespace: ignored + access_check: + simple: + privilege: + resource_pattern: cluster + action_type: foo + fields: + foo: string + reply_type: reply + """), idl.errors.ERROR_ID_UNKOWN_ENUM_VALUE) + + # Test simple with access check and privileges + self.assert_bind(test_preamble + textwrap.dedent(""" + commands: + test1: + description: foo + command_name: foo + api_version: "" + namespace: ignored + access_check: + simple: + privilege: + resource_pattern: cluster + action_type: [addShard, serverStatus] + fields: + foo: string + reply_type: reply + """)) + + # Test simple with privilege with duplicate action_type + self.assert_bind_fail( + test_preamble + textwrap.dedent(""" + commands: + test1: + description: foo + command_name: foo + api_version: "" + namespace: ignored + access_check: + simple: + privilege: + resource_pattern: cluster + action_type: [addShard, addShard] + fields: + foo: string + reply_type: reply + """), idl.errors.ERROR_ID_DUPLICATE_ACTION_TYPE) + if __name__ == '__main__': diff --git a/buildscripts/idl/tests/test_parser.py b/buildscripts/idl/tests/test_parser.py index 4aa64613c8f..83db67bf5b2 100644 --- a/buildscripts/idl/tests/test_parser.py +++ b/buildscripts/idl/tests/test_parser.py @@ -1665,11 +1665,29 @@ class TestParser(testcase.IDLTestcase): reply_type: foo_reply_struct """)) + self.assert_parse( + textwrap.dedent(""" + commands: + foo: + description: foo + command_name: foo + api_version: 1 + namespace: ignored + access_check: + simple: + privilege: + resource_pattern: foo + action_type: foo + fields: + foo: bar + reply_type: foo_reply_struct + """)) + def test_access_checks_negative(self): # type: () -> None """Negative access_check test cases.""" - # check is not a sequence + # check and privilege are present self.assert_parse_fail( textwrap.dedent(""" commands: @@ -1680,14 +1698,47 @@ class TestParser(testcase.IDLTestcase): namespace: ignored access_check: simple: - check: - - one - - two + check: foo + privilege: + resource_pattern: foo + action_type: foo + fields: + foo: bar + reply_type: foo_reply_struct + """), idl.errors.ERROR_ID_EITHER_CHECK_OR_PRIVILEGE) + + # simple: true fails + self.assert_parse_fail( + textwrap.dedent(""" + commands: + foo: + description: foo + command_name: foo + api_version: 1 + namespace: ignored + access_check: + simple: true fields: foo: bar reply_type: foo_reply_struct """), idl.errors.ERROR_ID_IS_NODE_TYPE) + # simple empty fails + self.assert_parse_fail( + textwrap.dedent(""" + commands: + foo: + description: foo + command_name: foo + api_version: 1 + namespace: ignored + access_check: + simple: {} + fields: + foo: bar + reply_type: foo_reply_struct + """), idl.errors.ERROR_ID_EITHER_CHECK_OR_PRIVILEGE) + if __name__ == '__main__': diff --git a/src/mongo/db/auth/resource_pattern.h b/src/mongo/db/auth/resource_pattern.h index 6f51a768120..d82281d3625 100644 --- a/src/mongo/db/auth/resource_pattern.h +++ b/src/mongo/db/auth/resource_pattern.h @@ -189,6 +189,13 @@ public: return H::combine(std::move(h), rp._ns, rp._matchType); } + /** + * Returns a pattern for IDL generated code to use. + */ + static ResourcePattern forAuthorizationContract(MatchTypeEnum e) { + return ResourcePattern(e); + } + private: // AuthorizationContract works directly with MatchTypeEnum. Users should not be concerned with // how a ResourcePattern was constructed. diff --git a/src/mongo/idl/idl_test.cpp b/src/mongo/idl/idl_test.cpp index e4e40683edd..bc4780c43e4 100644 --- a/src/mongo/idl/idl_test.cpp +++ b/src/mongo/idl/idl_test.cpp @@ -37,6 +37,7 @@ #include "mongo/bson/bsontypes.h" #include "mongo/bson/oid.h" #include "mongo/db/auth/authorization_contract.h" +#include "mongo/db/auth/resource_pattern.h" #include "mongo/idl/unittest_gen.h" #include "mongo/rpc/op_msg.h" #include "mongo/unittest/bson_test_util.h" @@ -3685,6 +3686,13 @@ TEST(IDLAccessCheck, TestSimpleAccessCheck) { verifyContract(ac, AccessCheckSimpleAccessCheck::kAuthorizationContract); } +TEST(IDLAccessCheck, TestSimplePrivilegeAccessCheck) { + AuthorizationContract ac; + ac.addPrivilege(Privilege(ResourcePattern::forClusterResource(), ActionType::addShard)); + ac.addPrivilege(Privilege(ResourcePattern::forClusterResource(), ActionType::serverStatus)); + + verifyContract(ac, AccessCheckSimplePrivilege::kAuthorizationContract); +} } // namespace } // namespace mongo diff --git a/src/mongo/idl/unittest.idl b/src/mongo/idl/unittest.idl index eb4d10bf7dd..9238324b2a6 100644 --- a/src/mongo/idl/unittest.idl +++ b/src/mongo/idl/unittest.idl @@ -38,6 +38,7 @@ imports: - "mongo/idl/basic_types.idl" - "mongo/idl/unittest_import.idl" - "mongo/db/auth/access_checks.idl" + - "mongo/db/auth/action_type.idl" types: @@ -1052,6 +1053,18 @@ commands: check: is_authenticated reply_type: OkReply + AccessCheckSimplePrivilege: + description: A versioned API command with access_check and privilege + command_name: AccessCheckSimplePrivilegeCommandName + namespace: ignored + strict: true + api_version: "1" + access_check: + simple: + privilege: + resource_pattern: cluster + action_type: [addShard, serverStatus] + reply_type: OkReply # Test that we correctly generate C++ base classes for versioned API commands with different # key names, command names, and C++ names. -- cgit v1.2.1