From 3faf68c829605a7b9a287414ea0d3ebba9c272ff Mon Sep 17 00:00:00 2001 From: timalces Date: Thu, 29 Feb 2024 17:09:39 +0000 Subject: [PATCH 01/10] create user team during sign up --- app/jobs/create_user_team_job.rb | 31 +++++++++++++++++++++++++++++++ app/jobs/user_signup_job.rb | 1 + 2 files changed, 32 insertions(+) create mode 100644 app/jobs/create_user_team_job.rb diff --git a/app/jobs/create_user_team_job.rb b/app/jobs/create_user_team_job.rb new file mode 100644 index 000000000..c9538b907 --- /dev/null +++ b/app/jobs/create_user_team_job.rb @@ -0,0 +1,31 @@ +require 'faraday' + +class CreateUserTeamJob < CreateTeamJob + + def perform(user, cloud_service_config, **options) + records_created = false + team = nil + + ActiveRecord::Base.transaction do + team = Team.new(name: "#{user.name}_team") + unless team.save + logger.info("Unable to create team for #{user.name} #{team.errors.full_messages.join("; ")}") + return + end + + team_role = TeamRole.new(team: team, user: user, role: "admin") + if team_role.save + records_created = true + else + logger.info("Unable to create team role for #{user.name} #{team_role.errors.full_messages.join("; ")}") + raise ActiveRecord::Rollback, "Team role creation failed, rolling back creation of user team" + end + end + + if records_created + super(team, cloud_service_config, **options) + else + raise + end + end +end diff --git a/app/jobs/user_signup_job.rb b/app/jobs/user_signup_job.rb index 23e1a855f..ff9bc3390 100644 --- a/app/jobs/user_signup_job.rb +++ b/app/jobs/user_signup_job.rb @@ -38,6 +38,7 @@ def call result = Result.from(response.body) result.validate!(:cloud) result.sync(@user, :cloud) + CreateUserTeamJob.perform_later(@user, @cloud_service_config) rescue ::ActiveModel::ValidationError @logger.warn("Failed to sync response to user: #{$!.message}") raise From 1434dfef6aa1e395ccc3d58fa52f2592f723b384 Mon Sep 17 00:00:00 2001 From: timalces Date: Thu, 29 Feb 2024 18:49:38 +0000 Subject: [PATCH 02/10] added creation of user team on signup --- app/jobs/create_user_team_job.rb | 22 +++++++------------ app/models/ability.rb | 2 +- app/models/user.rb | 12 ++++++++++ app/views/teams/index.html.erb | 1 + .../20240229172848_add_single_user_to_team.rb | 5 +++++ db/schema.rb | 8 +++++++ 6 files changed, 35 insertions(+), 15 deletions(-) create mode 100644 db/migrate/20240229172848_add_single_user_to_team.rb diff --git a/app/jobs/create_user_team_job.rb b/app/jobs/create_user_team_job.rb index c9538b907..c931503f9 100644 --- a/app/jobs/create_user_team_job.rb +++ b/app/jobs/create_user_team_job.rb @@ -1,31 +1,25 @@ -require 'faraday' - class CreateUserTeamJob < CreateTeamJob + queue_as :default + retry_on ::ActiveModel::ValidationError, wait: :polynomially_longer, attempts: 10 def perform(user, cloud_service_config, **options) - records_created = false team = nil ActiveRecord::Base.transaction do - team = Team.new(name: "#{user.name}_team") + team = Team.new(name: "#{user.name}_team", single_user: true) unless team.save logger.info("Unable to create team for #{user.name} #{team.errors.full_messages.join("; ")}") - return + raise ActiveModel::ValidationError, team end team_role = TeamRole.new(team: team, user: user, role: "admin") - if team_role.save - records_created = true - else + unless team_role.save logger.info("Unable to create team role for #{user.name} #{team_role.errors.full_messages.join("; ")}") - raise ActiveRecord::Rollback, "Team role creation failed, rolling back creation of user team" + logger.info("Rolling back creation of team #{team.name}") + raise ActiveModel::ValidationError, team_role end end - if records_created - super(team, cloud_service_config, **options) - else - raise - end + CreateTeamJob.perform_later(team, cloud_service_config) end end diff --git a/app/models/ability.rb b/app/models/ability.rb index 00a28d2ef..61c2f9f90 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -51,7 +51,7 @@ def non_root_abilities can [:read, :update], User, id: @user.id can :read, Team, id: @user.team_ids - can :manage, TeamRole, team_id: @user.team_roles.where(role: "admin").pluck(:team_id) + can :manage, TeamRole, team_id: @user.teams_where_admin.where(single_user: false).pluck(:id) # Invoice is an ActiveModel::Model, but not an ActiveRecord::Base. Setting # abilities like this might not work too well. Or perhaps its fine. diff --git a/app/models/user.rb b/app/models/user.rb index bd89c6c1e..82da36ba1 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -124,4 +124,16 @@ def mark_as_pending_deletion update(deleted_at: Time.current) allowlisted_jwts.destroy_all end + + #################################### + # + # Private Instance Methods + # + #################################### + + private + + def teams_where_admin + @teams_where_admin ||= teams.where(team_roles: { role: 'admin' }) + end end diff --git a/app/views/teams/index.html.erb b/app/views/teams/index.html.erb index 50cd255ef..427b89454 100644 --- a/app/views/teams/index.html.erb +++ b/app/views/teams/index.html.erb @@ -13,6 +13,7 @@ <% t.attribute_column :id, sortable: true %> <% t.attribute_column :name, sortable: true %> + <% t.attribute_column :single_user, title: "Single user team", sortable: true, tooltip: "A single user team cannot have more than one user assigned" %> <% TeamRole::VALID_ROLES.each do |role| %> <% t.custom_column role.pluralize.capitalize do |team| %> <% presenter_for(team).team_users_list(role) %> diff --git a/db/migrate/20240229172848_add_single_user_to_team.rb b/db/migrate/20240229172848_add_single_user_to_team.rb new file mode 100644 index 000000000..31f7ea9ef --- /dev/null +++ b/db/migrate/20240229172848_add_single_user_to_team.rb @@ -0,0 +1,5 @@ +class AddSingleUserToTeam < ActiveRecord::Migration[7.1] + def change + add_column :teams, :single_user, :boolean, default: false, null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index 1b00eb8ab..475307453 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,11 @@ # # It's strongly recommended that you check this file into your version control system. +<<<<<<< HEAD ActiveRecord::Schema[7.1].define(version: 2024_03_04_171606) do +======= +ActiveRecord::Schema[7.1].define(version: 2024_02_29_172848) do +>>>>>>> d9ad4f7 (added creation of user team on signup) # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -265,6 +269,10 @@ t.datetime "deleted_at" t.datetime "created_at", null: false t.datetime "updated_at", null: false +<<<<<<< HEAD +======= + t.boolean "single_user", default: false, null: false +>>>>>>> d9ad4f7 (added creation of user team on signup) t.index ["billing_acct_id"], name: "index_teams_on_billing_acct_id", unique: true, where: "(NOT NULL::boolean)" t.index ["deleted_at"], name: "teams_deleted_at_not_null", where: "(deleted_at IS NOT NULL)" t.index ["deleted_at"], name: "teams_deleted_at_null", where: "(deleted_at IS NULL)" From 3f0ae1522f6b43bd0768e9a07cd3cc8731d7ffd8 Mon Sep 17 00:00:00 2001 From: timalces Date: Fri, 1 Mar 2024 13:46:08 +0000 Subject: [PATCH 03/10] added tests and logic improvements --- app/jobs/create_user_team_job.rb | 10 ++--- app/models/ability.rb | 8 ++++ db/schema.rb | 7 --- spec/jobs/create_user_team_job_spec.rb | 59 ++++++++++++++++++++++++++ spec/jobs/user_signup_job_spec.rb | 17 ++++++++ 5 files changed, 89 insertions(+), 12 deletions(-) create mode 100644 spec/jobs/create_user_team_job_spec.rb diff --git a/app/jobs/create_user_team_job.rb b/app/jobs/create_user_team_job.rb index c931503f9..70944c9f7 100644 --- a/app/jobs/create_user_team_job.rb +++ b/app/jobs/create_user_team_job.rb @@ -1,20 +1,20 @@ -class CreateUserTeamJob < CreateTeamJob +class CreateUserTeamJob < ApplicationJob queue_as :default retry_on ::ActiveModel::ValidationError, wait: :polynomially_longer, attempts: 10 - def perform(user, cloud_service_config, **options) + def perform(user, cloud_service_config) team = nil ActiveRecord::Base.transaction do - team = Team.new(name: "#{user.name}_team", single_user: true) + team = Team.new(name: "#{user.login}_team", single_user: true) unless team.save - logger.info("Unable to create team for #{user.name} #{team.errors.full_messages.join("; ")}") + logger.info("Unable to create team for #{user.login} #{team.errors.full_messages.join("; ")}") raise ActiveModel::ValidationError, team end team_role = TeamRole.new(team: team, user: user, role: "admin") unless team_role.save - logger.info("Unable to create team role for #{user.name} #{team_role.errors.full_messages.join("; ")}") + logger.info("Unable to create team role for #{user.login} #{team_role.errors.full_messages.join("; ")}") logger.info("Rolling back creation of team #{team.name}") raise ActiveModel::ValidationError, team_role end diff --git a/app/models/ability.rb b/app/models/ability.rb index 61c2f9f90..548767ce5 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -27,6 +27,14 @@ def root_abilities # Don't allow any admin users to be deleted. cannot :destroy, User, root: true + + cannot :manage, TeamRole, team_id: Team.where(single_user: true).pluck(:id) + + # Don't allow admins to receive credits + cannot :create, CreditDeposit do |deposit| + user = deposit.user + user.root || user.project_id.nil? || user.billing_acct_id.nil? + end end # Abilities for non-root users. diff --git a/db/schema.rb b/db/schema.rb index 475307453..2a369ea4d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,11 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -<<<<<<< HEAD ActiveRecord::Schema[7.1].define(version: 2024_03_04_171606) do -======= -ActiveRecord::Schema[7.1].define(version: 2024_02_29_172848) do ->>>>>>> d9ad4f7 (added creation of user team on signup) # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -269,10 +265,7 @@ t.datetime "deleted_at" t.datetime "created_at", null: false t.datetime "updated_at", null: false -<<<<<<< HEAD -======= t.boolean "single_user", default: false, null: false ->>>>>>> d9ad4f7 (added creation of user team on signup) t.index ["billing_acct_id"], name: "index_teams_on_billing_acct_id", unique: true, where: "(NOT NULL::boolean)" t.index ["deleted_at"], name: "teams_deleted_at_not_null", where: "(deleted_at IS NOT NULL)" t.index ["deleted_at"], name: "teams_deleted_at_null", where: "(deleted_at IS NULL)" diff --git a/spec/jobs/create_user_team_job_spec.rb b/spec/jobs/create_user_team_job_spec.rb new file mode 100644 index 000000000..2afe1714d --- /dev/null +++ b/spec/jobs/create_user_team_job_spec.rb @@ -0,0 +1,59 @@ +require 'rails_helper' + +RSpec.describe CreateUserTeamJob, type: :job do + include ActiveJob::TestHelper + let(:stubs) { Faraday::Adapter::Test::Stubs.new } + let!(:user) { create(:user, :with_openstack_account, login: "bilbo") } + let(:changes) { {} } + let(:cloud_service_config) { create(:cloud_service_config) } + + subject(:job) { + CreateUserTeamJob.perform_now(user, cloud_service_config) + } + + before(:each) do + clear_enqueued_jobs + clear_performed_jobs + end + + it "creates a single user team with the user's username" do + expect(Team.count).to eq 0 + expect { subject }.to change(Team, :count).by(1) + team = Team.last + expect(team.name).to eq "bilbo_team" + expect(team.single_user).to eq true + end + + it "assigns user as team admin" do + expect(TeamRole.count).to eq 0 + expect { subject }.to change(TeamRole, :count).by(1) + role = TeamRole.last + team = Team.last + expect(role.user).to eq user + expect(role.team).to eq team + expect(role.role).to eq 'admin' + end + + it "rolls back creation of team if user assignment fails" do + expect(TeamRole.count).to eq 0 + expect(Team.count).to eq 0 + user.root = true + subject + expect(TeamRole.count).to eq 0 + expect(Team.count).to eq 0 + + expect(CreateTeamJob).not_to have_been_enqueued + end + + it "enqueues creation of a team in openstack" do + subject + expect(CreateTeamJob).to have_been_enqueued.with(Team.last, cloud_service_config) + end + + it "does not enqueue creation of team in openstack if unsuccessful" do + create(:team, name: "bilbo_team") + expect { subject }.not_to change(Team, :count) + expect(CreateTeamJob).not_to have_been_enqueued + end + +end diff --git a/spec/jobs/user_signup_job_spec.rb b/spec/jobs/user_signup_job_spec.rb index 712f8b9e8..a3b257d57 100644 --- a/spec/jobs/user_signup_job_spec.rb +++ b/spec/jobs/user_signup_job_spec.rb @@ -1,6 +1,7 @@ require 'rails_helper' RSpec.describe UserSignupJob, type: :job do + include ActiveJob::TestHelper let(:stubs) { Faraday::Adapter::Test::Stubs.new } let(:cloud_service_config) { create(:cloud_service_config) } let(:user) { create(:user) } @@ -60,6 +61,14 @@ it "does not update the cloud_user_id" do expect { subject.call rescue nil }.not_to change(user, :cloud_user_id).from(nil) end + + it "does not enqueue user team creation" do + clear_enqueued_jobs + clear_performed_jobs + + subject.call rescue nil + expect(CreateUserTeamJob).not_to have_been_enqueued + end end context "when response contains expected fields" do @@ -74,6 +83,14 @@ expect { subject.call } .to change(user, :cloud_user_id).from(nil).to(cloud_user_id) end + + it "enqueues user team creation" do + clear_enqueued_jobs + clear_performed_jobs + + subject.call + expect(CreateUserTeamJob).to have_been_enqueued.with(user, cloud_service_config) + end end end From c61f86b2643f5db79d68a2039ccd8cef93b65bdf Mon Sep 17 00:00:00 2001 From: timalces Date: Fri, 1 Mar 2024 17:37:45 +0000 Subject: [PATCH 04/10] assign user as admin in openstack --- ..._job.rb => create_single_user_team_job.rb} | 5 +- app/jobs/create_team_then_role_job.rb | 26 +++++ app/jobs/user_signup_job.rb | 2 +- ...rb => create_single_user_team_job_spec.rb} | 10 +- spec/jobs/create_team_job_spec.rb | 107 +---------------- spec/jobs/create_team_then_role_job_spec.rb | 55 +++++++++ spec/jobs/user_signup_job_spec.rb | 4 +- spec/support/shared_examples/jobs.rb | 108 ++++++++++++++++++ 8 files changed, 202 insertions(+), 115 deletions(-) rename app/jobs/{create_user_team_job.rb => create_single_user_team_job.rb} (84%) create mode 100644 app/jobs/create_team_then_role_job.rb rename spec/jobs/{create_user_team_job_spec.rb => create_single_user_team_job_spec.rb} (79%) create mode 100644 spec/jobs/create_team_then_role_job_spec.rb diff --git a/app/jobs/create_user_team_job.rb b/app/jobs/create_single_user_team_job.rb similarity index 84% rename from app/jobs/create_user_team_job.rb rename to app/jobs/create_single_user_team_job.rb index 70944c9f7..5c73b266f 100644 --- a/app/jobs/create_user_team_job.rb +++ b/app/jobs/create_single_user_team_job.rb @@ -1,9 +1,10 @@ -class CreateUserTeamJob < ApplicationJob +class CreateSingleUserTeamJob < ApplicationJob queue_as :default retry_on ::ActiveModel::ValidationError, wait: :polynomially_longer, attempts: 10 def perform(user, cloud_service_config) team = nil + team_role = nil ActiveRecord::Base.transaction do team = Team.new(name: "#{user.login}_team", single_user: true) @@ -20,6 +21,6 @@ def perform(user, cloud_service_config) end end - CreateTeamJob.perform_later(team, cloud_service_config) + CreateTeamThenRoleJob.perform_later(team, team_role, cloud_service_config) end end diff --git a/app/jobs/create_team_then_role_job.rb b/app/jobs/create_team_then_role_job.rb new file mode 100644 index 000000000..b061cce15 --- /dev/null +++ b/app/jobs/create_team_then_role_job.rb @@ -0,0 +1,26 @@ +require 'faraday' + +class CreateTeamThenRoleJob < CreateTeamJob + def perform(team, team_role, cloud_service_config, **options) + runner = Runner.new( + team: team, + team_role: team_role, + cloud_service_config: cloud_service_config, + logger: logger, + **options + ) + runner.call + end + + class Runner < CreateTeamJob::Runner + def initialize(team_role:, **kwargs) + @team_role = team_role + super(**kwargs) + end + + def call + super + CreateTeamRoleJob.perform_later(@team_role, @cloud_service_config) + end + end +end diff --git a/app/jobs/user_signup_job.rb b/app/jobs/user_signup_job.rb index ff9bc3390..e8796ba74 100644 --- a/app/jobs/user_signup_job.rb +++ b/app/jobs/user_signup_job.rb @@ -38,7 +38,7 @@ def call result = Result.from(response.body) result.validate!(:cloud) result.sync(@user, :cloud) - CreateUserTeamJob.perform_later(@user, @cloud_service_config) + CreateSingleUserTeamJob.perform_later(@user, @cloud_service_config) rescue ::ActiveModel::ValidationError @logger.warn("Failed to sync response to user: #{$!.message}") raise diff --git a/spec/jobs/create_user_team_job_spec.rb b/spec/jobs/create_single_user_team_job_spec.rb similarity index 79% rename from spec/jobs/create_user_team_job_spec.rb rename to spec/jobs/create_single_user_team_job_spec.rb index 2afe1714d..67f5a86a2 100644 --- a/spec/jobs/create_user_team_job_spec.rb +++ b/spec/jobs/create_single_user_team_job_spec.rb @@ -1,6 +1,6 @@ require 'rails_helper' -RSpec.describe CreateUserTeamJob, type: :job do +RSpec.describe CreateSingleUserTeamJob, type: :job do include ActiveJob::TestHelper let(:stubs) { Faraday::Adapter::Test::Stubs.new } let!(:user) { create(:user, :with_openstack_account, login: "bilbo") } @@ -8,7 +8,7 @@ let(:cloud_service_config) { create(:cloud_service_config) } subject(:job) { - CreateUserTeamJob.perform_now(user, cloud_service_config) + CreateSingleUserTeamJob.perform_now(user, cloud_service_config) } before(:each) do @@ -42,18 +42,18 @@ expect(TeamRole.count).to eq 0 expect(Team.count).to eq 0 - expect(CreateTeamJob).not_to have_been_enqueued + expect(CreateTeamThenRoleJob).not_to have_been_enqueued end it "enqueues creation of a team in openstack" do subject - expect(CreateTeamJob).to have_been_enqueued.with(Team.last, cloud_service_config) + expect(CreateTeamThenRoleJob).to have_been_enqueued.with(Team.last, TeamRole.last, cloud_service_config) end it "does not enqueue creation of team in openstack if unsuccessful" do create(:team, name: "bilbo_team") expect { subject }.not_to change(Team, :count) - expect(CreateTeamJob).not_to have_been_enqueued + expect(CreateTeamThenRoleJob).not_to have_been_enqueued end end diff --git a/spec/jobs/create_team_job_spec.rb b/spec/jobs/create_team_job_spec.rb index b45d694ca..bc37fbb4d 100644 --- a/spec/jobs/create_team_job_spec.rb +++ b/spec/jobs/create_team_job_spec.rb @@ -7,110 +7,9 @@ subject(:job_runner) { CreateTeamJob::Runner.new(team: team, cloud_service_config: cloud_service_config, test_stubs: stubs) - } +} - describe "url" do - let(:team_service_path) { "/create_team" } - - subject { super().send(:url) } - - it "uses the correct ip, port and path" do - expect(subject).to eq "#{cloud_service_config.user_handler_base_url}#{team_service_path}" - end - end - - describe "body" do - subject { super().send(:body).with_indifferent_access } - - it "contains the team's name" do - expect(subject["name"]).to eq team.name - end - - context "when the team has a project id" do - let(:team) { create(:team, project_id: Faker::Internet.uuid) } - - it "contains the team's project id" do - expect(team.project_id).not_to be_nil - expect(subject["project_id"]).to eq team.project_id - end - end - - context "when the team does not have a project id" do - it "does not contain the team's project id" do - expect(team.project_id).to be_nil - expect(subject).not_to have_key "project_id" - expect(subject).not_to have_key :project_id - end - end - - context "when the team has a billing account id" do - let(:team) { create(:team, billing_acct_id: Faker::Internet.uuid) } - - it "contains the team's billing account id" do - expect(team.billing_acct_id).not_to be_nil - expect(subject["billing_account_id"]).to eq team.billing_acct_id - end - end - - context "when the team does not have a billing account id" do - it "does not contain the team's billing account id" do - expect(team.billing_acct_id).to be_nil - expect(subject).not_to have_key "billing_account_id" - expect(subject).not_to have_key :billing_account_id - end - end - - it "contains the correct cloud environment config" do - expect(subject[:cloud_env]).to eq({ - "auth_url" => cloud_service_config.internal_auth_url, - "user_id" => cloud_service_config.admin_user_id, - "password" => cloud_service_config.admin_foreign_password, - "project_id" => cloud_service_config.admin_project_id - }) - end - end - - describe "updating the team's details from the response" do - let(:team_service_path) { "/create_team" } - context "when response does not contain expected fields" do - let(:response_body) { {} } - - before(:each) do - stubs.post(team_service_path) { |env| [ 201, {}, response_body ] } - end - - it "raises ActiveModel::ValidationError" do - expect { subject.call }.to raise_error ActiveModel::ValidationError - end - - it "does not update the project_id" do - expect { subject.call rescue nil }.not_to change(team, :project_id).from(nil) - end - - it "does not update the billing_acct_id" do - expect { subject.call rescue nil }.not_to change(team, :billing_acct_id).from(nil) - end - end - - context "when response contains expected fields" do - let(:project_id) { SecureRandom.uuid } - let(:billing_acct_id) { SecureRandom.uuid } - let(:response_body) { - {project_id: project_id, billing_account_id: billing_acct_id} - .stringify_keys - } - - before(:each) do - stubs.post(team_service_path) { |env| [ 201, {}, response_body ] } - end - - it "updates the team's project_id and billing_acct_id" do - expect { subject.call } - .to change(team, :project_id).from(nil).to(project_id) - .and change(team, :billing_acct_id).from(nil).to(billing_acct_id) - end - end - end + include_examples 'creating team job' describe "skipping deleted teams" do let(:team) { create(:team, deleted_at: Time.current) } @@ -120,6 +19,4 @@ described_class.perform_now(team, cloud_service_config, test_stubs: stubs) end end - - include_examples 'auth token header' end diff --git a/spec/jobs/create_team_then_role_job_spec.rb b/spec/jobs/create_team_then_role_job_spec.rb new file mode 100644 index 000000000..00d94ec1f --- /dev/null +++ b/spec/jobs/create_team_then_role_job_spec.rb @@ -0,0 +1,55 @@ +require 'rails_helper' + +RSpec.describe CreateTeamThenRoleJob, type: :job do + include ActiveJob::TestHelper + let(:stubs) { Faraday::Adapter::Test::Stubs.new } + let(:cloud_service_config) { create(:cloud_service_config) } + let(:team) { create(:team) } + let(:team_role) { create(:team_role, team: team) } + + subject(:job_runner) { + CreateTeamThenRoleJob::Runner.new(team: team, team_role: team_role, cloud_service_config: cloud_service_config, test_stubs: stubs) + } + + include_examples 'creating team job' + + describe "creating role on success" do + let(:team_service_path) { "/create_team" } + + before(:each) do + clear_enqueued_jobs + clear_performed_jobs + end + + context "when team creation request succeeds" do + let(:project_id) { SecureRandom.uuid } + let(:billing_acct_id) { SecureRandom.uuid } + let(:response_body) { + {project_id: project_id, billing_account_id: billing_acct_id} + .stringify_keys + } + + before(:each) do + stubs.post(team_service_path) { |env| [ 201, {}, response_body ] } + end + + it "enqueues job to create role" do + subject.call + expect(CreateTeamRoleJob).to have_been_enqueued.with(team_role, cloud_service_config) + end + end + + context "when team creation request fails" do + let(:response_body) { {} } + + before(:each) do + stubs.post(team_service_path) { |env| [ 201, {}, response_body ] } + end + + it "does not enqueue role creation" do + subject.call rescue nil + expect(CreateTeamRoleJob).not_to have_been_enqueued + end + end + end +end diff --git a/spec/jobs/user_signup_job_spec.rb b/spec/jobs/user_signup_job_spec.rb index a3b257d57..3c622c513 100644 --- a/spec/jobs/user_signup_job_spec.rb +++ b/spec/jobs/user_signup_job_spec.rb @@ -67,7 +67,7 @@ clear_performed_jobs subject.call rescue nil - expect(CreateUserTeamJob).not_to have_been_enqueued + expect(CreateSingleUserTeamJob).not_to have_been_enqueued end end @@ -89,7 +89,7 @@ clear_performed_jobs subject.call - expect(CreateUserTeamJob).to have_been_enqueued.with(user, cloud_service_config) + expect(CreateSingleUserTeamJob).to have_been_enqueued.with(user, cloud_service_config) end end end diff --git a/spec/support/shared_examples/jobs.rb b/spec/support/shared_examples/jobs.rb index 8dafc6fba..2b23fda88 100644 --- a/spec/support/shared_examples/jobs.rb +++ b/spec/support/shared_examples/jobs.rb @@ -12,3 +12,111 @@ expect(decoded["exp"]).to eq decoded["iat"] + 60 end end + +RSpec.shared_examples "creating team job" do + describe "url" do + let(:team_service_path) { "/create_team" } + + subject { super().send(:url) } + + it "uses the correct ip, port and path" do + expect(subject).to eq "#{cloud_service_config.user_handler_base_url}#{team_service_path}" + end + end + + describe "body" do + subject { super().send(:body).with_indifferent_access } + + it "contains the team's name" do + expect(subject["name"]).to eq team.name + end + + context "when the team has a project id" do + let(:team) { create(:team, project_id: Faker::Internet.uuid) } + + it "contains the team's project id" do + expect(team.project_id).not_to be_nil + expect(subject["project_id"]).to eq team.project_id + end + end + + context "when the team does not have a project id" do + it "does not contain the team's project id" do + expect(team.project_id).to be_nil + expect(subject).not_to have_key "project_id" + expect(subject).not_to have_key :project_id + end + end + + context "when the team has a billing account id" do + let(:team) { create(:team, billing_acct_id: Faker::Internet.uuid) } + + it "contains the team's billing account id" do + expect(team.billing_acct_id).not_to be_nil + expect(subject["billing_account_id"]).to eq team.billing_acct_id + end + end + + context "when the team does not have a billing account id" do + it "does not contain the team's billing account id" do + expect(team.billing_acct_id).to be_nil + expect(subject).not_to have_key "billing_account_id" + expect(subject).not_to have_key :billing_account_id + end + end + + it "contains the correct cloud environment config" do + expect(subject[:cloud_env]).to eq({ + "auth_url" => cloud_service_config.internal_auth_url, + "user_id" => cloud_service_config.admin_user_id, + "password" => cloud_service_config.admin_foreign_password, + "project_id" => cloud_service_config.admin_project_id + }) + end + end + + describe "updating the team's details from the response" do + let(:team_service_path) { "/create_team" } + context "when response does not contain expected fields" do + let(:response_body) { {} } + + before(:each) do + stubs.post(team_service_path) { |env| [ 201, {}, response_body ] } + end + + it "raises ActiveModel::ValidationError" do + expect { subject.call }.to raise_error ActiveModel::ValidationError + end + + it "does not update the project_id" do + expect { subject.call rescue nil }.not_to change(team, :project_id).from(nil) + end + + it "does not update the billing_acct_id" do + expect { subject.call rescue nil }.not_to change(team, :billing_acct_id).from(nil) + end + end + + context "when response contains expected fields" do + let(:project_id) { SecureRandom.uuid } + let(:billing_acct_id) { SecureRandom.uuid } + let(:response_body) { + {project_id: project_id, billing_account_id: billing_acct_id} + .stringify_keys + } + + before(:each) do + stubs.post(team_service_path) { |env| [ 201, {}, response_body ] } + end + + it "updates the team's project_id and billing_acct_id" do + expect { subject.call } + .to change(team, :project_id).from(nil).to(project_id) + .and change(team, :billing_acct_id).from(nil).to(billing_acct_id) + end + end + end + + include_examples 'auth token header' +end + From d4be75092d9b00119a72206404d2d99750c9484a Mon Sep 17 00:00:00 2001 From: timalces Date: Fri, 1 Mar 2024 18:27:40 +0000 Subject: [PATCH 05/10] added team role validation for single user team --- app/models/team_role.rb | 7 +++++++ spec/models/team_role_spec.rb | 35 +++++++++++++++++++++++++---------- 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/app/models/team_role.rb b/app/models/team_role.rb index b8bef7148..e6610f32c 100644 --- a/app/models/team_role.rb +++ b/app/models/team_role.rb @@ -46,6 +46,7 @@ def self.perform_search(term, search_scope = default_searchable_columns) validates :user_id, uniqueness: { scope: :team_id, message: "User can only have one role per team" } validate :user_not_root + validate :one_role_for_single_user_team ###################################### # @@ -77,6 +78,12 @@ def user_not_root self.errors.add(:user, 'must not be super admin') if user&.root? end + def one_role_for_single_user_team + if team&.single_user && team.team_roles.where.not(id: id).exists? + self.errors.add(:team, 'is a single user team and already has an assigned user') + end + end + # What user can see in irv may have changed def broadcast_change BroadcastUserRacksJob.perform_now(self.user_id) diff --git a/spec/models/team_role_spec.rb b/spec/models/team_role_spec.rb index cf637c13a..65d66f62f 100644 --- a/spec/models/team_role_spec.rb +++ b/spec/models/team_role_spec.rb @@ -28,9 +28,31 @@ end end - it "is not valid without a team" do - subject.team = nil - expect(subject).to have_error(:team, :blank) + describe "team" do + it "is not valid without a team" do + subject.team = nil + expect(subject).to have_error(:team, :blank) + end + + it "must be a unique user team combination" do + new_role = build(:team_role, team: subject.team, user: subject.user) + expect(new_role).to have_error(:user_id, :taken) + new_role.team = team + expect(new_role).to be_valid + end + + it "allows multiple roles for a regular team" do + new_role = build(:team_role, team: subject.team) + expect(new_role).to be_valid + end + + it "does not allow multiple roles for a single user team" do + subject.team.single_user = true + subject.team.save! + new_role = build(:team_role, team: subject.team) + expect(new_role).to have_error(:team, "is a single user team and already has an assigned user") + expect(subject).to be_valid + end end describe "role" do @@ -44,12 +66,5 @@ expect(subject).to have_error(:role, :inclusion) end end - - it "must be a unique user team combination" do - new_role = build(:team_role, team: subject.team, user: subject.user) - expect(new_role).to have_error(:user_id, :taken) - new_role.team = team - expect(new_role).to be_valid - end end end From c19f2760989296bd2ec2f623c369c7af93c5766a Mon Sep 17 00:00:00 2001 From: timalces Date: Tue, 5 Mar 2024 16:39:44 +0000 Subject: [PATCH 06/10] updated migration post rebase --- ..._user_to_team.rb => 20240304171607_add_single_user_to_team.rb} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename db/migrate/{20240229172848_add_single_user_to_team.rb => 20240304171607_add_single_user_to_team.rb} (100%) diff --git a/db/migrate/20240229172848_add_single_user_to_team.rb b/db/migrate/20240304171607_add_single_user_to_team.rb similarity index 100% rename from db/migrate/20240229172848_add_single_user_to_team.rb rename to db/migrate/20240304171607_add_single_user_to_team.rb From be30fb3d9db21e032ab723d650a149b2f296f331 Mon Sep 17 00:00:00 2001 From: timalces Date: Tue, 5 Mar 2024 18:07:25 +0000 Subject: [PATCH 07/10] more rebase fixes --- app/models/ability.rb | 6 ------ app/models/user.rb | 8 -------- db/schema.rb | 2 +- 3 files changed, 1 insertion(+), 15 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index 548767ce5..1f3ededeb 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -29,12 +29,6 @@ def root_abilities cannot :destroy, User, root: true cannot :manage, TeamRole, team_id: Team.where(single_user: true).pluck(:id) - - # Don't allow admins to receive credits - cannot :create, CreditDeposit do |deposit| - user = deposit.user - user.root || user.project_id.nil? || user.billing_acct_id.nil? - end end # Abilities for non-root users. diff --git a/app/models/user.rb b/app/models/user.rb index 82da36ba1..b27877319 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -125,14 +125,6 @@ def mark_as_pending_deletion allowlisted_jwts.destroy_all end - #################################### - # - # Private Instance Methods - # - #################################### - - private - def teams_where_admin @teams_where_admin ||= teams.where(team_roles: { role: 'admin' }) end diff --git a/db/schema.rb b/db/schema.rb index 2a369ea4d..bd0591240 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.1].define(version: 2024_03_04_171606) do +ActiveRecord::Schema[7.1].define(version: 2024_03_04_171607) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" From 21c28d76d51bccd030b55012029db64df9af7c06 Mon Sep 17 00:00:00 2001 From: timalces Date: Thu, 7 Mar 2024 17:54:27 +0000 Subject: [PATCH 08/10] don't initial create job --- app/jobs/create_single_user_team_job.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/jobs/create_single_user_team_job.rb b/app/jobs/create_single_user_team_job.rb index 5c73b266f..76e9328ca 100644 --- a/app/jobs/create_single_user_team_job.rb +++ b/app/jobs/create_single_user_team_job.rb @@ -1,6 +1,5 @@ class CreateSingleUserTeamJob < ApplicationJob queue_as :default - retry_on ::ActiveModel::ValidationError, wait: :polynomially_longer, attempts: 10 def perform(user, cloud_service_config) team = nil @@ -9,13 +8,13 @@ def perform(user, cloud_service_config) ActiveRecord::Base.transaction do team = Team.new(name: "#{user.login}_team", single_user: true) unless team.save - logger.info("Unable to create team for #{user.login} #{team.errors.full_messages.join("; ")}") + logger.info("Unable to create team for #{user.login} #{team.errors.details}") raise ActiveModel::ValidationError, team end team_role = TeamRole.new(team: team, user: user, role: "admin") unless team_role.save - logger.info("Unable to create team role for #{user.login} #{team_role.errors.full_messages.join("; ")}") + logger.info("Unable to create team role for #{user.login} #{team_role.errors.details}") logger.info("Rolling back creation of team #{team.name}") raise ActiveModel::ValidationError, team_role end From 1f0bba0587ef38c37c1ca37e1bd1d87f7d36f4ef Mon Sep 17 00:00:00 2001 From: timalces Date: Thu, 7 Mar 2024 17:55:00 +0000 Subject: [PATCH 09/10] ability and test improvements --- app/models/ability.rb | 4 ++-- spec/jobs/create_single_user_team_job_spec.rb | 10 ++-------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index 1f3ededeb..4ae518784 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -28,7 +28,7 @@ def root_abilities # Don't allow any admin users to be deleted. cannot :destroy, User, root: true - cannot :manage, TeamRole, team_id: Team.where(single_user: true).pluck(:id) + cannot :manage, TeamRole, team: Team.where(single_user: true) end # Abilities for non-root users. @@ -53,7 +53,7 @@ def non_root_abilities can [:read, :update], User, id: @user.id can :read, Team, id: @user.team_ids - can :manage, TeamRole, team_id: @user.teams_where_admin.where(single_user: false).pluck(:id) + can :manage, TeamRole, team: @user.teams_where_admin.where(single_user: false) # Invoice is an ActiveModel::Model, but not an ActiveRecord::Base. Setting # abilities like this might not work too well. Or perhaps its fine. diff --git a/spec/jobs/create_single_user_team_job_spec.rb b/spec/jobs/create_single_user_team_job_spec.rb index 67f5a86a2..fd1cc61d5 100644 --- a/spec/jobs/create_single_user_team_job_spec.rb +++ b/spec/jobs/create_single_user_team_job_spec.rb @@ -17,7 +17,6 @@ end it "creates a single user team with the user's username" do - expect(Team.count).to eq 0 expect { subject }.to change(Team, :count).by(1) team = Team.last expect(team.name).to eq "bilbo_team" @@ -25,7 +24,6 @@ end it "assigns user as team admin" do - expect(TeamRole.count).to eq 0 expect { subject }.to change(TeamRole, :count).by(1) role = TeamRole.last team = Team.last @@ -35,12 +33,8 @@ end it "rolls back creation of team if user assignment fails" do - expect(TeamRole.count).to eq 0 - expect(Team.count).to eq 0 user.root = true - subject - expect(TeamRole.count).to eq 0 - expect(Team.count).to eq 0 + expect { subject rescue nil }.to not_change(Team, :count).and not_change(TeamRole, :count) expect(CreateTeamThenRoleJob).not_to have_been_enqueued end @@ -52,7 +46,7 @@ it "does not enqueue creation of team in openstack if unsuccessful" do create(:team, name: "bilbo_team") - expect { subject }.not_to change(Team, :count) + expect { subject rescue nil }.not_to change(Team, :count) expect(CreateTeamThenRoleJob).not_to have_been_enqueued end From b5b8869be0646e1a346a0747c5e74390d99601d8 Mon Sep 17 00:00:00 2001 From: timalces Date: Thu, 7 Mar 2024 17:55:18 +0000 Subject: [PATCH 10/10] highlight if team is user's personal team --- app/presenters/team_presenter.rb | 8 ++++++++ app/views/teams/index.html.erb | 8 ++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/app/presenters/team_presenter.rb b/app/presenters/team_presenter.rb index a3610cac4..87f707317 100644 --- a/app/presenters/team_presenter.rb +++ b/app/presenters/team_presenter.rb @@ -1,6 +1,10 @@ class TeamPresenter < Presenter include Costed + def name(user) + personal_team_for_user?(user) ? "#{o.name} (your personal team)" : o.name + end + def status if o.deleted_at.nil? "Active" @@ -49,4 +53,8 @@ def form_hint(attribute) I18n.t("simple_form.customisations.hints.team.edit.#{attribute}.present") end end + + def personal_team_for_user?(user) + o.single_user && !user.root && user.teams_where_admin.where(id: o.id).exists? + end end diff --git a/app/views/teams/index.html.erb b/app/views/teams/index.html.erb index 427b89454..ee96746e8 100644 --- a/app/views/teams/index.html.erb +++ b/app/views/teams/index.html.erb @@ -12,8 +12,12 @@ <% end %> <% t.attribute_column :id, sortable: true %> - <% t.attribute_column :name, sortable: true %> - <% t.attribute_column :single_user, title: "Single user team", sortable: true, tooltip: "A single user team cannot have more than one user assigned" %> + <% t.custom_column "Name", sortable: true, db_column: :name do |team| %> + <% presenter_for(team).name(current_user) %> + <% end %> + <% if current_user.root? %> + <% t.attribute_column :single_user, title: "Single user team", sortable: true, tooltip: "A single user team cannot have more than one user assigned" %> + <% end %> <% TeamRole::VALID_ROLES.each do |role| %> <% t.custom_column role.pluralize.capitalize do |team| %> <% presenter_for(team).team_users_list(role) %>