Handle some of the error cases with github login.

data/model.py

@@ -15,18 +15,52 @@ class DataModelException(Exception):
   pass
 
 
-def create_user(username, password, email):
-  pw_hash = bcrypt.hashpw(password, bcrypt.gensalt())
+class InvalidEmailAddressException(DataModelException):
+  pass
 
+
+class InvalidUsernameException(DataModelException):
+  pass
+
+
+class InvalidPasswordException(DataModelException):
+  pass
+
+
+def create_user(username, password, email):
   if not validate_email(email):
-    raise DataModelException('Invalid email address: %s' % email)
+    raise InvalidEmailAddressException('Invalid email address: %s' % email)
   if not validate_username(username):
-    raise DataModelException('Invalid username: %s' % username)
-  if not validate_password(password):
-    raise DataModelException('Invalid password, password must be at least ' +
-                             '8 characters and contain no whitespace.')
+    raise InvalidUsernameException('Invalid username: %s' % username)
+
+  # We allow password none for the federated login case.
+  if password is not None and not validate_password(password):
+    raise InvalidPasswordException('Invalid password, password must be at ' + 
+                                   'least 8 characters and contain no ' + 
+                                   'whitespace.')
 
   try:
+    existing = User.get((User.username == username) | (User.email == email))
+
+    logger.debug('Existing user with same username or email.')
+
+    # A user already exists with either the same username or email
+    if existing.username == username:
+      raise InvalidUsernameException('Username has already been taken: %s' %
+                                     username)
+    raise InvalidEmailAddressException('Email has already been used: %s' %
+                                       email)
+
+  except User.DoesNotExist:
+    # This is actually the happy path
+    logger.debug('Email and username are unique!')
+    pass
+
+  try:
+    pw_hash = None
+    if password is not None:
+      pw_hash = bcrypt.hashpw(password, bcrypt.gensalt())
+
     new_user = User.create(username=username, password_hash=pw_hash,
                            email=email)
     return new_user
@@ -35,18 +69,16 @@ def create_user(username, password, email):
 
 
 def create_federated_user(username, email, service_name, service_id):
-  try:
-    new_user = User.create(username=username, email=email, verified=True)
+    new_user = create_user(username, None, email)
+    new_user.verified = True
+    new_user.save()
+
     service = LoginService.get(LoginService.name == service_name)
     federated_user = FederatedLogin.create(user=new_user, service=service,
                                            service_ident=service_id)
 
     return new_user
 
-  except Exception as ex:
-    raise DataModelException(ex.message)
-
-
 def verify_federated_login(service_name, service_id):
   selected = FederatedLogin.select(FederatedLogin, User)
   with_service = selected.join(LoginService)
@@ -98,7 +130,9 @@ def verify_user(username, password):
   except User.DoesNotExist:
     return None
 
-  if bcrypt.hashpw(password, fetched.password_hash) == fetched.password_hash:
+  if (fetched.password_hash and 
+      bcrypt.hashpw(password, fetched.password_hash) ==
+      fetched.password_hash):
     return fetched
 
   # We weren't able to authorize the user

endpoints/api.py

@@ -72,13 +72,8 @@ def create_user_api():
     send_confirmation_email(new_user.username, new_user.email, code.code)
     return make_response('Created', 201)
   except model.DataModelException as ex:
-    message = ex.message
-    m = re.search('column ([a-zA-Z]+) is not unique', message)
-    if m and m.group(1):
-      message = m.group(1) + ' already exists'
-
     error_resp = jsonify({
-      'message': message,
+      'message': ex.message,
     })
     error_resp.status_code = 400
     return error_resp

endpoints/web.py

@@ -135,14 +135,18 @@ def github_oauth_callback():
   to_login = model.verify_federated_login('github', github_id)
   if not to_login:
     # try to create the user
-    to_login = model.create_federated_user(username, found_email, 'github',
-                                           github_id)
+
+    try:
+      to_login = model.create_federated_user(username, found_email, 'github',
+                                             github_id)
+    except model.DataModelException, ex:
+      return render_template('githuberror.html', error_message=ex.message)
 
   if common_login(to_login):
     return redirect(url_for('index'))
 
   # TODO something bad happened, we need to tell the user somehow
-  return redirect(url_for('signin'))
+  return render_template('githuberror.html')
 
 
 @app.route('/confirm', methods=['GET'])

templates/githuberror.html

@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <title>Error Logging in with GitHub - Quay</title>
+
+    <link rel="stylesheet" href="//netdna.bootstrapcdn.com/bootstrap/3.0.0/css/bootstrap.no-icons.min.css">
+    <link rel="stylesheet" href="//netdna.bootstrapcdn.com/font-awesome/3.2.1/css/font-awesome.min.css">
+
+    <link rel="stylesheet" href="static/css/signin.css">
+  </head>
+  <body>
+    <div class="container">
+      <div class="row">
+        <div class="col-md-12">
+          <h2>There was an error logging in with GitHub.</h2>
+
+          {% if error_message %}
+            <div class="alert alert-danger">{{ error_message }}</div>
+          {% endif %}
+
+          <div>
+            Please register using the <a href="/">registration form</a> to continue.
+          </div>
+        </div>
+      </div>
+
+    </div>
+  </body>
+</html>

templates/signin.html

@@ -17,7 +17,7 @@
 
         <span class="social-alternate">
           <i class="icon-circle"></i>
-          <span class="inner-text">OR</i>
+          <span class="inner-text">OR</span>
         </span>
 
         <a href="https://github.com/login/oauth/authorize?client_id={{ github_client_id }}&scope=user:email" class="btn btn-primary btn-lg btn-block"><i class="icon-github icon-large"></i> Sign In with GitHub</a>