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
=== modified file 'softwarecenter/backend/login_sso.py'
--- softwarecenter/backend/login_sso.py 2012-03-19 14:20:55 +0000
+++ softwarecenter/backend/login_sso.py 2012-04-19 16:13:22 +0000
@@ -85,7 +85,7 @@
85 self._credentials = credentials85 self._credentials = credentials
8686
87 def _on_credentials_error(self, app_name, error, detailed_error=""):87 def _on_credentials_error(self, app_name, error, detailed_error=""):
88 LOG.error("_on_credentails_error for %s: %s (%s)" % (88 LOG.error("_on_credentials_error for %s: %s (%s)" % (
89 app_name, error, detailed_error))89 app_name, error, detailed_error))
90 if app_name != self.appname:90 if app_name != self.appname:
91 return91 return
@@ -93,10 +93,11 @@
93 self.emit("login-failed")93 self.emit("login-failed")
9494
95 def _on_authorization_denied(self, app_name):95 def _on_authorization_denied(self, app_name):
96 LOG.error("_on_authorization_denied: %s" % app_name)96 LOG.info("_on_authorization_denied: %s" % app_name)
97 if app_name != self.appname:97 if app_name != self.appname:
98 return98 return
99 self.cancel_login()99 self.cancel_login()
100 self.emit("login-canceled")
100101
101102
102class LoginBackendDbusSSOFake(LoginBackend):103class LoginBackendDbusSSOFake(LoginBackend):
103104
=== modified file 'softwarecenter/ui/gtk3/widgets/recommendations.py'
--- softwarecenter/ui/gtk3/widgets/recommendations.py 2012-04-04 05:48:40 +0000
+++ softwarecenter/ui/gtk3/widgets/recommendations.py 2012-04-19 16:13:22 +0000
@@ -25,10 +25,15 @@
25from softwarecenter.ui.gtk3.em import StockEms25from softwarecenter.ui.gtk3.em import StockEms
26from softwarecenter.ui.gtk3.widgets.containers import (FramedHeaderBox,26from softwarecenter.ui.gtk3.widgets.containers import (FramedHeaderBox,
27 FlowableGrid)27 FlowableGrid)
28from softwarecenter.ui.gtk3.utils import get_parent_xid
28from softwarecenter.db.categories import (RecommendedForYouCategory,29from softwarecenter.db.categories import (RecommendedForYouCategory,
29 AppRecommendationsCategory)30 AppRecommendationsCategory)
30from softwarecenter.backend.recagent import RecommenderAgent31from softwarecenter.backend.recagent import RecommenderAgent
32from softwarecenter.backend.login_sso import get_sso_backend
33from softwarecenter.backend.ubuntusso import get_ubuntu_sso_backend
34from softwarecenter.enums import SOFTWARE_CENTER_NAME_KEYRING
31from softwarecenter.utils import utf835from softwarecenter.utils import utf8
36from softwarecenter.netstatus import network_state_is_connected
3237
33LOG = logging.getLogger(__name__)38LOG = logging.getLogger(__name__)
3439
@@ -150,7 +155,7 @@
150 self.set_header_label(_(u"Recommended For You"))155 self.set_header_label(_(u"Recommended For You"))
151 self.recommended_for_you_content = None156 self.recommended_for_you_content = None
152 if self.recommender_agent.is_opted_in():157 if self.recommender_agent.is_opted_in():
153 self._update_recommended_for_you_content()158 self._try_sso_login()
154 else:159 else:
155 self._show_opt_in_view()160 self._show_opt_in_view()
156161
@@ -186,12 +191,13 @@
186 self.opt_in_to_recommendations_service()191 self.opt_in_to_recommendations_service()
187192
188 def opt_in_to_recommendations_service(self):193 def opt_in_to_recommendations_service(self):
194 # first we verify the ubuntu sso login/oath status, and if that is good
189 # we upload the user profile here, and only after this is finished195 # we upload the user profile here, and only after this is finished
190 # do we fire the request for recommendations and finally display196 # do we fire the request for recommendations and finally display
191 # them here -- a spinner is shown for this process (the spec197 # them here -- a spinner is shown for this process (the spec
192 # wants a progress bar, but we don't have access to real-time198 # wants a progress bar, but we don't have access to real-time
193 # progress info)199 # progress info)
194 self._upload_user_profile_and_get_recommendations()200 self._try_sso_login()
195201
196 def opt_out_of_recommendations_service(self):202 def opt_out_of_recommendations_service(self):
197 # tell the backend that the user has opted out203 # tell the backend that the user has opted out
@@ -205,6 +211,61 @@
205 self.emit("recommendations-opt-out")211 self.emit("recommendations-opt-out")
206 self._disconnect_recommender_listeners()212 self._disconnect_recommender_listeners()
207213
214 def _try_sso_login(self):
215 # display the SSO login dialog if needed
216 # FIXME: consider improving the text in the SSO dialog, for now
217 # we simply reuse the opt-in text from the panel since we
218 # are well past string freeze
219 self.spinner_notebook.show_spinner()
220 self.sso = get_sso_backend(get_parent_xid(self),
221 SOFTWARE_CENTER_NAME_KEYRING,
222 self.RECOMMENDATIONS_OPT_IN_TEXT)
223 self.sso.connect("login-successful", self._maybe_login_successful)
224 self.sso.connect("login-failed", self._login_failed)
225 self.sso.connect("login-canceled", self._login_canceled)
226 self.sso.login_or_register()
227
228 def _maybe_login_successful(self, sso, oauth_result):
229 self.ssoapi = get_ubuntu_sso_backend()
230 self.ssoapi.connect("whoami", self._whoami_done)
231 self.ssoapi.connect("error", self._whoami_error)
232 # this will automatically verify the keyring token and retrigger
233 # login (once) if its expired
234 self.ssoapi.whoami()
235
236 def _whoami_done(self, ssologin, result):
237 # we are all squared up with SSO login, now we can proceed with the
238 # recommendations display, or the profile upload if this is an
239 # initial opt-in
240 if self.recommender_agent.is_opted_in():
241 self._update_recommended_for_you_content()
242 else:
243 self._upload_user_profile_and_get_recommendations()
244
245 def _whoami_error(self, ssologin, e):
246 self.spinner_notebook.hide_spinner()
247 if not network_state_is_connected():
248 # if there is an error in the SSO whois, first just check if we
249 # have network access and if we do no, just hide the panel
250 self._hide_recommended_for_you_panel()
251 else:
252 # an error that is not network related indicates that the user's
253 # token has likely been revoked or invalidated on the server, for
254 # this case we want to reset the user's opt-in status
255 self.opt_out_of_recommendations_service()
256
257 def _login_failed(self, sso):
258 # if the user cancels out of the SSO dialog, reset everything to the
259 # opt-in view state
260 self.spinner_notebook.hide_spinner()
261 self.opt_out_of_recommendations_service()
262
263 def _login_canceled(self, sso):
264 # if the user cancels out of the SSO dialog, reset everything to the
265 # opt-in view state
266 self.spinner_notebook.hide_spinner()
267 self.opt_out_of_recommendations_service()
268
208 def _upload_user_profile_and_get_recommendations(self):269 def _upload_user_profile_and_get_recommendations(self):
209 # initiate upload of the user profile here270 # initiate upload of the user profile here
210 self._upload_user_profile()271 self._upload_user_profile()

Subscribers

People subscribed via source and target branches