Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(spanner): Support fine grained access control #19067

Merged
merged 9 commits into from Sep 5, 2022
@@ -0,0 +1,79 @@
# Copyright 2022 Google LLC
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# https://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.


require "spanner_helper"

describe "Fine Grained Access Control", :spanner do
let(:table_name) { "stuffs" }
let(:db) { spanner }
let(:db_client) { spanner_client }
let(:admin) { $spanner_db_admin }
let(:instance_id) { $spanner_instance_id }
let(:database_id) { $spanner_database_id }

before do
skip if emulator_enabled?
db_client.delete table_name # remove all data
db_client.insert table_name, [
{ id: 1, bool: false },
{ id: 2, bool: false },
{ id: 3, bool: true },
{ id: 4, bool: false },
{ id: 5, bool: true }
]

db_path = admin.database_path project: db.project_id,
instance: instance_id,
database: database_id

db_job = admin.update_database_ddl database: db_path, statements: [
"CREATE ROLE selector",
NivedhaSenthil marked this conversation as resolved.
Show resolved Hide resolved
"GRANT SELECT ON TABLE stuffs TO ROLE selector"
NivedhaSenthil marked this conversation as resolved.
Show resolved Hide resolved
]
db_job.wait_until_done!
end

it "should be able to do granted actions for role" do
skip if emulator_enabled?
selector_client = db.client $spanner_instance_id, $spanner_database_id, database_role: "selector"
_(selector_client.read(table_name, [:id]).rows.map(&:to_h)).must_equal [{ id: 1 },
{ id: 2 },
{ id: 3 },
{ id: 4 },
{ id: 5 }]
end

it "should give error for actions without access" do
skip if emulator_enabled?
selector_client = db.client $spanner_instance_id, $spanner_database_id, database_role: "selector"
error = assert_raises Google::Cloud::PermissionDeniedError do
selector_client.insert table_name, [
{ id: 1, bool: false }
]
end

assert_includes error.message, "Role selector does not have required privileges on table stuffs"
end

it "should give error when database role does not exists" do
skip if emulator_enabled?

error = assert_raises Google::Cloud::PermissionDeniedError do
db.client $spanner_instance_id, $spanner_database_id, database_role: "unknown"
end

assert_includes error.message, "Role not found: unknown"
end
end
15 changes: 12 additions & 3 deletions google-cloud-spanner/lib/google/cloud/spanner/client.rb
Expand Up @@ -52,10 +52,11 @@ class Client
##
# @private Creates a new Spanner Client instance.
def initialize project, instance_id, database_id, session_labels: nil,
pool_opts: {}, query_options: nil
pool_opts: {}, query_options: nil, database_role: nil
@project = project
@instance_id = instance_id
@database_id = database_id
@database_role = database_role
@session_labels = session_labels
@pool = Pool.new self, **pool_opts
@query_options = query_options
Expand Down Expand Up @@ -97,6 +98,12 @@ def database
@project.database instance_id, database_id
end

# The Spanner session creator role.
# @return [String]
def database_role
@database_role
end

# A hash of values to specify the custom query options for executing
# SQL query.
# @return [Hash]
Expand Down Expand Up @@ -2115,7 +2122,8 @@ def create_new_session
Admin::Database::V1::DatabaseAdmin::Paths.database_path(
project: project_id, instance: instance_id, database: database_id
),
labels: @session_labels
labels: @session_labels,
database_role: @database_role
Session.from_grpc grpc, @project.service, query_options: @query_options
end

Expand Down Expand Up @@ -2144,7 +2152,8 @@ def batch_create_sessions session_count
project: project_id, instance: instance_id, database: database_id
),
session_count,
labels: @session_labels
labels: @session_labels,
database_role: @database_role
resp.session.map { |grpc| Session.from_grpc grpc, @project.service, query_options: @query_options }
end

Expand Down
3 changes: 1 addition & 2 deletions google-cloud-spanner/lib/google/cloud/spanner/pool.rb
Expand Up @@ -277,8 +277,7 @@ def can_allocate_more_sessions?
end

def create_keepalive_task!
@keepalive_task = Concurrent::TimerTask.new(execution_interval: 300,
timeout_interval: 60) do
@keepalive_task = Concurrent::TimerTask.new execution_interval: 300 do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we removing timeout_interval ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timeout_interval is not taken into account by concurrent_ruby now https://github.com/ruby-concurrency/concurrent-ruby/blob/4cfc0a107b10d3ab213396d53411a1ee487200af/lib/concurrent-ruby/concurrent/timer_task.rb#L268 .. using it puts a lot of unnecessary warnings when running tests

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks. Can we just add a comment here for that?
Overall LGTM

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer adding it in the commit description when merging,
As we are anyways removing the option. Does that sound fine ?

keepalive_or_release!
end
@keepalive_task.execute
Expand Down
5 changes: 3 additions & 2 deletions google-cloud-spanner/lib/google/cloud/spanner/project.rb
Expand Up @@ -569,7 +569,7 @@ def create_database instance_id, database_id, statements: [],
# end
#
def client instance_id, database_id, pool: {}, labels: nil,
query_options: nil
query_options: nil, database_role: nil
# Convert from possible Google::Protobuf::Map
labels = labels.to_h { |k, v| [String(k), String(v)] } if labels
# Configs set by environment variables take over client-level configs.
Expand All @@ -581,7 +581,8 @@ def client instance_id, database_id, pool: {}, labels: nil,
Client.new self, instance_id, database_id,
session_labels: labels,
pool_opts: valid_session_pool_options(pool),
query_options: query_options
query_options: query_options,
database_role: database_role
end

