Ubuntu Software Center

Merge lp:~gary-lasker/software-center/recommendations-sso-login-lp973612 into lp:software-center

  1. recommendations-sso-login-lp973612
  2. Merge into trunk
Proposed by Gary Lasker
Status: Merged
Merged at revision: 2989
Proposed branch: lp:~gary-lasker/software-center/recommendations-sso-login-lp973612
Merge into: lp:software-center
Diff against target: 131 lines (+66/-4)
2 files modified
softwarecenter/backend/login_sso.py (+3/-2)
softwarecenter/ui/gtk3/widgets/recommendations.py (+63/-2)
To merge this branch: bzr merge lp:~gary-lasker/software-center/recommendations-sso-login-lp973612
Reviewer Review Type Date Requested Status
software-store-developers Pending
Review via email: mp+102044@code.launchpad.net

Description of the change

This branch fixes the recommendations opt-in issue described in bug 973612, and also implements most of the changes needed for bug 967064. It implements an SSO login flow specific to the recommendations service, and reuses the opt-in text from the opt-in panel in the SSO dialog itself so that we can avoid adding an entire new string this far past string freeze for Precise.

The branch also takes care of the case where a user had previously opted-in to recommendations and their Ubuntu SSO token has since been removed, revoked, or otherwise found to be invalid.

Finally, a small UI improvement is included where a spinner is now show immediately upon opting-in, where previously there was a small delay where the opt-in panel continued to show before the profile upload spinner appeared.

To post a comment you must log in.
Revision history for this message
Gary Lasker (gary-lasker) wrote :

P.S. All unit tests pass. And thank you for your review!

Revision history for this message
Michael Vogt (mvo) wrote :

Thanks for your work on this bug, the code looks good.

During testing I noticed a small issues:

If you are opted into recommendations and you start software-center with no network connection it will opt-out
the user again as the missing network connection causes a failure to login. To test:
1. open s-c
2. opt into recommendations and wait until they show up
3. close s-c
4. disconnect network (via NetworkManager)
5. start s-c and notice that it says "Turn on recommendations" now
(as the user loose the UUID when we opt him/her out on error this one should probably be fixed).

A corner case that is harder to hit in the real world is if the user manually deleted his/her token via
seahorse or by removing the keyring file entirely. In this case in my testing s-c still pops up a dialog
asking for login on startup instead of going into the "opt-out" state. This is not such a big deal I think
as AFAICT this condition can only be triggered when we have no token but a UUID (very unlikely in the real
world). The more relevant case of the user invalidating his/her token on login.ubuntu.com works like a charm :)

Revision history for this message
Michael Vogt (mvo) wrote :

Another small tweak is to change:

=== modified file 'softwarecenter/backend/login_sso.py'
--- softwarecenter/backend/login_sso.py 2012-04-15 23:06:14 +0000
+++ softwarecenter/backend/login_sso.py 2012-04-16 07:29:15 +0000
@@ -97,7 +97,7 @@
         if app_name != self.appname:
             return
         self.cancel_login()
- self.emit("login-failed")
+ self.emit("login-canceled")

The signal name from the SSO is a little bit confusing, as AuthorizationDenied means that the
user did not authorize the app, i.e. the user clicked "cancel" in the auth dialog (or closed it).
Changing that seems to be fine and your fixes keep working. And thanks for demoting the LOG.error to
a LOG.warn, we can probably even make this info only, its kinf of nice to have it in the logs, but
like you said, its really not that much of a error :)

Revision history for this message
Gary Lasker (gary-lasker) wrote :

Thank you, Michael, for your detailed review and suggestions! I appreciate your taking the time with this. I'll make the fix and the changes you specify and resubmit.

Revision history for this message
Michael Vogt (mvo) wrote :

Thanks Gary, if its easier I'm happy to merge this in iterations, i.e. I think the "user-manually-deleted-his/her-token-via-seahorse" is less important than the other one so I'm totally fine with a branch that leaves this case out for now and we can merge it later/in a seperate branch.

lp:~gary-lasker/software-center/recommendations-sso-login-lp973612 updated
2983. By Gary Lasker

login-canceled is more appropriate for the case where the user has canceled the auth dialog

2984. By Gary Lasker

log when the user has cancelled the authorization dialog

Revision history for this message
Gary Lasker (gary-lasker) wrote :

