Merge lp:~canonical-isd-hackers/canonical-identity-provider/721318-enhance-merge into lp:~canonical-isd-hackers/canonical-identity-provider/stable

[David Owen]

Display a more informative error message when the source account has been used to log in to RPs, and display those RPs.

Added a more informative error message when the source account has been used to log in to RPs, also explaining that this is a problem because any accounts at those RPs will become inaccessible by the user. Also, display a list of the RPs so the action can be verified with the user.

During testing, the test consumer would block when making an API call---the WSGI server was serving only a single request at a time. This is strange because I thought the wsgiserver script was added in order to run multi-threaded to test the test consumer. Nevertheless, all the docs I could find seemed to indicate it was single-threaded, single-process, not select-based, &c., unless explicitly configured to be so.

[David Owen (dsowen)]

Set to merge into stable as this is needed in production to satisfy a pending RT.

[Michael Nelson (michael.nelson)]

Hi David - the changes for the better error look great.

Regarding the wsgi server change, I don't quite understand... you didn't add any tests in this branch, which implies that the test must have been hanging in trunk?

If wsgiserver.py is only used for testing/dev env., then fine, otherwise, please check the annotation and chat with whoever added it to see if you comment above is correct.
Thanks!

review: Approve

=== modified file 'identityprovider/bin/accounts_merge.py'
--- identityprovider/bin/accounts_merge.py	2011-02-16 15:09:40 +0000
+++ identityprovider/bin/accounts_merge.py	2011-02-23 17:01:36 +0000
@@ -26,9 +26,20 @@
 
         used_to_log_in = self.account_was_used_to_log_in(source)
         if used_to_log_in and not self.skip_safety_warnings:
-            self.error("E03: Source account was used to log in to other "
-                  "services. Use --skip-safety-warnings to merge "
-                  "those accounts.")
+            sites = source.last_authenticated_sites(limit=None)
+            urls = [s.trust_root for s in sites]
+            self.error(
+"""E03: Source account was used to log in to other services.  Merging
+accounts will make it so the user could no longer access those other
+services using the old SSO account.  The user may lose access to
+history or important information.  Please confirm this with the user
+before continuing.
+
+The other services are:
+
+%s
+
+Use --skip-safety-warnings to merge the accounts anyway.""" % '\n'.join(urls))
 
         if source.pk == destination.pk:
             self.error("E08: Trying to merge account with itself")
@@ -282,12 +293,15 @@
         from unittest import main
         main(argv=["AccountMergerTestCase"])
     else:
-        if options.dry_run:
-            print "Dry run, no changes will be saved to DB."
-        account_merger = AccountMerger(options.dry_run,
-                                       options.skip_safety_warnings)
-        account_merger.merge(options.destination, options.source)
-        print "%s merged into %s" % (options.source, options.destination)
+        if options.source is None or options.destination is None:
+            parser.print_help()
+        else:
+            if options.dry_run:
+                print "Dry run, no changes will be saved to DB."
+            account_merger = AccountMerger(options.dry_run,
+                                           options.skip_safety_warnings)
+            account_merger.merge(options.destination, options.source)
+            print "%s merged into %s" % (options.source, options.destination)
 
 
 

=== modified file 'identityprovider/models/account.py'
--- identityprovider/models/account.py	2011-01-11 15:50:14 +0000
+++ identityprovider/models/account.py	2011-02-23 17:01:36 +0000
@@ -114,8 +114,8 @@
         return email.order_by('email')
 
     def last_authenticated_sites(self, limit=10):
-        sites = self.openidrpsummary_set.order_by('date_last_used')
-        return sites.reverse()[:limit]
+        sites = self.openidrpsummary_set.order_by('-date_last_used')
+        return sites if limit is None else sites[:limit]
 
     def sites_with_active_sessions(self, age_hours=24):
         """

=== modified file 'scripts/merge-accounts' (properties changed: -x to +x)
=== modified file 'wsgiserver.py'
--- wsgiserver.py	2010-04-21 15:29:24 +0000
+++ wsgiserver.py	2011-02-23 17:01:36 +0000
@@ -1,13 +1,18 @@
 # Copyright 2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-from wsgiref.simple_server import make_server
+from SocketServer import ThreadingMixIn
+from wsgiref.simple_server import WSGIServer, make_server
 
 import os
 os.environ['DJANGO_SETTINGS_MODULE'] = 'settings'
 
 from identityprovider.wsgi import make_app
-httpd = make_server('', 8000, make_app())
+
+class ThreadedWSGIServer(ThreadingMixIn, WSGIServer):
+    pass
+
+httpd = make_server('', 8000, make_app(), server_class=ThreadedWSGIServer)
 
 
 print "Serving HTTP on port 8000..."