##
Expand Down
8 changes: 4 additions & 4 deletions google-cloud-spanner/lib/google/cloud/spanner/service.rb
Expand Up @@ -293,20 +293,20 @@ def get_session session_name, call_options: nil
end

def create_session database_name, labels: nil,
call_options: nil
call_options: nil, database_role: nil
opts = default_options session_name: database_name,
call_options: call_options
session = V1::Session.new labels: labels if labels
session = V1::Session.new labels: labels, creator_role: database_role if labels || database_role
service.create_session(
{ database: database_name, session: session }, opts
)
end

def batch_create_sessions database_name, session_count, labels: nil,
call_options: nil
call_options: nil, database_role: nil
opts = default_options session_name: database_name,
call_options: call_options
session = V1::Session.new labels: labels if labels
session = V1::Session.new labels: labels, creator_role: database_role if labels || database_role
# The response may have fewer sessions than requested in the RPC.
request = {
database: database_name,
Expand Down
21 changes: 21 additions & 0 deletions google-cloud-spanner/test/google/cloud/spanner/project_test.rb
Expand Up @@ -15,8 +15,29 @@
require "helper"

describe Google::Cloud::Spanner::Project, :mock_spanner do
let(:instance_id) { "my-instance-id" }
let(:database_id) { "my-database-id" }
let(:session_id) { "session123" }
let(:session_grpc) {
Google::Cloud::Spanner::V1::Session.new name: session_path(instance_id, database_id, session_id)
}
let(:session) { Google::Cloud::Spanner::Session.from_grpc session_grpc, spanner.service }
let(:batch_create_sessions_grpc) {
Google::Cloud::Spanner::V1::BatchCreateSessionsResponse.new session: [session_grpc]
}

it "knows the project identifier" do
_(spanner).must_be_kind_of Google::Cloud::Spanner::Project
_(spanner.project_id).must_equal project
end

it "creates client with database role" do
mock = Minitest::Mock.new
request_session = Google::Cloud::Spanner::V1::Session.new labels: nil, creator_role: "test_role"
mock.expect :batch_create_sessions, batch_create_sessions_grpc, [Hash,::Gapic::CallOptions]
spanner.service.mocked_service = mock

client = spanner.client instance_id, database_id, pool: { min: 1, max: 1 }, database_role: "test-role"
_(client.database_role).must_equal "test-role"
end
end
67 changes: 52 additions & 15 deletions google-cloud-spanner/test/google/cloud/spanner/service_test.rb
Expand Up @@ -12,22 +12,59 @@
# See the License for the specific language governing permissions and
# limitations under the License.

describe Google::Cloud::Spanner::Service do
require "helper"

describe Google::Cloud::Spanner::Service, :mock_spanner do
let(:instance_id) { "my-instance-id" }
let(:database_id) { "my-database-id" }
let(:session_id) { "session123" }
let(:default_options) { ::Gapic::CallOptions.new metadata: { "google-cloud-resource-prefix" => database_path(instance_id, database_id) } }
let(:session_grpc) { Google::Cloud::Spanner::V1::Session.new name: session_path(instance_id, database_id, session_id) }

describe ".new" do
it "sets quota_project with given value" do
expected_quota_project = "test_quota_project"
service = Google::Cloud::Spanner::Service.new(
"test_project", nil, quota_project: expected_quota_project
)
assert_equal expected_quota_project, service.quota_project
end
it "sets quota_project with given value" do
expected_quota_project = "test_quota_project"
service = Google::Cloud::Spanner::Service.new(
"test_project", nil, quota_project: expected_quota_project
)
assert_equal expected_quota_project, service.quota_project
end

it "sets quota_project from credentials if not given from config" do
expected_quota_project = "test_quota_project"
service = Google::Cloud::Spanner::Service.new(
"test_project", OpenStruct.new(quota_project_id: expected_quota_project)
)
assert_equal expected_quota_project, service.quota_project
end

end

describe ".create_session" do
it "creates session with given database role" do
mock = Minitest::Mock.new
session = Google::Cloud::Spanner::V1::Session.new labels: nil, creator_role: "test_role"
mock.expect :create_session, session_grpc, [{ database: database_path(instance_id, database_id), session: session }, default_options]
service = Google::Cloud::Spanner::Service.new(
"test_project", OpenStruct.new(client: OpenStruct.new(updater_proc: Proc.new{""}))
)
service.mocked_service = mock
service.create_session database_path(instance_id, database_id), database_role: "test_role"
mock.verify
end
end

it "sets quota_project from credentials if not given from config" do
expected_quota_project = "test_quota_project"
service = Google::Cloud::Spanner::Service.new(
"test_project", OpenStruct.new(quota_project_id: expected_quota_project)
)
assert_equal expected_quota_project, service.quota_project
end
describe ".batch_create_sessions" do
it "batch creates session with given database role" do
mock = Minitest::Mock.new
session = Google::Cloud::Spanner::V1::Session.new labels: nil, creator_role: "test_role"
mock.expect :batch_create_sessions, OpenStruct.new(session: Array.new(10) { session_grpc }), [{database: database_path(instance_id, database_id), session_count: 10, session_template: session }, default_options]
service = Google::Cloud::Spanner::Service.new(
"test_project", OpenStruct.new(client: OpenStruct.new(updater_proc: Proc.new{""}))
)
service.mocked_service = mock
service.batch_create_sessions database_path(instance_id, database_id), 10, database_role: "test_role"
mock.verify
end
end
end