>From b52fc32df961684a2d6c7c41e6a5dbab7827f688 Mon Sep 17 00:00:00 2001
From: Pavel Raiskup <praiskup@redhat.com>
Date: Wed, 20 Aug 2014 22:50:37 +0200
Subject: [PATCH 1/2] [frontend] do not strictly resist on Fedora's OpenID

To allow users use multiple authentication options concurrently
(e.g. simply password/hash auth in future & Fedora OpenID), we
need to keep single and unique username in our database.  To allow
that, transform the 'openid_name' column from 'user' table into
simple 'username'.
---
 .../bd0a15c7b6f_dont_require_fed_openid.py         | 66 ++++++++++++++++++++++
 .../coprs_frontend/coprs/logic/builds_logic.py     |  3 +-
 frontend/coprs_frontend/coprs/logic/coprs_logic.py |  8 +--
 frontend/coprs_frontend/coprs/models.py            | 31 ++++------
 frontend/coprs_frontend/coprs/views/misc.py        | 21 ++++++-
 frontend/coprs_frontend/tests/coprs_test_case.py   | 12 ++--
 .../test_views/test_admin/test_admin_general.py    |  4 +-
 7 files changed, 108 insertions(+), 37 deletions(-)
 create mode 100644 frontend/coprs_frontend/alembic/versions/bd0a15c7b6f_dont_require_fed_openid.py

diff --git a/frontend/coprs_frontend/alembic/versions/bd0a15c7b6f_dont_require_fed_openid.py b/frontend/coprs_frontend/alembic/versions/bd0a15c7b6f_dont_require_fed_openid.py
new file mode 100644
index 0000000..d6e121f
--- /dev/null
+++ b/frontend/coprs_frontend/alembic/versions/bd0a15c7b6f_dont_require_fed_openid.py
@@ -0,0 +1,66 @@
+"""Do not require Fedora's OpenID in DB schema
+
+Revision ID: bd0a15c7b6f
+Revises: 5055336e8c44
+Create Date: 2014-08-20 21:49:05.299819
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = 'bd0a15c7b6f'
+down_revision = '5055336e8c44'
+
+from alembic import op
+import sqlalchemy as sa
+import logging
+
+logger = logging.getLogger('alembic')
+
+metadata = sa.MetaData()
+
+def username_default(oid_user):
+    return oid_user.replace(".id.fedoraproject.org/", "") \
+                   .replace("http://", "")
+
+def upgrade():
+    op.add_column(u'user', sa.Column('username', sa.String(length=100)))
+
+    sa_user = sa.Table("user", metadata,
+            sa.Column("id", sa.Integer),
+            sa.Column("openid_name", sa.String(length=100)),
+            sa.Column("username", sa.String(length=100))
+    )
+
+    for u in op.get_bind().execute(sa.select([sa_user.c.id, sa_user.c.openid_name])):
+        username = username_default(u[1])
+        logger.info("converting {0}'s account".format(username))
+        op.get_bind().execute(sa_user.update() \
+                .where(sa_user.c.id==u[0]) \
+                .values(username=username))
+
+    if op.get_bind().dialect.name != 'sqlite':
+        # Pretty sad we can not set this non-nullable in SQLite
+        op.alter_column("user", "username", nullable=False)
+        # We can live with redundant openid_name column..
+        op.drop_column(u'user', u'openid_name')
+
+
+def downgrade():
+    op.add_column(u'user', sa.Column(u'openid_name', sa.VARCHAR(length=100), nullable=True))
+
+    sa_user = sa.Table("user", metadata,
+            sa.Column("id", sa.Integer),
+            sa.Column("openid_name", sa.String(length=100)),
+            sa.Column("username", sa.String(length=100))
+    )
+
+    for u in op.get_bind().execute(sa.select([sa_user.c.id, sa_user.c.username])):
+        openid_name = "http://{0}.id.fedoraproject.org/".format(str(u[1]))
+        op.get_bind().execute(sa_user.update() \
+                .where(sa_user.c.id==u[0]) \
+                .values(openid_name=openid_name)
+        )
+
+    if op.get_bind().dialect.name != 'sqlite':
+        op.drop_column(u'user', 'username')
+        op.alter_column("user", "openid_name", nullable=False)
diff --git a/frontend/coprs_frontend/coprs/logic/builds_logic.py b/frontend/coprs_frontend/coprs/logic/builds_logic.py
index 26567b4..2172116 100644
--- a/frontend/coprs_frontend/coprs/logic/builds_logic.py
+++ b/frontend/coprs_frontend/coprs/logic/builds_logic.py
@@ -68,8 +68,7 @@ class BuildsLogic(object):
                      .options(db.contains_eager(models.Build.copr))
                      .join(models.Copr.owner)
                      .filter(models.Copr.name == coprname)
-                     .filter(models.User.openid_name ==
-                             models.User.openidize_name(username))
+                     .filter(models.User.username == username)
                      .order_by(models.Build.submitted_on.desc()))
         else:
             raise exceptions.ArgumentMissingException(
diff --git a/frontend/coprs_frontend/coprs/logic/coprs_logic.py b/frontend/coprs_frontend/coprs/logic/coprs_logic.py
index 54efefe..cbd5880 100644
--- a/frontend/coprs_frontend/coprs/logic/coprs_logic.py
+++ b/frontend/coprs_frontend/coprs/logic/coprs_logic.py
@@ -32,8 +32,7 @@ class CoprsLogic(object):
 
         query = (cls.get_all()
                  .filter(models.Copr.name == coprname)
-                 .filter(models.User.openid_name ==
-                         models.User.openidize_name(username))
+                 .filter(models.User.username == username)
                  )
 
         if with_builds:
@@ -72,7 +71,7 @@ class CoprsLogic(object):
 
         if user_relation == "owned":
             query = query.filter(
-                models.User.openid_name == models.User.openidize_name(username))
+                models.User.username == username)
         elif user_relation == "allowed":
             aliased_user = db.aliased(models.User)
 
@@ -81,8 +80,7 @@ class CoprsLogic(object):
                      .filter(models.CoprPermission.copr_builder ==
                              helpers.PermissionEnum('approved'))
                      .join(aliased_user, models.CoprPermission.user)
-                     .filter(aliased_user.openid_name ==
-                             models.User.openidize_name(username)))
+                     .filter(aliased_user.username == username))
 
         if with_mock_chroots:
             query = (query.outerjoin(*models.Copr.mock_chroots.attr)
diff --git a/frontend/coprs_frontend/coprs/models.py b/frontend/coprs_frontend/coprs/models.py
index e30b819..420f49c 100644
--- a/frontend/coprs_frontend/coprs/models.py
+++ b/frontend/coprs_frontend/coprs/models.py
@@ -7,25 +7,31 @@ from coprs import constants
 from coprs import db
 from coprs import helpers
 
-
 class User(db.Model, helpers.Serializer):
 
     """
     Represents user of the copr frontend
     """
 
+    # PK;  TODO: the 'username' could be also PK
     id = db.Column(db.Integer, primary_key=True)
-    # openid_name for fas, e.g. http://bkabrda.id.fedoraproject.org/
-    openid_name = db.Column(db.String(100), nullable=False)
-    # just mail :)
+
+    # unique username
+    username = db.Column(db.String(100), nullable=False, unique=True)
+
+    # email
     mail = db.Column(db.String(150), nullable=False)
-    # just timezone ;)
+
+    # optional timezone
     timezone = db.Column(db.String(50), nullable=True)
+
     # is this user proven? proven users can modify builder memory and
     # timeout for single builds
     proven = db.Column(db.Boolean, default=False)
+
     # is this user admin of the system?
     admin = db.Column(db.Boolean, default=False)
+
     # stuff for the cli interface
     api_login = db.Column(db.String(40), nullable=False, default="abc")
     api_token = db.Column(db.String(40), nullable=False, default="abc")
@@ -38,8 +44,7 @@ class User(db.Model, helpers.Serializer):
         Return the short username of the user, e.g. bkabrda
         """
 
