Skip to content

Commit 81d2106

Browse files
dbolducdbolduc
authored
cleanup(generator): stricter IAM checks (#10965)
1 parent a7baf7f commit 81d2106

2 files changed

Lines changed: 31 additions & 11 deletions

File tree

‎generator/internal/descriptor_utils.cc‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -428,12 +428,20 @@ void SetHttpGetQueryParameters(
428428
absl::visit(HttpInfoVisitor(method, method_vars), parsed_http_info);
429429
}
430430

431+
bool IsKnownIdempotentMethod(google::protobuf::MethodDescriptor const& m) {
432+
return (m.name() == "GetIamPolicy" &&
433+
m.output_type()->full_name() == "google.iam.v1.Policy" &&
434+
m.input_type()->full_name() == "google.iam.v1.GetIamPolicyRequest") ||
435+
(m.name() == "TestIamPermissions" &&
436+
m.output_type()->full_name() ==
437+
"google.iam.v1.TestIamPermissionsResponse" &&
438+
m.input_type()->full_name() ==
439+
"google.iam.v1.TestIamPermissionsRequest");
440+
}
441+
431442
std::string DefaultIdempotencyFromHttpOperation(
432443
google::protobuf::MethodDescriptor const& method) {
433-
if (method.name() == "GetIamPolicy" ||
434-
method.name() == "TestIamPermissions") {
435-
return "kIdempotent";
436-
}
444+
if (IsKnownIdempotentMethod(method)) return "kIdempotent";
437445
if (method.options().HasExtension(google::api::http)) {
438446
google::api::HttpRule http_rule =
439447
method.options().GetExtension(google::api::http);

‎generator/internal/descriptor_utils_test.cc‎

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,14 @@ INSTANTIATE_TEST_SUITE_P(
335335
return std::get<0>(info.param);
336336
});
337337

338+
char const* const kIamProto =
339+
"syntax = \"proto3\";\n"
340+
"package google.iam.v1;\n"
341+
"message Policy {}\n"
342+
"message GetIamPolicyRequest {}\n"
343+
"message TestIamPermissionsRequest {}\n"
344+
"message TestIamPermissionsResponse {}\n";
345+
338346
char const* const kClientProto =
339347
"syntax = \"proto3\";\n"
340348
"package google.api;\n"
@@ -390,6 +398,7 @@ char const* const kServiceProto =
390398
"import \"google/api/annotations.proto\";\n"
391399
"import \"google/api/client.proto\";\n"
392400
"import \"google/api/http.proto\";\n"
401+
"import \"google/iam/v1/fake_iam.proto\";\n"
393402
"import \"google/longrunning/operation.proto\";\n"
394403
"// Leading comments about message Foo.\n"
395404
"message Foo {\n"
@@ -530,14 +539,16 @@ char const* const kServiceProto =
530539
" option (google.api.method_signature) = \"name,title,number\";\n"
531540
" }\n"
532541
" // Test that the method defaults to kIdempotent.\n"
533-
" rpc GetIamPolicy(Empty) returns (Empty) {\n"
542+
" rpc GetIamPolicy(google.iam.v1.GetIamPolicyRequest)\n"
543+
" returns (google.iam.v1.Policy) {\n"
534544
" option (google.api.http) = {\n"
535545
" post: \"/v1/foo\"\n"
536546
" body: \"*\"\n"
537547
" };\n"
538548
" }\n"
539549
" // Test that the method defaults to kIdempotent.\n"
540-
" rpc TestIamPermissions(Empty) returns (Empty) {\n"
550+
" rpc TestIamPermissions(google.iam.v1.TestIamPermissionsRequest)\n"
551+
" returns (google.iam.v1.TestIamPermissionsResponse) {\n"
541552
" option (google.api.http) = {\n"
542553
" post: \"/v1/foo\"\n"
543554
" body: \"*\"\n"
@@ -568,6 +579,7 @@ class CreateMethodVarsTest
568579
{std::string("google/api/client.proto"), kClientProto},
569580
{std::string("google/api/http.proto"), kHttpProto},
570581
{std::string("google/api/annotations.proto"), kAnnotationsProto},
582+
{std::string("google/iam/v1/fake_iam.proto"), kIamProto},
571583
{std::string("google/longrunning/operation.proto"),
572584
kLongrunningOperationsProto},
573585
{std::string("test/test.proto"), kSourceLocationTestInput},
@@ -614,7 +626,7 @@ TEST_F(CreateMethodVarsTest, FormatMethodCommentsProtobufRequest) {
614626
HasSubstr(R"""( ///
615627
/// Leading comments about rpc Method0$$.
616628
///
617-
/// @param request @googleapis_link{google::protobuf::Bar,google/foo/v1/service.proto#L16}
629+
/// @param request @googleapis_link{google::protobuf::Bar,google/foo/v1/service.proto#L17}
618630
/// @param opts Optional. Override the class-level options, such as retry and
619631
/// backoff policies.
620632
///
@@ -769,7 +781,7 @@ INSTANTIATE_TEST_SUITE_P(
769781
MethodVarsTestValues("google.protobuf.Service.Method0",
770782
"method_return_doxygen_link",
771783
"@googleapis_link{google::protobuf::Empty,google/"
772-
"foo/v1/service.proto#L31}"),
784+
"foo/v1/service.proto#L32}"),
773785
MethodVarsTestValues("google.protobuf.Service.Method0",
774786
"method_http_query_parameters",
775787
"google.protobuf.Service.Method0"),
@@ -785,7 +797,7 @@ INSTANTIATE_TEST_SUITE_P(
785797
MethodVarsTestValues("google.protobuf.Service.Method1",
786798
"method_return_doxygen_link",
787799
"@googleapis_link{google::protobuf::Bar,google/"
788-
"foo/v1/service.proto#L16}"),
800+
"foo/v1/service.proto#L17}"),
789801
MethodVarsTestValues("google.protobuf.Service.Method1",
790802
"method_http_query_parameters", ""),
791803
// Method2
@@ -816,7 +828,7 @@ INSTANTIATE_TEST_SUITE_P(
816828
MethodVarsTestValues("google.protobuf.Service.Method2",
817829
"method_longrunning_deduced_return_doxygen_link",
818830
"@googleapis_link{google::protobuf::Bar,google/"
819-
"foo/v1/service.proto#L16}"),
831+
"foo/v1/service.proto#L17}"),
820832
MethodVarsTestValues("google.protobuf.Service.Method2",
821833
"method_http_query_parameters", ""),
822834
// Method3
@@ -944,7 +956,7 @@ INSTANTIATE_TEST_SUITE_P(
944956
MethodVarsTestValues("google.protobuf.Service.Method7",
945957
"method_longrunning_deduced_return_doxygen_link",
946958
"@googleapis_link{google::protobuf::Bar,google/"
947-
"foo/v1/service.proto#L16}"),
959+
"foo/v1/service.proto#L17}"),
948960
// Method8
949961
MethodVarsTestValues("google.protobuf.Service.Method8",
950962
"method_signature0",

0 commit comments

Comments
 (0)