Closed Bug 840870 Opened 8 years ago Closed 8 years ago

Remove restriction that a chat can only be opened when a user is logged in?

Categories

(Firefox Graveyard :: SocialAPI, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: markh, Assigned: mixedpuppy)

References

Details

Attachments

(1 file, 6 obsolete files)

I just helped out a provider who was stung by the fact that we silently refused to open a chat window when no user was logged in.  This restriction seems somewhat arbitrary and doesn't offer any protection against abuse of the social API.  I see no good reason for this restriction and there may even be use-cases to allow it (eg, imagine a provider allowing a chat session to their support personnel when the user can't log in)
IMO there is only one place that haveLoggedInUser makes sense, that is in showProfile.  We should just check for provider.profile rather than trying to have a loggedin api.
A fly in the ointment here is that we currently close all chats when the user logs out, which is something we need to maintain.  Doing this sanely means our API would need to insist that we only ever get exactly 1 notification from the provider when the user is logged out (otherwise, we would end up in the situation where chats were open in the "not logged in" case, and if the provider later tells us "no one is still logged in" we would end up closing all those chats.)  Or we'd end up with far more complex state management for something where there isn't an identified realistic use-case.

So I think I'll close this as INVALID over the next few days unless someone objects - Shane, what do you think?
Flags: needinfo?(mixedpuppy)
(In reply to Mark Hammond (:markh) from comment #2)
> (otherwise, we
> would end up in the situation where chats were open in the "not logged in"
> case, and if the provider later tells us "no one is still logged in" we
> would end up closing all those chats.)  Or we'd end up with far more complex
> state management ...

Actually, a very similar problem could exist today - see bug 850969.
I think we can do two things here.

1. if a provider doesn't use login, we allow chat windows to open anyway and we close them on provider change.

2. if a provider does use login, we should close chat windows if we receive a new user-profile where the username does not match the previous profile.
Flags: needinfo?(mixedpuppy)
Depends on: 850969
As long as we maintain user-profile api I'm now thinking that if we get an empty profile sent, we close all chat windows (for that provider) as if the user logged out, regardless of whether there was a previous login.
Blocks: 889427
(In reply to Shane Caraveo (:mixedpuppy) from comment #5)
> As long as we maintain user-profile api I'm now thinking that if we get an
> empty profile sent, we close all chat windows (for that provider) as if the
> user logged out, regardless of whether there was a previous login.

I'm not sure how this addresses comment 2 - ie, if there is a chat window opened when noone is logged in, and the provider again happens to mention that noone is still logged in, we don't want to close the open chats.

I'd have thought something like comment 4 makes sense - only close chats when the login state changes, using the userid as a 'key'
(In reply to Mark Hammond (:markh) from comment #6)

> I'd have thought something like comment 4 makes sense - only close chats
> when the login state changes, using the userid as a 'key'

yes, that would be better.
Attached patch nologin restriction (obsolete) — Splinter Review
moved login restriction removal from patch in bug 891221 to here.
Attachment #777872 - Flags: feedback?(mhammond)
Duplicate of this bug: 850969
Blocks: 891221
Comment on attachment 777872 [details] [diff] [review]
nologin restriction

Review of attachment 777872 [details] [diff] [review]:
-----------------------------------------------------------------

The earlier comments in this bug don't seem to have been implemented at all?

::: browser/base/content/test/social/browser_social_chatwindow.js
@@ +439,5 @@
>            openChat(function() {
>              ok(!window.SocialChatBar.hasChats, "first window has no chats");
>              ok(secondWindow.SocialChatBar.hasChats, "second window has a chat");
>              secondWindow.close();
> +            port.close();

do the tests really fail if this isn't done here?

::: browser/base/content/test/social/browser_social_toolbar.js
@@ +123,5 @@
>      } catch(e) {}
>      let numIcons = Object.keys(Social.provider.ambientNotificationIcons).length;
>      ok(numIcons == 3, "prevent adding more than 3 ambient notification icons");
>  
> +    let mButton = document.getElementById("social-mark-button");

does the change to browser-social really cause this pre-existing test bug to be hit?  It seems quite unrelated.
Attachment #777872 - Flags: feedback?(mhammond) → feedback-
(In reply to Mark Hammond (:markh) from comment #10)
> Comment on attachment 777872 [details] [diff] [review]
> nologin restriction
> 
> Review of attachment 777872 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The earlier comments in this bug don't seem to have been implemented at all?

The only item not checked is if the username changed or not, otherwise comment #4 is implemented.  There isn't a clean way to do that right now, so I punted in order to think about it more.

> ::: browser/base/content/test/social/browser_social_chatwindow.js
> @@ +439,5 @@
> >            openChat(function() {
> >              ok(!window.SocialChatBar.hasChats, "first window has no chats");
> >              ok(secondWindow.SocialChatBar.hasChats, "second window has a chat");
> >              secondWindow.close();
> > +            port.close();
> 
> do the tests really fail if this isn't done here?

yes, the onmessage was staying around long enough to cause spurious messages/errors in the next test.

> ::: browser/base/content/test/social/browser_social_toolbar.js
> @@ +123,5 @@
> >      } catch(e) {}
> >      let numIcons = Object.keys(Social.provider.ambientNotificationIcons).length;
> >      ok(numIcons == 3, "prevent adding more than 3 ambient notification icons");
> >  
> > +    let mButton = document.getElementById("social-mark-button");
> 
> does the change to browser-social really cause this pre-existing test bug to
> be hit?  It seems quite unrelated.

yes, and I looked further into this, doesn't make sense at all (outside of timing, ambient buttons added prior to the wait), which tells me it could end up as an intermittent at some point.  I'm happy to move this to a separate bug and get it landed.
Attached patch nologin (obsolete) — Splinter Review
toolbar issue will move to another bug
now only closes chat windows if the userName changes rather than on any profile change (e.g. the user could change their image)
Assignee: nobody → mixedpuppy
Attachment #777872 - Attachment is obsolete: true
Attachment #781848 - Flags: feedback?(mhammond)
Comment on attachment 781848 [details] [diff] [review]
nologin

Review of attachment 781848 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-social.js
@@ +398,5 @@
>      if (dwu.isHandlingUserInput)
>        this.chatbar.focus();
>      return true;
>    },
> +  closeForOrigin: function(origin) {

it still doesn't make sense to me that this code, living in every window, is also doing the enumeration of top-level windows.

The observer that drives this is sent in SocialService, which inturn already has a reference to MozSocialAPI, which inturn already has "open chat" functions exported.  So why not just drop the observer completely, and have SocialService call a new function exported from MozSocialAPI for "close chat" capabilities?
Attachment #781848 - Flags: feedback?(mhammond) → feedback+
Attached patch nologin (obsolete) — Splinter Review
based on irc discussion
Attachment #781848 - Attachment is obsolete: true
Attachment #782317 - Flags: feedback?(mhammond)
Comment on attachment 782317 [details] [diff] [review]
nologin

Review of attachment 782317 [details] [diff] [review]:
-----------------------------------------------------------------

thanks!

::: browser/base/content/test/social/browser_chat_tearoff.js
@@ +80,2 @@
>            domwindow.removeEventListener("load", _load, false);
> +  

trailing space

::: toolkit/components/social/SocialService.jsm
@@ +838,5 @@
>    // Called by the workerAPI to update our profile information.
>    updateUserProfile: function(profile) {
>      if (!profile)
>        profile = {};
> +    let accountChanged = !this.profile || this.profile.userName != profile.userName;

We might as well handle the logged-out case better too - I think simply:

.. (!this.profile != !profile) || (this.profile.userName != profile.userName);

will work?
Attachment #782317 - Flags: feedback?(mhammond) → review+
(In reply to Mark Hammond (:markh) from comment #15)

> ::: toolkit/components/social/SocialService.jsm
> @@ +838,5 @@
> >    // Called by the workerAPI to update our profile information.
> >    updateUserProfile: function(profile) {
> >      if (!profile)
> >        profile = {};
> > +    let accountChanged = !this.profile || this.profile.userName != profile.userName;
> 
> We might as well handle the logged-out case better too - I think simply:
> 
> .. (!this.profile != !profile) || (this.profile.userName !=
> profile.userName);
> 
> will work?

!profile will always be false in this case. so that really becomes "if (this.profile || this.profile.userName != profile.userName);" which wont work if this.profile is undefined.  I'm not clear what you're trying to catch that I'm already not catching.
Yeah, I didn't look closely enough at the context.  What I was trying to catch was calling with a null profile when this.profile is already null (but yeah, this.profile and profile are never null given the line above...)
(but I guess Object.keys(profile).length could work?)
Attached patch nologin (obsolete) — Splinter Review
update for bitrot
Attachment #782317 - Attachment is obsolete: true
Attachment #789191 - Flags: review+
Depends on: 891216
No longer depends on: 891216
No longer blocks: 891221
Attached patch nologin (obsolete) — Splinter Review
patch to land independently of frameworker changes per irc discussion.  just asking for a quick re-review.

https://tbpl.mozilla.org/?tree=Try&rev=13f8f04eb4de
Attachment #789191 - Attachment is obsolete: true
Attachment #790372 - Flags: review?(mhammond)
Attached patch nologin (obsolete) — Splinter Review
Discovered a bug with closeAllChatWindows while doing some manual testing.  This patch fixes the problem (caused by using element.children incorrectly) as modifies the test to watch for that in the future.

https://tbpl.mozilla.org/?tree=Try&rev=4af652404700
Attachment #790372 - Attachment is obsolete: true
Attachment #790372 - Flags: review?(mhammond)
Attachment #790459 - Flags: review?(mhammond)
Comment on attachment 790459 [details] [diff] [review]
nologin

Review of attachment 790459 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

::: browser/base/content/test/social/browser_chat_tearoff.js
@@ +80,2 @@
>            domwindow.removeEventListener("load", _load, false);
> +  

whitespace nit

@@ +173,5 @@
> +            next();
> +          }, false);
> +
> +          is(doc.documentElement.getAttribute("windowtype"), "Social:Chat", "Social:Chat window opened");
> +          is(doc.location.href, "chrome://browser/content/chatWindow.xul", "Should have seen the right window open");

this check is redundant given the check at the top of the listener
Attachment #790459 - Flags: review?(mhammond) → review+
Attached patch nologinSplinter Review
updated with comments

https://hg.mozilla.org/integration/fx-team/rev/c211aa7adf48
Attachment #790459 - Attachment is obsolete: true
Attachment #790997 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/c211aa7adf48
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.