From eec31da471a7a776633cb1500b60c131f74e0636 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADtor=20Avelino?= Date: Wed, 4 Oct 2017 11:33:22 -0300 Subject: [PATCH] Fixed the namespace and team name clashes Currently we have a problem when a user is removed and added back to Portus. The team and namespace associated with that user are not removed. So, when an admin tries to create a user or an user tries to create an account, an error mentioning that there's already a namespace with that username is shown. With this patch we avoid that by adding an incremental number to the end of the name. For example, if username is "brian" and there's already a namespace/team with that name, "brian0" namespace and team will be created instead. If "brian" gets removed again, next time it will be "brian1" for both namespace and team. Remembering that we should remove this once we implement the namespace/team removal. --- app/models/namespace.rb | 25 ++++++++++++----- app/models/team.rb | 19 +++++++++++++ app/models/user.rb | 14 +++------- app/policies/namespace_policy.rb | 4 +-- spec/features/admin/users_spec.rb | 37 ++++++++++++++++++++++++++ spec/models/namespace_spec.rb | 10 +++++++ spec/models/team_spec.rb | 21 +++++++++++++-- spec/models/user_spec.rb | 33 ----------------------- spec/policies/namespace_policy_spec.rb | 22 +++++++-------- 9 files changed, 120 insertions(+), 65 deletions(-) diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 26999d3bd..919b8f3cc 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -93,16 +93,16 @@ def self.get_from_name(name, registry = nil) [namespace, name, registry] end - # Tries to transform the given name to a valid namespace name. If the name is - # already valid, then it's returned untouched. Otherwise, if the name cannot - # be turned into a valid namespace name, then nil is returned. + # Tries to transform the given name to a valid namespace name without + # clashing with existent namespaces. + # If the name cannot be turned into a valid namespace name, + # then nil is returned. + # If the name is valid, checks if it clashes with others namespaces and + # finds one until it's not being used and returns it. def self.make_valid(name) - return name if name =~ NAME_REGEXP - # One common case is LDAP and case sensitivity. With this in mind, try to # downcase everything and see if now it's fine. name = name.downcase - return name if name =~ NAME_REGEXP # Let's strip extra characters from the beginning and end. first = name.index(/[a-z0-9]/) @@ -120,6 +120,19 @@ def self.make_valid(name) name = final[0..MAX_NAME_LENGTH] return nil if name !~ NAME_REGEXP + + # To avoid any name conflict we append an incremental number to the end + # of the name returns it as the name that will be used on both Namespace + # and Team on the User#create_personal_namespace! method + # TODO: workaround until we implement the namespace/team removal + increment = 0 + original_name = name + while Namespace.exists?(name: name) + name = "#{original_name}#{increment}" + increment += 1 + break if increment > 1000 + end + name end diff --git a/app/models/team.rb b/app/models/team.rb index 013c74e2d..1a9579660 100644 --- a/app/models/team.rb +++ b/app/models/team.rb @@ -51,4 +51,23 @@ def member_ids def self.search_from_query(valid_teams, query) all_non_special.where(id: valid_teams).where(arel_table[:name].matches(query)) end + + # Tries to transform the given name to a valid team name without + # clashing with existent teams. + # Checks if it clashes with others teams and finds one until it's not + # being used and returns it. + def self.make_valid(name) + # To avoid any name conflict we append an incremental number to the end + # of the name returns it as the name that will be used on both Namespace + # and Team on the User#create_personal_namespace! method + # TODO: workaround until we implement the namespace/team removal + increment = 0 + original_name = name + while Team.exists?(name: name) + name = "#{original_name}#{increment}" + increment += 1 + end + + name + end end diff --git a/app/models/user.rb b/app/models/user.rb index 95df774e2..6268d5860 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -86,13 +86,6 @@ def private_namespace_and_team_available if ns.nil? errors.add(:username, "'#{username}' cannot be transformed into a " \ "valid namespace name") - elsif Namespace.exists?(name: ns) - clar = (ns != username) ? " (modified so it's valid)" : "" - errors.add(:username, "cannot be used: there is already a namespace " \ - "named '#{ns}'#{clar}") - elsif Team.exists?(name: username) - errors.add(:username, "cannot be used: there is already a team named " \ - "like this") end end @@ -117,13 +110,14 @@ def create_personal_namespace! # Leave early if the namespace already exists. This is fine because the # `private_namespace_and_team_available` method has already checked that # the name of the namespace is fine and that it doesn't clash. + return unless namespace_id.nil? + namespace_name = Namespace.make_valid(username) - ns = Namespace.find_by(name: namespace_name) - return ns if ns + team_name = Team.make_valid(username) # Note that this shouldn't be a problem since the User controller will make # sure that we don't create a user that clashes with this team. - team = Team.create!(name: username, owners: [self], hidden: true) + team = Team.create!(name: team_name, owners: [self], hidden: true) default_description = "This personal namespace belongs to #{username}." namespace = Namespace.find_or_create_by!( diff --git a/app/policies/namespace_policy.rb b/app/policies/namespace_policy.rb index b0288c2fa..01f0e6f20 100644 --- a/app/policies/namespace_policy.rb +++ b/app/policies/namespace_policy.rb @@ -80,10 +80,10 @@ def resolve .where( "(namespaces.visibility = :public OR namespaces.visibility = :protected " \ "OR team_users.user_id = :user_id) AND " \ - "namespaces.global = :global AND namespaces.name != :username", + "namespaces.global = :global AND namespaces.id != :namespace_id", public: Namespace.visibilities[:visibility_public], protected: Namespace.visibilities[:visibility_protected], - user_id: user.id, global: false, username: user.username + user_id: user.id, global: false, namespace_id: user.namespace_id ) .distinct end diff --git a/spec/features/admin/users_spec.rb b/spec/features/admin/users_spec.rb index f6f8b17fe..cf634b845 100644 --- a/spec/features/admin/users_spec.rb +++ b/spec/features/admin/users_spec.rb @@ -10,6 +10,43 @@ visit admin_users_path end + describe "create users", js: true do + scenario "admin creates a user" do + visit new_admin_user_path + + fill_in "Username", with: "username" + fill_in "Email", with: "email@email.com" + fill_in "user[password]", with: "password123" + fill_in "Password confirmation", with: "password123" + + click_button "Create" + + expect(page).to have_current_path(admin_users_path) + expect(page).to have_content("User 'username' was created successfully") + end + + scenario "admin adds back a removed user" do + expect(page).to have_css("#user_#{user.id}") + + find("#user_#{user.id} .remove-btn").click + find("#user_#{user.id} .btn-confirm-remove").click + + expect(page).to have_content("User '#{user.username}' was removed successfully") + + visit new_admin_user_path + + fill_in "Username", with: user.username + fill_in "Email", with: user.email + fill_in "user[password]", with: "password123" + fill_in "Password confirmation", with: "password123" + + click_button "Create" + + expect(page).to have_current_path(admin_users_path) + expect(page).to have_content("User '#{user.username}' was created successfully") + end + end + describe "remove users" do scenario "allows the admin to remove other users", js: true do expect(page).to have_css("#user_#{user.id}") diff --git a/spec/models/namespace_spec.rb b/spec/models/namespace_spec.rb index 656c6f7c9..0ee231272 100644 --- a/spec/models/namespace_spec.rb +++ b/spec/models/namespace_spec.rb @@ -159,6 +159,9 @@ end describe "make_valid" do + let!(:team) { create(:team) } + let!(:namespace) { create(:namespace, team: team) } + it "does nothing on already valid names" do ["name", "a", "a_a", "45", "n4", "h2o", "flavio.castelli"].each do |name| expect(Namespace.make_valid(name)).to eq name @@ -184,5 +187,12 @@ expect(Namespace.make_valid("M")).to eq "m" expect(Namespace.make_valid("_M_")).to eq "m" end + + it "adds an increment if a team with the name already exists" do + expect(Namespace.make_valid(namespace.name)).to eq "#{namespace.name}0" + + create(:namespace, name: "#{namespace.name}0", team: team) + expect(Namespace.make_valid(namespace.name)).to eq "#{namespace.name}1" + end end end diff --git a/spec/models/team_spec.rb b/spec/models/team_spec.rb index ef0ef9173..ce3940cb8 100644 --- a/spec/models/team_spec.rb +++ b/spec/models/team_spec.rb @@ -42,9 +42,26 @@ expect(Team.count).to be(3) # Personal namespaces don't count. - user = create(:user) - user.create_personal_namespace! + create(:user) expect(Team.all_non_special.count).to be(1) expect(Team.count).to be(4) end + + describe "make_valid" do + it "does nothing if there's no team with the name" do + name = "something" + + expect(Team.make_valid(name)).to eq name + end + + it "adds an increment if a team with the name already exists" do + name = "something" + + create(:team, name: name) + expect(Team.make_valid(name)).to eq "#{name}0" + + create(:team, name: "#{name}0") + expect(Team.make_valid(name)).to eq "#{name}1" + end + end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index e50698cca..956b735d5 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -54,39 +54,6 @@ "valid namespace name"]) end - it "adds an error if the namespace name clashes" do - name = "coolname" - team = create(:team, owners: [subject]) - create(:namespace, team: team, name: name) - - user = build(:user, username: name) - expect(user.save).to be false - expect(user.errors.size).to eq(1) - expect(user.errors.first) - .to match_array([:username, "cannot be used: there is already a " \ - "namespace named 'coolname'"]) - - user = build(:user, username: name + "_") - expect(user.save).to be false - expect(user.errors.size).to eq(1) - expect(user.errors.first) - .to match_array([:username, "cannot be used: there is already a " \ - "namespace named 'coolname' (modified so it's valid)"]) - end - - it "adds an error if there's a team named like that" do - name = "coolname" - team = create(:team, name: name, owners: [subject]) - create(:namespace, team: team, name: "somethingelse") - - user = build(:user, username: name) - expect(user.save).to be false - expect(user.errors.size).to eq(1) - expect(user.errors.first) - .to match_array([:username, "cannot be used: there is already a " \ - "team named like this"]) - end - it "works beautifully if everything was fine" do name = "coolname" team = create(:team, owners: [subject]) diff --git a/spec/policies/namespace_policy_spec.rb b/spec/policies/namespace_policy_spec.rb index 69f53209c..542d73126 100644 --- a/spec/policies/namespace_policy_spec.rb +++ b/spec/policies/namespace_policy_spec.rb @@ -7,6 +7,7 @@ let(:owner) { create(:user) } let(:viewer) { create(:user) } let(:contributor) { create(:user) } + let(:registry) { create(:registry) } let(:team) do create(:team, owners: [owner], @@ -17,14 +18,13 @@ create( :namespace, description: "short test description.", - registry: @registry, + registry: registry, team: team ) end before :each do @admin = create(:admin) - @registry = create(:registry) end permissions :pull? do @@ -54,7 +54,7 @@ end it "always allows access to a global namespace" do - expect(subject).to permit(create(:user), @registry.global_namespace) + expect(subject).to permit(user, registry.global_namespace) end it "disallows access to a non-logged user if the namespace is private" do @@ -109,11 +109,11 @@ context "global namespace" do it "allows access to administrators" do - expect(subject).to permit(@admin, @registry.global_namespace) + expect(subject).to permit(@admin, registry.global_namespace) end it "denies access to other users" do - expect(subject).not_to permit(user, @registry.global_namespace) + expect(subject).not_to permit(user, registry.global_namespace) end end end @@ -137,11 +137,11 @@ context "global namespace" do it "allows access to administrators" do - expect(subject).to permit(@admin, @registry.global_namespace) + expect(subject).to permit(@admin, registry.global_namespace) end it "denies access to other users" do - expect(subject).not_to permit(user, @registry.global_namespace) + expect(subject).not_to permit(user, registry.global_namespace) end end end @@ -192,20 +192,19 @@ expected = team.namespaces expect(Pundit.policy_scope(viewer, Namespace).to_a).to match_array(expected) - expect(Pundit.policy_scope(create(:user), Namespace).to_a).to be_empty + expect(Pundit.policy_scope(user, Namespace).to_a).to be_empty end it "always shows public namespaces" do n = create(:namespace, visibility: :visibility_public) create(:team, namespaces: [n], owners: [owner]) - expect(Pundit.policy_scope(create(:user), Namespace).to_a).to match_array([n]) + expect(Pundit.policy_scope(user, Namespace).to_a).to match_array([n]) end it "never shows public or personal namespaces" do - user = create(:user) expect(Namespace.find_by(name: user.username)).not_to be_nil create(:namespace, global: true, visibility: :visibility_public) - expect(Pundit.policy_scope(create(:user), Namespace).to_a).to be_empty + expect(Pundit.policy_scope(user, Namespace).to_a).to be_empty end it "does not show duplicates" do @@ -216,7 +215,6 @@ end it "shows protected namespaces" do - user = create(:user) n = create(:namespace, visibility: :visibility_protected) create(:team, namespaces: [n], owners: [owner]) expect(Pundit.policy_scope(user, Namespace)).to match_array([n])