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

@@ -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
 

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

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!(

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

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 protected]"
+      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}")

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

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

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])

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])