From de2e1d8997ef8b20b322fd972971c5230df2f0a3 Mon Sep 17 00:00:00 2001 From: Austin Gebauer <34121980+austingebauer@users.noreply.github.com> Date: Wed, 22 Dec 2021 20:24:34 -0500 Subject: [PATCH] Fixes roleset bindings for BigQuery datasets (#130) --- CHANGELOG.md | 14 +++++++++ plugin/iamutil/dataset_resource.go | 42 +++++++++++++++++++++++-- plugin/iamutil/dataset_resource_test.go | 4 +-- plugin/iamutil/resource_parser.go | 2 +- plugin/iamutil/resource_parser_test.go | 18 +++++------ 5 files changed, 65 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 854fcb77..da598ec3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,19 @@ ## Next +## v0.11.1 +### Unreleased + +BUG FIXES: + +* Fixes role bindings for BigQuery dataset resources [[GH-130](https://github.com/hashicorp/vault-plugin-secrets-gcp/pull/130)] + +## v0.10.3 +### Unreleased + +BUG FIXES: + +* Fixes role bindings for BigQuery dataset resources [[GH-130](https://github.com/hashicorp/vault-plugin-secrets-gcp/pull/130)] + ## 0.8.1 ### December 14, 2020 diff --git a/plugin/iamutil/dataset_resource.go b/plugin/iamutil/dataset_resource.go index 51903284..0242a11e 100644 --- a/plugin/iamutil/dataset_resource.go +++ b/plugin/iamutil/dataset_resource.go @@ -118,6 +118,18 @@ func datasetAsPolicy(ds *Dataset) *Policy { for _, accessBinding := range ds.Access { var iamMember string + // Role mapping must be applied for datasets in order to properly + // detect when to change bindings (via RemoveBindings()) after a + // modification or deletion occurs. This is due to BigQuery + // access roles accepting both legacy (e.g., OWNER) and current + // (e.g., roles/bigquery.dataOwner) role references. The API will + // only return the legacy format, so this mapping allows us to properly + // diff the current and desired roles to set the access policy. + // + // See the access[].role description in the following document for details + // https://cloud.google.com/bigquery/docs/reference/rest/v2/datasets#Dataset + role := mapLegacyRoles(accessBinding.Role) + //NOTE: Can either have GroupByEmail or UserByEmail but not both if accessBinding.GroupByEmail != "" { iamMember = fmt.Sprintf("group:%s", accessBinding.GroupByEmail) @@ -126,11 +138,11 @@ func datasetAsPolicy(ds *Dataset) *Policy { } else { iamMember = fmt.Sprintf("user:%s", accessBinding.UserByEmail) } - if binding, ok := bindingMap[accessBinding.Role]; ok { + if binding, ok := bindingMap[role]; ok { binding.Members = append(binding.Members, iamMember) } else { - bindingMap[accessBinding.Role] = &Binding{ - Role: accessBinding.Role, + bindingMap[role] = &Binding{ + Role: role, Members: []string{iamMember}, } } @@ -140,3 +152,27 @@ func datasetAsPolicy(ds *Dataset) *Policy { } return policy } + +// mapLegacyRoles returns a current role name given a legacy role name. +// +// The following role mappings will be applied: +// - OWNER -> roles/bigquery.dataOwner +// - WRITER -> roles/bigquery.dataEditor +// - READER -> roles/bigquery.dataViewer +// +// See the access[].role description in the following document for details +// https://cloud.google.com/bigquery/docs/reference/rest/v2/datasets#Dataset +// +// Returns the given role if no mapping applies. +func mapLegacyRoles(role string) string { + switch role { + case "OWNER": + return "roles/bigquery.dataOwner" + case "WRITER": + return "roles/bigquery.dataEditor" + case "READER": + return "roles/bigquery.dataViewer" + default: + return role + } +} diff --git a/plugin/iamutil/dataset_resource_test.go b/plugin/iamutil/dataset_resource_test.go index 72891798..e9c51c9d 100644 --- a/plugin/iamutil/dataset_resource_test.go +++ b/plugin/iamutil/dataset_resource_test.go @@ -210,8 +210,8 @@ func getTestFixtures() (*Policy, *Dataset) { func testResource() *DatasetResource { return &DatasetResource{ relativeId: &gcputil.RelativeResourceName{ - Name: "dataset", - TypeKey: "projects/dataset", + Name: "datasets", + TypeKey: "projects/datasets", IdTuples: map[string]string{ "projects": "project", "datasets": "dataset", diff --git a/plugin/iamutil/resource_parser.go b/plugin/iamutil/resource_parser.go index a8c7589d..0d2fb19f 100644 --- a/plugin/iamutil/resource_parser.go +++ b/plugin/iamutil/resource_parser.go @@ -125,7 +125,7 @@ func (apis GeneratedResources) Parse(rawName string) (Resource, error) { return nil, err } switch cfg.TypeKey { - case "projects/dataset": + case "projects/datasets": return &DatasetResource{relativeId: relName, config: cfg}, nil default: return &IamResource{relativeId: relName, config: cfg}, nil diff --git a/plugin/iamutil/resource_parser_test.go b/plugin/iamutil/resource_parser_test.go index 5a1e080d..ad3c885c 100644 --- a/plugin/iamutil/resource_parser_test.go +++ b/plugin/iamutil/resource_parser_test.go @@ -13,7 +13,7 @@ import ( var letters = "ABCDEFGHIJKLMNOP" -func TestEnabledIamResources_RelativeName(t *testing.T) { +func TestEnabledResources_RelativeName(t *testing.T) { enabledApis := GetEnabledResources() for resourceType, services := range generatedResources { @@ -39,7 +39,7 @@ func TestEnabledIamResources_RelativeName(t *testing.T) { } if resource != nil { - if err = verifyResource(resourceType, resource.(*IamResource)); err != nil { + if err = verifyResource(resourceType, resource); err != nil { t.Errorf("could not verify resource for relative resource name %q: %sv", testRelName, err) } } @@ -50,7 +50,7 @@ func TestEnabledIamResources_RelativeName(t *testing.T) { } } -func TestEnabledIamResources_FullName(t *testing.T) { +func TestEnabledResources_FullName(t *testing.T) { enabledApis := GetEnabledResources() for resourceType, services := range generatedResources { @@ -67,7 +67,7 @@ func TestEnabledIamResources_FullName(t *testing.T) { t.Errorf("failed to get resource for full resource name %s (type: %s): %v", testFullName, resourceType, err) continue } - if err = verifyResource(resourceType, resource.(*IamResource)); err != nil { + if err = verifyResource(resourceType, resource); err != nil { t.Errorf("could not verify resource for relative resource name %s: %v", testFullName, err) continue } @@ -226,13 +226,13 @@ func getFakeId(resourceType string) string { return strings.Trim(fakeId, "/") } -func verifyResource(rType string, resource *IamResource) (err error) { +func verifyResource(rType string, resource Resource) (err error) { var req *http.Request - if resource.relativeId.TypeKey != rType { - return fmt.Errorf("expected resource type %s, actual resource has different type %s", rType, resource.relativeId.TypeKey) + if resource.GetRelativeId().TypeKey != rType { + return fmt.Errorf("expected resource type %s, actual resource has different type %s", rType, resource.GetRelativeId().TypeKey) } - req, err = constructRequest(resource, &resource.config.GetMethod, nil) + req, err = constructRequest(resource, &resource.GetConfig().GetMethod, nil) if err != nil { return errwrap.Wrapf("unable to construct GetIamPolicyRequest: {{err}}", err) } @@ -240,7 +240,7 @@ func verifyResource(rType string, resource *IamResource) (err error) { return err } - req, err = constructRequest(resource, &resource.config.SetMethod, strings.NewReader("{}")) + req, err = constructRequest(resource, &resource.GetConfig().SetMethod, strings.NewReader("{}")) if err != nil { return errwrap.Wrapf("unable to construct SetIamPolicyRequest: {{err}}", err) }