Removed the usage of ldap_name and relax username

This has been done in two steps:

1. First of all I've introduced the `namespace_id` column to the `users`
   table. This means that from now own there is no mapping from users and their
   personal namespaces through the `username` column, from now on it's done with
   a plain SQL foreign key.
2. From the previous step, we get that restricting the username is no longer
   needed. Instead, the format of the username has been greatly relaxed, so the
   only concerns are:
    - It is unique in the DB.
    - We can produce a namespace for it. If the username is not a valid
      namespace name, then the resulting namespace will have its name altered
      so it can be valid. If it cannot be valid, then user creation will fail
      (which is very unlikely: this only happens in cases like "?" or something
       like that).

Fixes #732
Fixes #736

Signed-off-by: Miquel Sabaté Solà <msabate@suse.com>

app/controllers/auth/registrations_controller.rb

@@ -2,6 +2,7 @@ class Auth::RegistrationsController < Devise::RegistrationsController
   layout "authentication", except: :edit
 
   include CheckLDAP
+  include SessionFlash
 
   before_action :check_signup, only: [:new, :create]
   before_action :check_admin, only: [:new, :create]
@@ -20,7 +21,7 @@ def create
 
     resource.save
     if resource.persisted?
-      set_flash_message :notice, :signed_up
+      session_flash(resource, :signed_up)
       sign_up(resource_name, resource)
       respond_with resource, location: after_sign_up_path_for(resource), float: true
     else

app/controllers/auth/sessions_controller.rb

@@ -1,4 +1,6 @@
 class Auth::SessionsController < Devise::SessionsController
+  include SessionFlash
+
   layout "authentication"
 
   # Re-implementing. The logic is: if there is already a user that can log in
@@ -23,7 +25,13 @@ def new
 
   def create
     super
-    flash[:notice] = nil
+
+    if ::Portus::LDAP.enabled? && session[:first_login]
+      session[:first_login] = nil
+      session_flash(current_user, nil)
+    else
+      flash[:notice] = nil
+    end
   end
 
   def destroy

app/controllers/concerns/session_flash.rb

@@ -0,0 +1,33 @@
+# SessionFlash adds the `session_flash` method which deals with flashy messages
+# on signup/login, while notifying users about their personal namespace.
+module SessionFlash
+  extend ActiveSupport::Concern
+
+  # Sets the flash object accordingly for the given authenticated user. The
+  # method is the Devise method to be used for greeting the user (e.g.
+  # `:signed_up`). If method is nil, then a generic greeting will be set. This
+  # method also notifies users about their personal namespace (and whether it
+  # changed or not).
+  def session_flash(user, method)
+    # First of all we've got a greeting.
+    if method.nil?
+      flash[:notice] = "Welcome!"
+    else
+      set_flash_message :notice, method unless method.nil?
+    end
+
+    # This will happen for the first user, which is the admin that has to
+    # configure the registry.
+    return if user.namespace.nil?
+
+    # Now inform the user
+    ns = user.namespace.name
+    str = " Your personal namespace is '#{ns}'"
+    if user.username == ns
+      str += "."
+    else
+      str += " (your username was not a valid Docker namespace, so we had to tweak it)."
+    end
+    flash[:notice] << str
+  end
+end

app/controllers/dashboard_controller.rb

@@ -7,7 +7,7 @@ def index
 
     # The personal namespace could not exist, that happens when portus
     # does not have a registry associated yet (right after the initial setup)
-    personal_namespace = Namespace.find_by(name: current_user.username)
+    personal_namespace = current_user.namespace
     @personal_repositories = personal_namespace ? personal_namespace.repositories : []
 
     @stars = current_user.stars.order("updated_at desc")

app/models/namespace.rb

@@ -23,6 +23,13 @@
 class Namespace < ActiveRecord::Base
   include PublicActivity::Common
 
+  # This regexp is extracted from the reference package of Docker Distribution
+  # and it matches a valid namespace name.
+  NAME_REGEXP = /\A[a-z0-9]+(?:[._\\-][a-z0-9]+)*\Z/
+
+  # The maximum length of a namespace name.
+  MAX_NAME_LENGTH = 255
+
   has_many :webhooks
   has_many :repositories
   belongs_to :registry
@@ -32,7 +39,7 @@ class Namespace < ActiveRecord::Base
   validates :name,
             presence:   true,
             uniqueness: { scope: "registry_id" },
-            length:     { maximum: 255 },
+            length:     { maximum: MAX_NAME_LENGTH },
             namespace:  true
 
   # From the given repository name that can be prefix by the name of the