-        return self.openid_name.replace(
-            ".id.fedoraproject.org/", "").replace("http://", "")
+        return self.username
 
     def permissions_for_copr(self, copr):
         """
@@ -87,17 +92,6 @@ class User(db.Model, helpers.Serializer):
 
         return can_edit
 
-    @classmethod
-    def openidize_name(cls, name):
-        """
-        Create proper openid_name from short name.
-
-        >>> user.openid_name == User.openidize_name(user.name)
-        True
-        """
-
-        return "http://{0}.id.fedoraproject.org/".format(name)
-
     @property
     def serializable_attributes(self):
         # enumerate here to prevent exposing credentials
@@ -124,7 +118,6 @@ class User(db.Model, helpers.Serializer):
         except IOError:
             return ""
 
-
 class Copr(db.Model, helpers.Serializer):
 
     """
diff --git a/frontend/coprs_frontend/coprs/views/misc.py b/frontend/coprs_frontend/coprs/views/misc.py
index c557511..0d84df7 100644
--- a/frontend/coprs_frontend/coprs/views/misc.py
+++ b/frontend/coprs_frontend/coprs/views/misc.py
@@ -10,13 +10,27 @@ from coprs import helpers
 from coprs import models
 from coprs import oid
 
+def fed_openidize_name(name):
+    """
+    Create proper Fedora OpenID name from short name.
+
+    >>> fedoraoid == fed_openidize_name(user.name)
+    True
+    """
+
+    return "http://{0}.id.fedoraproject.org/".format(name)
+
+def fed_raw_name(oidname):
+    return oidname.replace(".id.fedoraproject.org/", "") \
+                  .replace("http://", "")
 
 @app.before_request
 def lookup_current_user():
     flask.g.user = None
     if "openid" in flask.session:
