Check for name conflicts between stageusers and users

Check if the username conflicts with an existing user. The check
is not perfect. A user might be created before the stage user is
activated. The code also suffers from a race condition. It's as
good as it can get without a better API, though.

The commit also improves the error reporting code to report multiple
errors at once. A user doesn't have to 'probe' for errors until she
succeeds. It improves the user experience.

closes #10

see https://fedorahosted.org/freeipa/ticket/5186

freeipa_community_portal/app.py

@@ -63,10 +63,12 @@ def GET(self): # pylint: disable=invalid-name
 
     def POST(self, **kwargs): # pylint: disable=invalid-name
         """POST /user"""
+        errors = []
         user = User(kwargs)
-        errors = check_captcha(kwargs)
-        if not errors: 
-            errors = user.save()
+        errors.extend(user.validate())
+        errors.extend(check_captcha(kwargs))
+        if not errors:
+            errors.extend(user.save())
         if not errors:
             # email the admin that the user has signed up
             SignUpMailer(user).mail()
@@ -95,11 +97,14 @@ def GET(self):
 
     def POST(self, **kwargs):
         """accepts a username and initiates a reset"""
-        errors = check_captcha(kwargs)
-        if not errors and not kwargs['username']:
-            errors = "Username is required"
+        errors = []
+        if not kwargs['username']:
+            errors.append("Username is required.")
+        errors.extend(check_captcha(kwargs))
         if errors:
-            return render('request_reset.html', errors=errors, captcha=captcha_helper.CaptchaHelper())
+            return render('request_reset.html',
+                          errors=errors,
+                          captcha=captcha_helper.CaptchaHelper())
         r = PasswordReset(kwargs['username'])
         r.save()
         if r.check_valid():
@@ -147,9 +152,9 @@ def render(template, **args):
 
 def check_captcha(args):
     if not captcha_helper.checkResponse(args['response'], args['solution']):
-        return "Incorrect Captcha response"
+        return ["Incorrect Captcha response"]
     else:
-        return None
+        return []
 
 
 conf = {

freeipa_community_portal/model/user.py

@@ -41,6 +41,20 @@ def __init__(self, args=None):
             self.username = self.given_name[0] + self.family_name
         self.email = args.get("email", "")
 
+    def validate(self):
+        err = []
+        if not self.given_name:
+            err.append('Given name is required.')
+        if not self.family_name:
+            err.append('Family name is required.')
+        if not self.email or "@" not in self.email:
+            err.append("Invalid email address.")
+        try:
+            self.check_available()
+        except errors.DuplicateEntry as exc:
+            err.append(exc.msg)
+        return err
+
     def save(self):
         """Save the model
 
@@ -50,12 +64,43 @@ def save(self):
         If there are validation errors, returns the error message. Otherwise
         returns None
         """
-        error = None
+        err = []
         try:
             self._call_api()
-        except (errors.ValidationError, errors.RequirementError, errors.DuplicateEntry) as err:
-            error = err.msg
-        return error
+        except (errors.ValidationError, errors.RequirementError, errors.DuplicateEntry) as exc:
+            err.append(exc.msg)
+        return err
+
+    def check_available(self):
+        """Checks if the user name is still available.
+
+        For a better user experience, this check is done in advanced.
+        """
+        api_connect()
+
+        message = 'active user with name "%(user)s" already exists' % {
+            'user': self.username
+        }
+
+        # Check if the username conflicts with an existing user. The check
+        # is not perfect. A user might be created before the stage user is
+        # activated. The code also suffers from a race condition. It's as
+        # good as it can get without a better API, though.
+        # see https://fedorahosted.org/freeipa/ticket/5186
+        try:
+            api.Command.user_show(uid=self.username)
+        except errors.NotFound:
+            pass
+        else:
+            raise errors.DuplicateEntry(message=message)
+
+        try:
+            api.Command.stageuser_show(uid=self.username)
+        except errors.NotFound:
+            pass
+        else:
+            raise errors.DuplicateEntry(message=message)
+
 
     def _call_api(self):
         """performs the actual API call. seperate method for testing purposes
@@ -66,5 +111,5 @@ def _call_api(self):
             givenname=self.given_name,
             sn=self.family_name,
             uid=self.username,
-            mail=self.email
+            mail=self.email,
         )

freeipa_community_portal/templates/new_user.html

@@ -7,9 +7,11 @@ <h2>Create a new user account</h2>
 
     {% if errors %}
     <div class="row">
-        <div class="alert alert-danger col-sm-offset-2 col-sm-6">
-            <p>{{ errors }}</p>
-        </div>
+        <ul class="alert alert-danger col-sm-offset-2 col-sm-6">
+            {% for error in errors %}
+                <li>{{ error }}</li>
+            {% endfor %}
+        </ul>
     </div>
     {% endif %}
 

freeipa_community_portal/templates/request_reset.html

@@ -8,9 +8,11 @@ <h2>Request a Password Reset</h2>
 
     {% if errors %}
     <div class="row">
-        <div class="alert alert-danger col-sm-offset-2 col-sm-6">
-            <p>{{ errors }}</p>
-        </div>
+        <ul class="alert alert-danger col-sm-offset-2 col-sm-6">
+            {% for error in errors %}
+                <li>{{ error }}</li>
+            {% endfor %}
+        </ul>
     </div>
     {% endif %}