Merge lp:~adamhjk/swift/lp607912 into lp:~hudson-openstack/swift/trunk

[Adam Jacob]

Fixes lp:607912. We now do a SELECT to find out if we have an account with the matching username - if we do, we grab the cfaccount hash from that row, and re-use it when we update the record. The corresponding INSERT is changed to an INSERT OR REPLACE, which will ensure that repeat creations of the same user with a new password will get the correct behavior.

[lp:~adamhjk/swift/lp607912]

37. By Adam Jacob <adam@latte> on 2010-07-21

Removing errant debug statement

[Chuck Thier (cthier)]

Hey Adam,

I just tested this, and creating the same user still adds extra entries in the db:

cthier@swiftchuck:~/swift/lp607912$ swift-auth-create-account test tester testing
http://127.0.0.1:8080/v1/4db8017a-68d1-485a-98dd-b94785a2f855
cthier@swiftchuck:~/swift/lp607912$ swift-auth-create-account test tester testing
http://127.0.0.1:8080/v1/4db8017a-68d1-485a-98dd-b94785a2f855
cthier@swiftchuck:~/swift/lp607912$ swift-auth-create-account test tester testing2
http://127.0.0.1:8080/v1/4db8017a-68d1-485a-98dd-b94785a2f855
cthier@swiftchuck:~/swift/lp607912$ sqlite3 /etc/swift/auth.db
SQLite version 3.6.22
Enter ".help" for instructions
Enter SQL statements terminated with a ";"
sqlite> select * from account;
test|http://127.0.0.1:8080/v1/4db8017a-68d1-485a-98dd-b94785a2f855|4db8017a-68d1-485a-98dd-b94785a2f855|tester|testing
test|http://127.0.0.1:8080/v1/4db8017a-68d1-485a-98dd-b94785a2f855|4db8017a-68d1-485a-98dd-b94785a2f855|tester|testing
test|http://127.0.0.1:8080/v1/4db8017a-68d1-485a-98dd-b94785a2f855|4db8017a-68d1-485a-98dd-b94785a2f855|tester|testing2

I think that the "INSERT OR REPLACE" sql isn't working the way you are expecting to. I wold recommend doing an "UPDATE" instead if it finds the account_hash.

[Adam Jacob (adamhjk)]

Ah, yeah, I should look at the schema. I'll fox.

Sent from my iPhone

On Jul 21, 2010, at 7:41 PM, Chuck Thier <email address hidden> wrote:

> Hey Adam,
>
> I just tested this, and creating the same user still adds extra entries in the db:
>
> cthier@swiftchuck:~/swift/lp607912$ swift-auth-create-account test tester testing
> http://127.0.0.1:8080/v1/4db8017a-68d1-485a-98dd-b94785a2f855
> cthier@swiftchuck:~/swift/lp607912$ swift-auth-create-account test tester testing
> http://127.0.0.1:8080/v1/4db8017a-68d1-485a-98dd-b94785a2f855
> cthier@swiftchuck:~/swift/lp607912$ swift-auth-create-account test tester testing2
> http://127.0.0.1:8080/v1/4db8017a-68d1-485a-98dd-b94785a2f855
> cthier@swiftchuck:~/swift/lp607912$ sqlite3 /etc/swift/auth.db
> SQLite version 3.6.22
> Enter ".help" for instructions
> Enter SQL statements terminated with a ";"
> sqlite> select * from account;
> test|http://127.0.0.1:8080/v1/4db8017a-68d1-485a-98dd-b94785a2f855|4db8017a-68d1-485a-98dd-b94785a2f855|tester|testing
> test|http://127.0.0.1:8080/v1/4db8017a-68d1-485a-98dd-b94785a2f855|4db8017a-68d1-485a-98dd-b94785a2f855|tester|testing
> test|http://127.0.0.1:8080/v1/4db8017a-68d1-485a-98dd-b94785a2f855|4db8017a-68d1-485a-98dd-b94785a2f855|tester|testing2
>
> I think that the "INSERT OR REPLACE" sql isn't working the way you are expecting to. I wold recommend doing an "UPDATE" instead if it finds the account_hash.
>
> --
> https://code.launchpad.net/~adamhjk/swift/lp607912/+merge/30588
> You are the owner of lp:~adamhjk/swift/lp607912.