+        username = fed_raw_name(flask.session["openid"])
         flask.g.user = models.User.query.filter(
-            models.User.openid_name == flask.session["openid"]).first()
+            models.User.username == username).first()
 
 
 @app.errorhandler(404)
@@ -48,15 +62,16 @@ def create_or_login(resp):
                          and fasusername in app.config["ALLOWED_USERS"])
                         or not app.config["USE_ALLOWED_USERS"]):
 
+        username = fed_raw_name(resp.identity_url)
         user = models.User.query.filter(
-            models.User.openid_name == resp.identity_url).first()
+            models.User.username == username).first()
         if not user:  # create if not created already
             expiration_date_token = datetime.date.today() + \
                 datetime.timedelta(
                     days=flask.current_app.config["API_TOKEN_EXPIRATION"])
 
             copr64 = base64.b64encode("copr") + "##"
-            user = models.User(openid_name=resp.identity_url, mail=resp.email,
+            user = models.User(username=username, mail=resp.email,
                                timezone=resp.timezone,
                                api_login=copr64 + helpers.generate_api_token(
                                    app.config["API_TOKEN_LENGTH"] - len(copr64)),
diff --git a/frontend/coprs_frontend/tests/coprs_test_case.py b/frontend/coprs_frontend/tests/coprs_test_case.py
index 179555c..54fe7fe 100644
--- a/frontend/coprs_frontend/tests/coprs_test_case.py
+++ b/frontend/coprs_frontend/tests/coprs_test_case.py
@@ -48,18 +48,18 @@ class CoprsTestCase(object):
     @pytest.fixture
     def f_users(self):
         self.u1 = models.User(
-            openid_name=u"http://user1.id.fedoraproject.org/",
+            username=u"user1",
             proven=False,
             admin=True,
             mail="user1@foo.bar")
 
         self.u2 = models.User(
-            openid_name=u"http://user2.id.fedoraproject.org/",
+            username=u"user2",
             proven=False,
             mail="user2@spam.foo")
 
         self.u3 = models.User(
-            openid_name=u"http://user3.id.fedoraproject.org/",
+            username=u"user3",
             proven=False,
             mail="baz@bar.bar")
 
@@ -198,7 +198,7 @@ class TransactionDecorator(object):
 
     with self.tc as test_client:
         with c.session_transaction() as session:
-            session['openid'] = self.u.openid_name
+            session['openid'] = self.u.username
 
     where 'u' stands for any user from 'f_users' fixture, use this to decorate
     your test function:
@@ -209,7 +209,7 @@ class TransactionDecorator(object):
         # you can also access object 'test_client' through 'self.test_client'
 
     where decorator parameter ''u'' stands for string representation of any
-    user from 'f_users' fixture from which you wish to store 'openid_name'.
+    user from 'f_users' fixture from which you wish to store 'username'.
     Please note that you **must** include 'f_users' fixture in decorated
     function parameters.
 
@@ -223,6 +223,6 @@ class TransactionDecorator(object):
         def wrapper(fn, fn_self, *args):
             with fn_self.tc as fn_self.test_client:
                 with fn_self.test_client.session_transaction() as session:
-                    session["openid"] = getattr(fn_self, self.user).openid_name
+                    session["openid"] = getattr(fn_self, self.user).username
                 return fn(fn_self, *args)
         return decorator.decorator(wrapper, fn)
diff --git a/frontend/coprs_frontend/tests/test_views/test_admin/test_admin_general.py b/frontend/coprs_frontend/tests/test_views/test_admin/test_admin_general.py
index 60eba07..7489374 100644
--- a/frontend/coprs_frontend/tests/test_views/test_admin/test_admin_general.py
+++ b/frontend/coprs_frontend/tests/test_views/test_admin/test_admin_general.py
@@ -9,7 +9,7 @@ class TestAdminLogin(CoprsTestCase):
     def test_nonadmin_cant_login(self, f_users, f_db):
         with self.tc as c:
             with c.session_transaction() as s:
-                s["openid"] = self.u2.openid_name
+                s["openid"] = self.u2.username
 
         r = c.get("/admin/", follow_redirects=True)
         assert self.text_to_check not in r.data
@@ -17,7 +17,7 @@ class TestAdminLogin(CoprsTestCase):
     def test_admin_can_login(self, f_users, f_db):
         with self.tc as c:
             with c.session_transaction() as s:
-                s["openid"] = self.u1.openid_name
+                s["openid"] = self.u1.username
 
         r = c.get("/admin/", follow_redirects=True)
         assert self.text_to_check in r.data
-- 
1.9.3