@@ -58,6 +65,31 @@ 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.
+  def self.make_valid(name)
+    return name if name =~ NAME_REGEXP
+
+    # First of all we strip extra characters from the beginning and end.
+    first = name.index(/[a-z0-9]/)
+    return nil if first.nil?
+    last = name.rindex(/[a-z0-9]/)
+    str = name[first..last]
+
+    # Replace weird characters with underscores.
+    str = str.gsub(/[^[a-z0-9\\.\\-_]]/, "_")
+
+    # Only one special character is allowed in between of alphanumeric
+    # characters. Thus, let's merge multiple appearences into one on each case.
+    # After that, the name should be fine, so let's trim it if it's too large.
+    final = str.gsub(/[._\\-]{2,}/, "_")
+    name = final[0..MAX_NAME_LENGTH]
+
+    return nil if name !~ NAME_REGEXP
+    name
+  end
+
   # Returns a String containing the cleaned name for this namespace. The
   # cleaned name will be the registry's hostname if this is a global namespace,
   # or the name of the namespace itself otherwise.

app/models/user.rb

@@ -22,12 +22,13 @@
 #  failed_attempts        :integer          default("0")
 #  locked_at              :datetime
 #  display_name           :string(255)
+#  namespace_id           :integer
 #
 # Indexes
 #
 #  index_users_on_display_name          (display_name) UNIQUE
 #  index_users_on_email                 (email) UNIQUE
-#  index_users_on_ldap_name             (ldap_name) UNIQUE
+#  index_users_on_namespace_id          (namespace_id)
 #  index_users_on_reset_password_token  (reset_password_token) UNIQUE
 #  index_users_on_username              (username) UNIQUE
 #
@@ -36,22 +37,14 @@ class User < ActiveRecord::Base
   devise :database_authenticatable, :registerable, :lockable,
          :recoverable, :rememberable, :trackable, :validatable, authentication_keys: [:username]
 
