Skip to content

Commit

Permalink
Change user hyper tag with UBID
Browse files Browse the repository at this point in the history
All resources are represented in our authorization system, referred to
as a "hyper tag". Each resource has a unique format, for example, users
are represented as "user/test@example.com", projects as
"project/pjbw1tcvkew67qcxqsjpbvh19c", and virtual machines as
"project/pjcm807azyxe6ht7ct027tcg83/location/hetzner-fsn1/vm/my-vm".

We prefer using user-friendly names over UBIDs, making it easier for
customers to edit their access policies. However, we identified a
problem with this approach. When a resource's hyper tag changes (e.g., a
user changes their email), the existing access policies need to be
updated accordingly. Without such updates, permissions to resources
could be affected.

Automatically updating access policies to apply these changes is not
recommended. Instead, I propose keeping the resource's UBID as the
hyper tag in the database, which eliminates the need for changes when
resources are renamed. This is because UBIDs remain constant throughout
a resource's lifetime.

We only need to map the prettified policy name when displaying or saving
access policies. The authorization system uses UBIDs in the background.

I have refactored the user's hyper tag for now. I will continue to
refactor other resources in subsequent PRs after the deployment of user
change.

We need to update user hyper tags in production after deployment.

From a security perspective, we only transform tags/UBIDs for resources
associated with this project.
  • Loading branch information
enescakir committed Feb 5, 2024
1 parent 6e56364 commit a52da99
Show file tree
Hide file tree
Showing 7 changed files with 42 additions and 6 deletions.
26 changes: 26 additions & 0 deletions model/access_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,32 @@ class AccessPolicy < Sequel::Model
many_to_one :project

include ResourceMethods

def transform_to_names
tag_to_name = project.accounts.map { [_1.hyper_tag_name, _1.to_policy] }.to_h
transform(tag_to_name)
self
end

def transform_to_tags
name_to_tag = project.accounts.map { [_1.to_policy, _1.hyper_tag_name] }.to_h
transform(name_to_tag)
self
end

def transform(lookup)
body["acls"] = body["acls"].map do
_1.tap { |acl|
acl["subjects"] = value_from_lookup(lookup, acl["subjects"])
acl["objects"] = value_from_lookup(lookup, acl["objects"])
}
end
end

def value_from_lookup(lookup, key)
return key.map { value_from_lookup(lookup, _1) } if key.is_a?(Array)
lookup[key] || key
end
end

# We need to unrestrict primary key so project.add_access_policy works
Expand Down
4 changes: 4 additions & 0 deletions model/account.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ class Account < Sequel::Model(:accounts)
include Authorization::HyperTagMethods

def hyper_tag_name(project = nil)
ubid
end

def to_policy(project = nil)
"user/#{email}"
end

Expand Down
1 change: 1 addition & 0 deletions model/project.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ class Project < Sequel::Model
many_to_many :minio_clusters, join_table: AccessTag.table_name, left_key: :project_id, right_key: :hyper_tag_id
many_to_many :private_subnets, join_table: AccessTag.table_name, left_key: :project_id, right_key: :hyper_tag_id
many_to_many :postgres_resources, join_table: AccessTag.table_name, left_key: :project_id, right_key: :hyper_tag_id
many_to_many :accounts, join_table: AccessTag.table_name, left_key: :project_id, right_key: :hyper_tag_id

one_to_many :invoices, order: Sequel.desc(:created_at)

Expand Down
2 changes: 1 addition & 1 deletion routes/web/project/policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class CloverWeb
return redirect_back_with_inputs
end

policy.update(body: body)
policy.tap { _1.body = body }.transform_to_tags.save_changes

flash["notice"] = "'#{policy.name}' policy updated for '#{@project.name}'"

Expand Down
1 change: 1 addition & 0 deletions serializers/web/access_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

class Serializers::Web::AccessPolicy < Serializers::Base
def self.base(ap)
ap.transform_to_names
{
id: ap.id,
ubid: ap.ubid,
Expand Down
6 changes: 5 additions & 1 deletion spec/lib/authorization_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -104,12 +104,16 @@

describe "#HyperTagMethods" do
it "hyper_tag_name" do
expect(users[0].hyper_tag_name).to eq("user/auth1@example.com")
expect(users[0].hyper_tag_name).to eq(users[0].ubid)
p = vms[0].projects.first
expect(vms[0].hyper_tag_name(p)).to eq("project/#{p.ubid}/location/hetzner-hel1/vm/vm0")
expect(projects[0].hyper_tag_name).to eq("project/#{projects[0].ubid}")
end

it "to_policy" do
expect(users[0].to_policy).to eq("user/auth1@example.com")
end

it "hyper_tag_name error" do
c = Class.new(Sequel::Model) do
include Authorization::HyperTagMethods
Expand Down
8 changes: 4 additions & 4 deletions spec/routes/web/project_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@
end

expect(page.title).to eq("Ubicloud - #{project.name} - Policy")
expect(page).to have_content project.access_policies.first.body.to_json
expect(page).to have_content project.access_policies.first.transform_to_names.body.to_json
end

it "raises forbidden when does not have permissions" do
Expand All @@ -257,16 +257,16 @@
end

it "can update policy" do
current_policy = project.access_policies.first.body
current_policy = project.access_policies.first
new_policy = {
acls: [
{actions: ["Project:policy"], objects: project.hyper_tag_name, subjects: user.hyper_tag_name}
{actions: ["Project:policy"], objects: project.hyper_tag_name, subjects: user.to_policy}
]
}

visit "#{project.path}/policy"

expect(page).to have_content current_policy.to_json
expect(page).to have_content current_policy.transform_to_names.body.to_json

fill_in "body", with: new_policy.to_json
click_button "Update"
Expand Down

0 comments on commit a52da99

Please sign in to comment.