Skip to content

Commit

Permalink
[fix] future priv (#281)
Browse files Browse the repository at this point in the history
<!-- Feel free to delete comments as you fill this in -->

<!-- summary of changes -->

## Test Plan
<!-- detail ways in which this PR has been tested or needs to be tested -->
* [ ] acceptance tests
<!-- add more below if you think they are relevant -->
* [x ] Test cases modified `table_grant_test.go` and `view_grant_test.go`

## References
<!-- issues documentation links, etc  -->

* Fixes #279
* Some notes about the fix: as I am not really experienced with the provider I did not find any elegant solution how to exactly test that the `readGenericGrant` is executed on the `ReadTableGrant` or `ReadViewGrant` function call. So I had to reflect the builder interface but that was not enough, as the builder in both cases is FutureGrant. So I made a workaround by using a `validPrivileges` set, in VIEWS it contains only one item, the `SELECT` privilege.
  • Loading branch information
funes79 authored Nov 2, 2020
1 parent ecb384f commit cdfdd21
Show file tree
Hide file tree
Showing 4 changed files with 120 additions and 6 deletions.
28 changes: 26 additions & 2 deletions pkg/resources/grant_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"database/sql"
"encoding/csv"
"fmt"
"reflect"
"strings"
"time"

Expand Down Expand Up @@ -213,6 +214,21 @@ func readGenericGrant(data *schema.ResourceData, meta interface{}, builder snowf
priv := data.Get("privilege").(string)
grantOption := data.Get("with_grant_option").(bool)

// This is the only way how I can test that this function is reading VIEW grants or TABLE grants
// is checking what kind of builder we have. If it is future grant, then I doubple check if the
// privilegeSet has only one member - SELECT - then it is a VIEW, if it has 6 members and contains
// Truncate then it must be Table
futureGrantOnViews := false
futureGrantOnTables := false
if reflect.TypeOf(builder) == reflect.TypeOf(&snowflake.FutureGrantBuilder{}) {
if _, ok := validPrivileges[privilegeSelect]; ok && len(validPrivileges) == 1 {
futureGrantOnViews = true
}
if _, ok := validPrivileges[privilegeTruncate]; ok && len(validPrivileges) == 6 {
futureGrantOnTables = true
}
}

// We re-aggregate grants that would be equivalent to the "ALL" grant
grants = filterALLGrants(grants, validPrivileges)

Expand All @@ -231,8 +247,16 @@ func readGenericGrant(data *schema.ResourceData, meta interface{}, builder snowf
// If not there, create an empty set
privileges = privilegeSet{}
}
// Add privilege to the set
privileges.addString(grant.Privilege)
// Add privilege to the set but consider valid privileges only
// for VIEW in ReadViewGrant
// and for non-VIEW in ReadTableGrant
if futureGrantOnViews || futureGrantOnTables {
if (futureGrantOnViews && grant.GrantType == "VIEW") || (futureGrantOnTables && grant.GrantType == "TABLE") {
privileges.addString(grant.Privilege)
}
} else { // Other grants
privileges.addString(grant.Privilege)
}
// Reassign set back
rolePrivileges[roleName] = privileges
case "SHARE":
Expand Down
79 changes: 75 additions & 4 deletions pkg/resources/schema_grant_acceptance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"os"
"testing"

"strings"

"github.com/hashicorp/terraform-plugin-sdk/helper/acctest"
"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
)
Expand All @@ -14,9 +16,9 @@ func TestAccSchemaGrant(t *testing.T) {
t.Skip("Skipping TestAccSchemaGrant")
}

sName := acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)
roleName := acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)
shareName := acctest.RandStringFromCharSet(10, acctest.CharSetAlpha)
sName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha))
roleName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha))
shareName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha))

resource.Test(t, resource.TestCase{
Providers: providers(),
Expand All @@ -29,7 +31,7 @@ func TestAccSchemaGrant(t *testing.T) {
resource.TestCheckResourceAttr("snowflake_schema_grant.test", "privilege", "USAGE"),
),
},
// FUTURE
// FUTURE SHARES
{
Config: schemaGrantConfig(sName, roleName, shareName, true),
Check: resource.ComposeTestCheckFunc(
Expand All @@ -48,6 +50,75 @@ func TestAccSchemaGrant(t *testing.T) {
})
}

func TestAccSchemaFutureGrants(t *testing.T) {

sName := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha))
roleNameTable := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha))
roleNameView := strings.ToUpper(acctest.RandStringFromCharSet(10, acctest.CharSetAlpha))

