Skip to content

Commit db48756

Browse files
msorensmsorens
authored
[Automate-2942] Add missing "required" field constraints (#3197)
* Make proto fields required These interservice protos now match the documented required fields in the gateway protos. Documentation for options is at https://github.com/envoyproxy/protoc-gen-validate Signed-off-by: michael sorens <[email protected]> * Standardize required string check with min_len Signed-off-by: michael sorens <[email protected]> * PR bonus Fix error message Signed-off-by: michael sorens <[email protected]> * Revert checks for enum The gateway has a check for this that occurs first, and even with grpcurl there seems to be a built-in check, so this is not needed. Signed-off-by: michael sorens <[email protected]> * Test fixes Signed-off-by: michael sorens <[email protected]> * Revert "Standardize required string check with min_len" This reverts commit 9f33b09. * Change empty string detection to non-whitespace That way we are also disallowing just whitespace entries Signed-off-by: michael sorens <[email protected]> * Revert "Change empty string detection to non-whitespace" This reverts commit 13bfc26. Reason: ugly message on the command line with `\\S`: "error": "invalid CreateTeamReq.Name: value does not match regex pattern \"\\\\S\"", * Revert "Revert "Standardize required string check with min_len"" This reverts commit 59406ef. * Regenerate from protos Signed-off-by: michael sorens <[email protected]> * Test correction Moving whitespace check into service itself due to ugly error at proto level: "error": "invalid CreateTeamReq.Name: value does not match regex pattern \"\\\\S\"", Signed-off-by: michael sorens <[email protected]> * Add field checking consistency After gateway proto-level validation, some errors (notably an all-whitespace value) are now handled more consitently Signed-off-by: michael sorens <[email protected]> * Fix integration test Name is now required on an update Signed-off-by: michael sorens <[email protected]> * Revert min_len checks Want the services to handle this themselves for a better error message. Instead of: value length must be at least 1 runes will get this: a team name is required and must contain at least one non-whitespace character" Signed-off-by: michael sorens <[email protected]> * Regenerate from protos Signed-off-by: michael sorens <[email protected]> * Complete validate*Input for authz, authn, teams This actually covers: tokens, policies, projects, roles, rules, and teams. Signed-off-by: michael sorens <[email protected]> * Standardize local user service Signed-off-by: michael sorens <[email protected]> * Regenerate from user protos Signed-off-by: michael sorens <[email protected]> * Fix unit test Signed-off-by: michael sorens <[email protected]> * Correct error codes Set explicitly to InvalidArgument instead of default InternalError. (Authz calls already include InvalidArgument wrapper.) Signed-off-by: michael sorens <[email protected]>
1 parent c34c7f0 commit db48756

32 files changed

+574
-591
lines changed

‎api/interservice/authn/auth_test.go

+50-33
Original file line numberDiff line numberDiff line change
@@ -16,73 +16,90 @@ func TestGeneratedProtobufUpToDate(t *testing.T) {
1616
func TestValidationCreateTokenID(t *testing.T) {
1717
negativeCases := map[string]*authn.CreateTokenReq{
1818
"with uppercase characters": &authn.CreateTokenReq{
19-
Id: "TestID",
20-
Projects: []string{"project1", "project2"},
19+
Id: "TestID",
20+
Description: "description",
21+
Projects: []string{"project1", "project2"},
2122
},
2223
"with non-dash characters": &authn.CreateTokenReq{
23-
Id: "test#space",
24-
Projects: []string{"project1", "project2"},
24+
Id: "test#space",
25+
Description: "description",
26+
Projects: []string{"project1", "project2"},
2527
},
2628
"with spaces": &authn.CreateTokenReq{
27-
Id: "test space",
28-
Projects: []string{"project1", "project2"},
29+
Id: "test space",
30+
Description: "description",
31+
Projects: []string{"project1", "project2"},
2932
},
3033
"whitespace projects list": &authn.CreateTokenReq{
31-
Id: "valid-id",
32-
Projects: []string{" ", "test"},
34+
Id: "valid-id",
35+
Description: "description",
36+
Projects: []string{" ", "test"},
3337
},
3438
"repeated projects in list": &authn.CreateTokenReq{
35-
Id: "valid-id",
36-
Projects: []string{"repeat", "repeat"},
39+
Id: "valid-id",
40+
Description: "description",
41+
Projects: []string{"repeat", "repeat"},
3742
},
3843
"project has invalid characters": &authn.CreateTokenReq{
39-
Id: "valid-id",
40-
Projects: []string{"valid", "wrong~"},
44+
Id: "valid-id",
45+
Description: "description",
46+
Projects: []string{"valid", "wrong~"},
4147
},
4248
"project has spaces": &authn.CreateTokenReq{
43-
Id: "valid-id",
44-
Projects: []string{"valid", "wrong space"},
49+
Id: "valid-id",
50+
Description: "description",
51+
Projects: []string{"valid", "wrong space"},
4552
},
4653
"project is too long": &authn.CreateTokenReq{
47-
Id: "valid-id",
48-
Projects: []string{"much-too-long-longest-word-in-english-pneumonoultramicroscopicsilicovolcanoconiosis", "valid"},
54+
Id: "valid-id",
55+
Description: "description",
56+
Projects: []string{"much-too-long-longest-word-in-english-pneumonoultramicroscopicsilicovolcanoconiosis", "valid"},
4957
},
5058
}
5159
positiveCases := map[string]*authn.CreateTokenReq{
5260
"projects are missing": &authn.CreateTokenReq{
53-
Id: "valid-id",
61+
Id: "valid-id",
62+
Description: "description",
5463
},
5564
"underscore in IDs": &authn.CreateTokenReq{
56-
Id: "valid_id",
57-
Projects: []string{"project_1", "project_2"},
65+
Id: "valid_id",
66+
Description: "description",
67+
Projects: []string{"project_1", "project_2"},
5868
},
5969
"with no characters": &authn.CreateTokenReq{
60-
Id: "",
61-
Projects: []string{"project1", "project2"},
70+
Id: "",
71+
Description: "description",
72+
Projects: []string{"project1", "project2"},
6273
},
6374
"with ID all lowercase": &authn.CreateTokenReq{
64-
Id: "test",
65-
Projects: []string{"project1", "project2"},
75+
Id: "test",
76+
Description: "description",
77+
Projects: []string{"project1", "project2"},
6678
},
6779
"with ID that has dashes": &authn.CreateTokenReq{
68-
Id: "-test-with-dashes-",
69-
Projects: []string{"project1", "project2"},
80+
Id: "-test-with-dashes-",
81+
Description: "description",
82+
Projects: []string{"project1", "project2"},
7083
},
7184
"with ID that has dashes and numbers": &authn.CreateTokenReq{
72-
Id: "1-test-with-1-and-dashes-0",
73-
Projects: []string{"project1", "project2"},
85+
Id: "1-test-with-1-and-dashes-0",
86+
Description: "description",
87+
Projects: []string{"project1", "project2"},
7488
},
7589
"with ID that has only numbers": &authn.CreateTokenReq{
76-
Id: "1235",
77-
Projects: []string{"project1", "project2"},
90+
Id: "1235",
91+
Description: "description",
92+
Projects: []string{"project1", "project2"},
7893
},
7994
"with a single project": &authn.CreateTokenReq{
80-
Id: "1235",
81-
Projects: []string{"project1"},
95+
Id: "1235",
96+
Description: "description",
97+
Projects: []string{"project1"},
8298
},
8399
"projects are empty": &authn.CreateTokenReq{
84-
Id: "valid-id",
85-
Projects: []string{},
100+
Id: "valid-id",
101+
Description: "description",
102+
Projects: []string{},
86103
},
87104
}
88105

‎api/interservice/authz/v2/policy.pb.go

+103-103
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎api/interservice/authz/v2/policy.pb.validate.go

+25-16
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎api/interservice/authz/v2/policy.proto

+9-8
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ message Role {
2525

2626
message CreatePolicyReq {
2727
string id = 1 [(validate.rules).string.pattern = "^[a-z0-9-_]{1,64}$"];
28-
string name = 2 [(validate.rules).string.min_len = 1];
28+
string name = 2;
2929
repeated string members = 3 [(validate.rules).repeated = {
3030
unique: true,
3131
items: {
@@ -133,6 +133,7 @@ message ReplacePolicyMembersResp {
133133
message AddPolicyMembersReq {
134134
string id = 1 [(validate.rules).string.pattern = "^[a-z0-9-_]{1,64}$"];
135135
repeated string members = 2 [(validate.rules).repeated = {
136+
min_items: 1,
136137
unique: true,
137138
items: {
138139
string: {
@@ -168,16 +169,16 @@ message ListRolesResp {
168169
}
169170

170171
message DeleteRoleReq {
171-
string id = 1
172-
[(validate.rules).string.pattern = "^[a-z0-9-_]{1,64}$"];
172+
string id = 1 [(validate.rules).string.pattern = "^[a-z0-9-_]{1,64}$"];
173173
}
174174
message DeleteRoleResp {}
175175

176176
message UpdateRoleReq {
177177
string id = 1 [(validate.rules).string.pattern = "^[a-z0-9-_]{1,64}$"];
178-
string name = 2 [(validate.rules).string.pattern = "\\S"];
178+
string name = 2;
179179
repeated string actions = 3
180180
[(validate.rules).repeated = {
181+
min_items: 1,
181182
unique: true,
182183
// this RE means: * OR *:verb OR svc:type:verb OR svc:* OR svc:*:verb OR svc:type:*
183184
items: {
@@ -206,6 +207,7 @@ message ListPolicyMembersResp {
206207
message RemovePolicyMembersReq {
207208
string id = 1 [(validate.rules).string.pattern = "^[a-z0-9-_]{1,64}$"];
208209
repeated string members = 2 [(validate.rules).repeated = {
210+
min_items: 1,
209211
unique: true,
210212
items: {
211213
string: {
@@ -239,12 +241,11 @@ message GetRoleReq {
239241
}
240242

241243
message CreateRoleReq {
242-
string id = 1
243-
[(validate.rules).string.pattern = "^[a-z0-9-_]{1,64}$"];
244-
string name = 2
245-
[(validate.rules).string.pattern = "\\S"];;
244+
string id = 1 [(validate.rules).string.pattern = "^[a-z0-9-_]{1,64}$"];
245+
string name = 2;
246246
repeated string actions = 3
247247
[(validate.rules).repeated = {
248+
min_items: 1,
248249
unique: true,
249250
// this RE means: * OR *:verb OR svc:type:verb OR svc:* OR svc:*:verb OR svc:type:*
250251
items: {

‎api/interservice/authz/v2/policy_test.go

+16-16
Original file line numberDiff line numberDiff line change
@@ -650,69 +650,66 @@ func TestValidationCreateRole(t *testing.T) {
650650
"empty ID": &v2.CreateRoleReq{
651651
Id: "",
652652
Name: "name of my team",
653+
Actions: []string{"cfgmgmt:nodes:*"},
653654
Projects: []string{"test"},
654655
},
655656
"invalid ID": &v2.CreateRoleReq{
656657
Id: "i d !",
657658
Name: "name of my team",
659+
Actions: []string{"cfgmgmt:nodes:*"},
658660
Projects: []string{"test"},
659661
},
660662
"whitespace ID": &v2.CreateRoleReq{
661663
Id: " ",
662664
Name: "name of my team",
665+
Actions: []string{"cfgmgmt:nodes:*"},
663666
Projects: []string{"test"},
664667
},
665668
"missing ID": &v2.CreateRoleReq{
666669
Name: "name of my team",
667-
Projects: []string{"test"},
668-
},
669-
"empty Name": &v2.CreateRoleReq{
670-
Id: "test",
671-
Name: "",
672-
Projects: []string{"test"},
673-
},
674-
"whitespace Name": &v2.CreateRoleReq{
675-
Id: "test",
676-
Name: " ",
677-
Projects: []string{"test"},
678-
},
679-
"missing Name": &v2.CreateRoleReq{
680-
Id: "test",
670+
Actions: []string{"cfgmgmt:nodes:*"},
681671
Projects: []string{"test"},
682672
},
683673
"ID contains invalid characters": &v2.CreateRoleReq{
684674
Id: "invalid~~~",
685675
Name: "this is valid ~ fun characters",
676+
Actions: []string{"cfgmgmt:nodes:*"},
686677
Projects: []string{"test", "test-2"},
687678
},
688679
"ID contains whitespace": &v2.CreateRoleReq{
689680
Id: "invalid id",
690681
Name: "this is valid ~ fun characters",
682+
Actions: []string{"cfgmgmt:nodes:*"},
691683
Projects: []string{"test", "test-2"},
692684
},
693685
"whitespace projects list": &v2.CreateRoleReq{
694686
Id: "this-is-valid-1",
695687
Name: "name of my team ~ fun characters 1 %",
688+
Actions: []string{"cfgmgmt:nodes:*"},
696689
Projects: []string{" ", "test"},
697690
},
698691
"repeated projects in list": &v2.CreateRoleReq{
699692
Id: "this-is-valid-1",
700693
Name: "name of my team ~ fun characters 1 %",
694+
Actions: []string{"cfgmgmt:nodes:*"},
701695
Projects: []string{"repeat", "repeat"},
702696
},
703697
"Project has invalid characters": &v2.CreateRoleReq{
704698
Id: "this-is-valid-1",
705699
Name: "name of my team ~ fun characters 1 %",
700+
Actions: []string{"cfgmgmt:nodes:*"},
706701
Projects: []string{"valid", "wrong~"},
707702
},
708703
"Project has spaces": &v2.CreateRoleReq{
709704
Id: "this-is-valid-1",
710705
Name: "name of my team ~ fun characters 1 %",
706+
Actions: []string{"cfgmgmt:nodes:*"},
711707
Projects: []string{"valid", "wrong space"},
712708
},
713709
"project has uppercase characters": &v2.CreateRoleReq{
714710
Id: "this-is-valid-1",
715711
Name: "name of my team ~ fun characters 1 %",
712+
Actions: []string{"cfgmgmt:nodes:*"},
716713
Projects: []string{"valid", "PROJECT1"},
717714
},
718715
}
@@ -721,16 +718,19 @@ func TestValidationCreateRole(t *testing.T) {
721718
"alphanumeric IDs allowed": &v2.CreateRoleReq{
722719
Id: "asdf-123-fun",
723720
Name: "this is valid ~ fun characters",
721+
Actions: []string{"cfgmgmt:nodes:*"},
724722
Projects: []string{"test", "test-2"},
725723
},
726724
"empty projects list": &v2.CreateRoleReq{
727725
Id: "this-is-valid-1",
728726
Name: "name of my team ~ fun characters 1 %",
727+
Actions: []string{"cfgmgmt:nodes:*"},
729728
Projects: []string{},
730729
},
731730
"missing projects list": &v2.CreateRoleReq{
732-
Id: "this-is-valid-1",
733-
Name: "name of my team ~ fun characters 1 %",
731+
Id: "this-is-valid-1",
732+
Name: "name of my team ~ fun characters 1 %",
733+
Actions: []string{"cfgmgmt:nodes:*"},
734734
},
735735
}
736736

0 commit comments

Comments
 (0)