[lp:~adamhjk/swift/lp607912]

38. By Adam Jacob <adam@latte> on 2010-07-26

Moving to direct INSERT or UPDATE

Unmerged revisions

38. By Adam Jacob <adam@latte> on 2010-07-26

Moving to direct INSERT or UPDATE

37. By Adam Jacob <adam@latte> on 2010-07-21

Removing errant debug statement

36. By Adam Jacob <adam@latte> on 2010-07-21

Re-creates existing users on multiple calls - fixes lp:607912

35. By Adam Jacob <adam@latte> on 2010-07-21

Adding test for existing account behavior

34. By Adam Jacob <adam@latte> on 2010-07-21

Select first to preserve the account hash

33. By Adam Jacob <adam@latte> on 2010-07-21

Creating users with INSERT OR REPLACE

=== modified file 'swift/auth/server.py'
--- swift/auth/server.py	2010-07-25 16:15:40 +0000
+++ swift/auth/server.py	2010-07-26 17:48:48 +0000
@@ -254,12 +254,27 @@
         :param new_user: The name for the new user
         :param new_password: The password for the new account
 
-        :returns: False if the create fails, storage url if successful
+        :returns: False if the create fails or the user exists, storage url if successful
         """
         begin = time()
         if not all((new_account, new_user, new_password)):
             return False
-        account_hash = self.add_storage_account()
+       
+        account_hash = False
+        update_or_insert = "insert"
+        with self.get_conn() as conn:
+            row = conn.execute('''SELECT cfaccount FROM account WHERE
+                                   account = ? AND user = ?''',
+                         (new_account, new_user)).fetchone()
+            if row:
+                update_or_insert = "update"
+                account_hash = row[0]
+
+        if not account_hash:
+            account_hash = self.add_storage_account()
+        else:
+            account_hash = self.add_storage_account(account_hash)
+
         if not account_hash:
             self.logger.info(
                 'FAILED create_account(%s, %s, _,) [%.02f]' %
@@ -267,11 +282,17 @@
             return False
         url = self.default_cluster_url.rstrip('/') + '/' + account_hash
         with self.get_conn() as conn:
-            conn.execute('''INSERT INTO account
-                (account, url, cfaccount, user, password)
-                VALUES (?, ?, ?, ?, ?)''',
-                (new_account, url, account_hash, new_user, new_password))
-            conn.commit()
+            if update_or_insert == "insert":
+                conn.execute('''INSERT INTO account
+                    (account, url, cfaccount, user, password)
+                    VALUES (?, ?, ?, ?, ?)''',
+                    (new_account, url, account_hash, new_user, new_password))
+                conn.commit()
+            elif update_or_insert == "update":
+                conn.execute('''UPDATE account SET password = ? 
+                             WHERE account = ? AND user = ?''', 
+                             (new_password, new_account, new_user))
+                conn.commit()
         self.logger.info(
             'SUCCESS create_account(%s, %s, _) = %s [%.02f]' %
             (repr(new_account), repr(new_user), repr(url), time() - begin))

=== modified file 'test/unit/auth/test_server.py'
--- test/unit/auth/test_server.py	2010-07-19 16:25:18 +0000
+++ test/unit/auth/test_server.py	2010-07-26 17:48:48 +0000
@@ -181,7 +181,14 @@
         url = self.controller.create_account('test', 'tester', 'testing')
         self.assert_(url)
         self.assertEquals('/'.join(url.split('/')[:-1]),
-            self.controller.default_cluster_url.rstrip('/'), repr(url))
+        self.controller.default_cluster_url.rstrip('/'), repr(url))
+
+    def test_create_account_already_exists(self):
+        auth_server.http_connect = fake_http_connect(201, 201, 201)
+        first_url = self.controller.create_account('test', 'tester', 'testing')
+        auth_server.http_connect = fake_http_connect(201, 201, 201)
+        second_url = self.controller.create_account('test', 'tester', 'testily')
+        self.assertEquals(first_url, second_url)
 
     def test_recreate_accounts_none(self):
         auth_server.http_connect = fake_http_connect(201, 201, 201)