resource.Test(t, resource.TestCase{
Providers: providers(),
Steps: []resource.TestStep{
// TABLE AND VIEW FUTURE GRANTS
{
Config: futureTableAndViewGrantConfig(sName, roleNameTable, roleNameView),
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("snowflake_view_grant.select_on_future_views", "roles.#", "1"),
resource.TestCheckResourceAttr("snowflake_table_grant.select_on_future_tables", "roles.#", "1"),
resource.TestCheckResourceAttr("snowflake_view_grant.select_on_future_views", "privilege", "SELECT"),
),
},
// IMPORT
{
ResourceName: "snowflake_view_grant.select_on_future_views",
ImportState: true,
ImportStateVerify: true,
},
},
})
}

func futureTableAndViewGrantConfig(n, role_table, role_view string) string {
return fmt.Sprintf(`
resource "snowflake_database" "test" {
name = "%v"
}
resource "snowflake_schema" "test" {
name = "%v"
database = snowflake_database.test.name
comment = "Terraform acceptance test"
}
resource "snowflake_role" "table_reader" {
name = "%v"
}
resource "snowflake_role" "view_reader" {
name = "%v"
}
resource "snowflake_table_grant" "select_on_future_tables" {
database_name = snowflake_database.test.name
schema_name = snowflake_schema.test.name
privilege = "SELECT"
on_future = true
roles = [snowflake_role.table_reader.name]
depends_on = [snowflake_schema.test, snowflake_role.table_reader]
}
resource "snowflake_view_grant" "select_on_future_views" {
database_name = snowflake_database.test.name
schema_name = snowflake_schema.test.name
privilege = "SELECT"
on_future = true
roles = [snowflake_role.view_reader.name]
depends_on = [snowflake_schema.test, snowflake_role.view_reader]
}
`, n, n, role_table, role_view)
}

func schemaGrantConfig(n, role, share string, future bool) string {
schema_name_config := `schema_name = snowflake_schema.test.name
shares = [snowflake_share.test.name]`
Expand Down
9 changes: 9 additions & 0 deletions pkg/resources/table_grant_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,13 @@ func TestFutureTableGrantCreate(t *testing.T) {
).WillReturnResult(sqlmock.NewResult(1, 1))
expectReadFutureTableGrant(mock)
err := resources.CreateTableGrant(d, db)
roles := d.Get("roles").(*schema.Set)
// After the CreateTableGrant has been created a ReadTableGrant reads the current grants
// and this read should ignore test-role-3 what is returned by SHOW FUTURE GRANTS ON SCHEMA PUBLIC because
// test-role-3 has been granted to a SELECT on future VIEW and not on future TABLE
r.True(roles.Contains("test-role-1"))
r.True(roles.Contains("test-role-2"))
r.False(roles.Contains("test-role-3"))
r.NoError(err)
})

Expand Down Expand Up @@ -151,6 +158,8 @@ func expectReadFutureTableGrant(mock sqlmock.Sqlmock) {
time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC), "SELECT", "TABLE", "test-db.PUBLIC.<TABLE>", "ROLE", "test-role-1", false,
).AddRow(
time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC), "SELECT", "TABLE", "test-db.PUBLIC.<TABLE>", "ROLE", "test-role-2", false,
).AddRow(
time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC), "SELECT", "VIEW", "test-db.PUBLIC.<VIEW>", "ROLE", "test-role-3", false,
)
mock.ExpectQuery(`^SHOW FUTURE GRANTS IN SCHEMA "test-db"."PUBLIC"$`).WillReturnRows(rows)
}
Expand Down
10 changes: 10 additions & 0 deletions pkg/resources/view_grant_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,14 @@ func TestFutureViewGrantCreate(t *testing.T) {
expectReadFutureViewGrant(mock)
err := resources.CreateViewGrant(d, db)
r.NoError(err)

roles := d.Get("roles").(*schema.Set)
// After the CreateFutureViewGrant has been created a ReadViewGrant reads the current grants
// and this read should ignore test-role-3 what is returned by SHOW FUTURE GRANTS ON SCHEMA PUBLIC because
// test-role-3 has been granted to a SELECT on future TABLE and not on future VIEW
r.True(roles.Contains("test-role-1"))
r.True(roles.Contains("test-role-2"))
r.False(roles.Contains("test-role-3"))
})

b := require.New(t)
Expand Down Expand Up @@ -150,6 +158,8 @@ func expectReadFutureViewGrant(mock sqlmock.Sqlmock) {
time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC), "SELECT", "VIEW", "test-db.PUBLIC.<VIEW>", "ROLE", "test-role-1", false,
).AddRow(
time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC), "SELECT", "VIEW", "test-db.PUBLIC.<VIEW>", "ROLE", "test-role-2", false,
).AddRow(
time.Date(2000, 1, 1, 0, 0, 0, 0, time.UTC), "SELECT", "TABLE", "test-db.PUBLIC.<TABLE>", "ROLE", "test-role-3", false,
)
mock.ExpectQuery(`^SHOW FUTURE GRANTS IN SCHEMA "test-db"."PUBLIC"$`).WillReturnRows(rows)
}
Expand Down

0 comments on commit cdfdd21

Please sign in to comment.