Btw, the "corner case" that you mentioned, that's actually implemented that way on purpose. Meaning, if the user for some reason invalidates their token with Seahorse, I assumed that they were not intending to also opt-out of recommendations so I left it such that they can just re-login at startup. But maybe this is an incorrect assumption?

I made the other tweaks you mentioned, just gotta fix the no-network bug now.

lp:~gary-lasker/software-center/recommendations-sso-login-lp973612 updated
2985. By Gary Lasker

if there is an error in the whois query, we can assume a network connectivity issue, so we simply hide the recommendations panel as we do for the case where we can't reach the recommendations server

2986. By Gary Lasker

revert the whoami_error fix as we also get these in the case where the user's token has been revoked or invalidated on the server side (so we do need to fully opt-out in this case), also, handle an sso login-failed event

2987. By Gary Lasker

really fix the case on startup if there is no network connection

Revision history for this message
Gary Lasker (gary-lasker) wrote :

This should be ready now. The startup network connectivity problem now works correctly, as does the case where the user's SSO token has been revoked or invalidated on the server.

Please check the changes and re-review. Thanks!

lp:~gary-lasker/software-center/recommendations-sso-login-lp973612 updated
2988. By Gary Lasker

merge with trunk

Revision history for this message
Michael Vogt (mvo) wrote :

Thanks for the update, this is merged to trunk now. Unfortunately I just discovered the following when doing a test:

1. opt-in to recommendations
2. delete the token on the server
3. close/reopen s-c
4. the following is printed on the terminal:

...
ERROR:softwarecenter.backend.login_sso:_on_credentials_error for Ubuntu Software Center: dbus.Dictionary({dbus.String(u'errtype'): dbus.String(u'DBusException'), dbus.String(u'message'): dbus.String(u"The '/org/freedesktop/secrets/collection/login/542' object does not exist")}, signature=dbus.Signature('ss')) ()
ERROR:softwarecenter.backend.login_sso:_on_credentials_error for Ubuntu Software Center: dbus.Dictionary({dbus.String(u'errtype'): dbus.String(u'DBusException'), dbus.String(u'message'): dbus.String(u"The '/org/freedesktop/secrets/collection/login/542' object does not exist")}, signature=dbus.Signature('ss')) ()
ERROR: can not obtain a oauth token
...