-  USERNAME_CHARS  = "a-z0-9"
-  USERNAME_FORMAT = /\A[#{USERNAME_CHARS}]{4,30}\Z/
-
   APPLICATION_TOKENS_MAX = 5
 
-  validates :username, presence: true, uniqueness: true,
-    format: {
-      with:    USERNAME_FORMAT,
-      message: "only lower case alphanumeric characters are allowed. "\
-        "Minimum 4 characters, maximum 30."
-    }
-
   # Actions performed before/after create.
+  validates :username, presence: true, uniqueness: true
   validate :private_namespace_and_team_available, on: :create
   after_create :create_personal_namespace!
 
+  belongs_to :namespace
   has_many :team_users
   has_many :teams, through: :team_users
   has_many :stars
@@ -64,14 +57,24 @@ class User < ActiveRecord::Base
   # Special method used by Devise to require an email on signup. This is always
   # true except for LDAP.
   def email_required?
-    !(Portus::LDAP.enabled? && !ldap_name.nil?)
+    !(Portus::LDAP.enabled? && email.blank?)
   end
 
   # It adds an error if the username clashes with either a namespace or a team.
   def private_namespace_and_team_available
-    return unless Namespace.exists?(name: username) || Team.exists?(name: username)
-    errors.add(:username, "'#{username}' cannot be used: there's either a "\
-      "namespace or a team named like this.")
+    ns = Namespace.make_valid(username)
+
+    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
 
   # Returns true if the current user is the Portus user.
@@ -92,30 +95,30 @@ def create_personal_namespace!
     # the registry is not configured yet, we cannot create the namespace
     return unless Registry.any?
 
-    # Leave early if the namespace already exists.
-    ns = Namespace.find_by(name: username)
+    # 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.
+    namespace_name = Namespace.make_valid(username)
+    ns = Namespace.find_by(name: namespace_name)
     return ns if ns
 
     # 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)
 
     default_description = "This personal namespace belongs to #{username}."
-    Namespace.find_or_create_by!(
+    namespace = Namespace.find_or_create_by!(
       team:        team,
-      name:        username,
+      name:        namespace_name,
       description: default_description,
       registry:    Registry.get # TODO: fix once we handle more registries
     )
+    update_attributes(namespace: namespace)
   end
 
   # Find the user that can be guessed from the given push event.
   def self.find_from_event(event)
-    if Portus::LDAP.enabled?
-      actor = User.find_by(ldap_name: event["actor"]["name"])
-    else
-      actor = User.find_by(username: event["actor"]["name"])
-    end
+    actor = User.find_by(username: event["actor"]["name"])
     logger.error "Cannot find user #{event["actor"]["name"]}" if actor.nil?
     actor
   end

app/views/devise/registrations/edit.html.slim

@@ -78,8 +78,8 @@
                       any affiliations with any team will be lost.
                   = submit_tag('Disable', class: 'btn btn-primary')
 
-  - if current_user.email?
-    - if current_user.ldap_name.nil?
+  - unless ::Portus::LDAP.enabled?
+    - if current_user.email?
       .col-sm-6
         .panel.panel-default
           .panel-heading

db/migrate/20160510153011_add_namespace_id_to_users.rb

@@ -0,0 +1,5 @@
+class AddNamespaceIdToUsers < ActiveRecord::Migration
+  def change
+    add_reference :users, :namespace, index: true, foreign_key: true, default: nil
+  end
+end

db/migrate/20160526105216_remove_index_on_ldap_users.rb

@@ -0,0 +1,5 @@
+class RemoveIndexOnLdapUsers < ActiveRecord::Migration
+  def change
+    remove_index "users", name: "index_users_on_ldap_name"
+  end
+end

db/schema.rb

@@ -165,12 +165,13 @@
     t.string   "ldap_name",              limit: 255
     t.integer  "failed_attempts",        limit: 4,   default: 0
     t.datetime "locked_at"
+    t.integer  "namespace_id",           limit: 4
     t.string   "display_name",           limit: 255
   end
 
   add_index "users", ["display_name"], name: "index_users_on_display_name", unique: true, using: :btree
   add_index "users", ["email"], name: "index_users_on_email", unique: true, using: :btree
-  add_index "users", ["ldap_name"], name: "index_users_on_ldap_name", unique: true, using: :btree
+  add_index "users", ["namespace_id"], name: "index_users_on_namespace_id", using: :btree
   add_index "users", ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true, using: :btree
   add_index "users", ["username"], name: "index_users_on_username", unique: true, using: :btree
 
@@ -217,6 +218,7 @@
   add_foreign_key "comments", "repositories"
   add_foreign_key "stars", "repositories"
   add_foreign_key "stars", "users"
+  add_foreign_key "users", "namespaces"
   add_foreign_key "webhook_deliveries", "webhooks"
   add_foreign_key "webhook_headers", "webhooks"
   add_foreign_key "webhooks", "namespaces"

lib/portus/ldap.rb

@@ -32,13 +32,9 @@ def authenticate!
       if @ldap
         # Try to bind to the LDAP server. If there's any failure, the
         # authentication process will fail without going to the any other
-        # strategy.
-        if @ldap.bind_as(bind_options)
-          user = find_or_create_user!
-          user.valid? ? success!(user) : fail!(user.errors.full_messages.join(","))
-        else
-          fail!(:ldap_bind_failed)
-        end
+        # strategy. Otherwise, login the current user into Portus (and create
+        # it if it doesn't exist).
+        @ldap.bind_as(bind_options) ? portus_login! : fail!(:ldap_bind_failed)
       else
         # rubocop:disable Style/SignalException
         fail(:ldap_failed)
@@ -148,53 +144,37 @@ def fill_user_params!
       params[:user] = { username: user, password: pass }
     end
 
+    # Fetch the user assumed from `params` and log it. If the user does not
+    # exist yet, it will be created and the `session[:first_login]` value will
+    # be set to true, so the sessions controller can act accordingly.
+    def portus_login!
+      user, created = find_or_create_user!
+      if user.valid?
+        session[:first_login] = true if created
+        success!(user)
+      else
+        fail!(user.errors.full_messages.join(","))
+      end
+    end
+
     # Retrieve the given user as an LDAP user. If it doesn't exist, create it
-    # with the parameters given in the form.
+    # with the parameters given in the form. Returns two objects: the user
+    # object and a boolean set to true if the returned user was just created.
     def find_or_create_user!
-      user = User.find_by(ldap_name: username)
+      user = User.find_by(username: username)
+      created = false
 
-      # The user does not exist in Portus yet, let's create it. If it does
-      # not match a valid username, it will be transformed into a proper one.
+      # The user does not exist in Portus yet, let's create it.
       unless user
-        ldap_name = username.dup
-        if User::USERNAME_FORMAT.match(ldap_name)
-          name = ldap_name
-        else
-          name = ldap_name.gsub(/[^#{User::USERNAME_CHARS}]/, "")
-        end
-
-        # This is to check that no clashes occur. This is quite improbable to
-        # happen, since it would mean that the name contains characters like
-        # "$", "!", etc. We also check that the name is longer than 4
-        # (requirement from Docker).
-        if name.length < 4 || User.exists?(username: name)
-          name = generate_random_name(name)
-        end
-
         user = User.create(
-          username:  name,
-          email:     guess_email,
-          password:  password,
-          admin:     !User.not_portus.any?,
-          ldap_name: ldap_name
+          username: username,
+          email:    guess_email,
+          password: password,
+          admin:    !User.not_portus.any?
         )
+        created = user.persisted?
       end
-      user
-    end
-
-    # It generates a new name that doesn't clash with any of the existing ones.
-    def generate_random_name(name)
-      # Even if the name has just one character, adding a number of at least
-      # three digits would make the name valid.
-      offset = name.length < 4 ? 100 : 0
-
-      10.times do
-        nn = "#{name}#{Random.rand(offset + 101)}"
-        return nn unless User.exists?(username: nn)
-      end
-
-      # We have not been able to generate a new name, let's raise an exception.
-      fail!(:random_generation_failed)
+      [user, created]
     end
 
     # If the "ldap.guess_email" option is enabled, try to guess the email for