So it appears that ubuntu-sso tries to bring up another dialog but fails (that looks like a issue with the ubuntu-sso-client). The reason I flag this is that it appears that the problem with the login dialog at startup might come back if this ubuntu-sso-client issue is fixed.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'softwarecenter/backend/login_sso.py'
2--- softwarecenter/backend/login_sso.py 2012-03-19 14:20:55 +0000
3+++ softwarecenter/backend/login_sso.py 2012-04-19 16:13:22 +0000
4@@ -85,7 +85,7 @@
5 self._credentials = credentials
6
7 def _on_credentials_error(self, app_name, error, detailed_error=""):
8- LOG.error("_on_credentails_error for %s: %s (%s)" % (
9+ LOG.error("_on_credentials_error for %s: %s (%s)" % (
10 app_name, error, detailed_error))
11 if app_name != self.appname:
12 return
13@@ -93,10 +93,11 @@
14 self.emit("login-failed")
15
16 def _on_authorization_denied(self, app_name):
17- LOG.error("_on_authorization_denied: %s" % app_name)
18+ LOG.info("_on_authorization_denied: %s" % app_name)
19 if app_name != self.appname:
20 return
21 self.cancel_login()
22+ self.emit("login-canceled")
23
24
25 class LoginBackendDbusSSOFake(LoginBackend):
26
27=== modified file 'softwarecenter/ui/gtk3/widgets/recommendations.py'
28--- softwarecenter/ui/gtk3/widgets/recommendations.py 2012-04-04 05:48:40 +0000
29+++ softwarecenter/ui/gtk3/widgets/recommendations.py 2012-04-19 16:13:22 +0000
30@@ -25,10 +25,15 @@
31 from softwarecenter.ui.gtk3.em import StockEms
32 from softwarecenter.ui.gtk3.widgets.containers import (FramedHeaderBox,
33 FlowableGrid)
34+from softwarecenter.ui.gtk3.utils import get_parent_xid
35 from softwarecenter.db.categories import (RecommendedForYouCategory,
36 AppRecommendationsCategory)
37 from softwarecenter.backend.recagent import RecommenderAgent
38+from softwarecenter.backend.login_sso import get_sso_backend
39+from softwarecenter.backend.ubuntusso import get_ubuntu_sso_backend
40+from softwarecenter.enums import SOFTWARE_CENTER_NAME_KEYRING
41 from softwarecenter.utils import utf8
42+from softwarecenter.netstatus import network_state_is_connected
43
44 LOG = logging.getLogger(__name__)
45
46@@ -150,7 +155,7 @@
47 self.set_header_label(_(u"Recommended For You"))
48 self.recommended_for_you_content = None
49 if self.recommender_agent.is_opted_in():
50- self._update_recommended_for_you_content()
51+ self._try_sso_login()
52 else:
53 self._show_opt_in_view()
54
55@@ -186,12 +191,13 @@
56 self.opt_in_to_recommendations_service()
57
58 def opt_in_to_recommendations_service(self):
59+ # first we verify the ubuntu sso login/oath status, and if that is good
60 # we upload the user profile here, and only after this is finished
61 # do we fire the request for recommendations and finally display
62 # them here -- a spinner is shown for this process (the spec
63 # wants a progress bar, but we don't have access to real-time
64 # progress info)
65- self._upload_user_profile_and_get_recommendations()
66+ self._try_sso_login()
67
68 def opt_out_of_recommendations_service(self):
69 # tell the backend that the user has opted out
70@@ -205,6 +211,61 @@
71 self.emit("recommendations-opt-out")
72 self._disconnect_recommender_listeners()
73
74+ def _try_sso_login(self):
75+ # display the SSO login dialog if needed
76+ # FIXME: consider improving the text in the SSO dialog, for now
77+ # we simply reuse the opt-in text from the panel since we
78+ # are well past string freeze
79+ self.spinner_notebook.show_spinner()
80+ self.sso = get_sso_backend(get_parent_xid(self),
81+ SOFTWARE_CENTER_NAME_KEYRING,
82+ self.RECOMMENDATIONS_OPT_IN_TEXT)
83+ self.sso.connect("login-successful", self._maybe_login_successful)
84+ self.sso.connect("login-failed", self._login_failed)
85+ self.sso.connect("login-canceled", self._login_canceled)
86+ self.sso.login_or_register()
87+
88+ def _maybe_login_successful(self, sso, oauth_result):
89+ self.ssoapi = get_ubuntu_sso_backend()
90+ self.ssoapi.connect("whoami", self._whoami_done)
91+ self.ssoapi.connect("error", self._whoami_error)
92+ # this will automatically verify the keyring token and retrigger
93+ # login (once) if its expired
94+ self.ssoapi.whoami()
95+
96+ def _whoami_done(self, ssologin, result):
97+ # we are all squared up with SSO login, now we can proceed with the
98+ # recommendations display, or the profile upload if this is an
99+ # initial opt-in
100+ if self.recommender_agent.is_opted_in():
101+ self._update_recommended_for_you_content()
102+ else:
103+ self._upload_user_profile_and_get_recommendations()
104+
105+ def _whoami_error(self, ssologin, e):
106+ self.spinner_notebook.hide_spinner()
107+ if not network_state_is_connected():
108+ # if there is an error in the SSO whois, first just check if we
109+ # have network access and if we do no, just hide the panel
110+ self._hide_recommended_for_you_panel()
111+ else:
112+ # an error that is not network related indicates that the user's
113+ # token has likely been revoked or invalidated on the server, for
114+ # this case we want to reset the user's opt-in status
115+ self.opt_out_of_recommendations_service()
116+
117+ def _login_failed(self, sso):
118+ # if the user cancels out of the SSO dialog, reset everything to the
119+ # opt-in view state
120+ self.spinner_notebook.hide_spinner()
121+ self.opt_out_of_recommendations_service()
122+
123+ def _login_canceled(self, sso):
124+ # if the user cancels out of the SSO dialog, reset everything to the
125+ # opt-in view state
126+ self.spinner_notebook.hide_spinner()
127+ self.opt_out_of_recommendations_service()
128+
129 def _upload_user_profile_and_get_recommendations(self):
130 # initiate upload of the user profile here
131 self._upload_user_profile()

Subscribers

People subscribed via source and target branches