799502 - Override the UA string for moodle to get the rich text editor rather than plain textareas

Status: REOPENED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

+++ This bug was initially created as a clone of Bug #797703 +++

see bug 797703 for details

Comment 1

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 669535 [details] [diff] [review]
patch

r=me

I hate the web.  :(

Comment 2

[Gervase Markham [:gerv]]

Comment on attachment 669535 [details] [diff] [review]
patch

r=gerv, echoing bz's opinion. If it were possible to check for cookies beginning with "MoodleSession" rather than containing it, that would be even better. I believe JS now has a startsWith?

Can you quickly outline what you did to test this?

Gerv

Comment 3

[Dão Gottwald [:dao]]

(In reply to Gervase Markham [:gerv] from comment #2)
> Comment on attachment 669535 [details] [diff] [review]
> patch
> 
> r=gerv, echoing bz's opinion. If it were possible to check for cookies
> beginning with "MoodleSession" rather than containing it, that would be even
> better. I believe JS now has a startsWith?

The Cookie header contains multiple cookies here, so it's more complicated than that.

> Can you quickly outline what you did to test this?

I loaded http://demo.moodle.net/user/edit.php?id=5&course=1 and logged in as admin.

Comment 4

[Gervase Markham [:gerv]]

OK, fair enough on the cookie name.

Gerv

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 669535 [details] [diff] [review]
patch

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

Looks good to me.  I'd prefer having "UserAgentOverrides.init();" + the added block in a separate function to keep OnProfileStartup clean, but it's up to you.
Better yet I think would be to have this specific knowledge about Moodle (and other future cases?) in UserAgentOverrides.jsm instead of nsBrowserGlue, but I assume you did it this way to keep this Desktop-only.

Comment 6

[Dão Gottwald [:dao]]

Attachment: patch v2

(In reply to :Felipe Gomes from comment #5)
> Looks good to me.  I'd prefer having "UserAgentOverrides.init();" + the
> added block in a separate function to keep OnProfileStartup clean, but it's
> up to you.

done

> Better yet I think would be to have this specific knowledge about Moodle
> (and other future cases?) in UserAgentOverrides.jsm instead of
> nsBrowserGlue, but I assume you did it this way to keep this Desktop-only.

exactly

Comment 7

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bead66674f12

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

moving tracking to this bug, removing it from 797703

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bead66674f12

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 669933 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 588909
User impact if declined: plain textarea instead of rich text editor on moodle (worse user experience)
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): reasonably low risk, and the User Agent override for moodle can be disabled via a pref
String or UUID changes made by this patch: none

Comment 11

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Is this feature really need?
The previous similar patch supporting aol.com is even so, I don't think these specified patch is good way.
These approach is domain specific. If this approach is recognized,  other sites in the world have the right to get a same support.

If we need to land this support necessarily, we should take more versatile method (e.g. site specfic option).

Comment 12

[Dão Gottwald [:dao]]

(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #11)
> Is this feature really need?

Yes, see the discussion in bug 797703. We expect that many Moodle instances won't be updated soon and we don't want a worse experience on Moodle for Firefox users.

Comment 13

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Dão Gottwald [:dao] from comment #12)
> Yes, see the discussion in bug 797703. We expect that many Moodle instances
> won't be updated soon and we don't want a worse experience on Moodle for
> Firefox users.

OK. I understand the how and why.

However, I don't like this approach...... Even if it's a temporary treatment, this approach is special treatment for a specified site. It may be not browser's business as a Web-platform.

Comment 14

[Tim Hunt]

Moodle is a web application that lots of universities (and other people) install on their own servers. So, adding a work-around for Moodle is like adding a work-around for Drupal or Wordpress. It is not like adding a work-around for Twitter or Facebook.

Comment 15

[Boris Zbarsky [:bzbarsky]]

No one _likes_ this approach.

Do feel free to suggest a better one if you have one....

Comment 16

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Tim, Boris, Thank you for your accounts.

At comment 13, I did not have a mind to go against this approach strongly.
I apologize that my comment dragged up the discussion.

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 669933 [details] [diff] [review]
patch v2

Thanks guys - I know this isn't the perfect solution, but it will definitely make our users have a much better experience.

Comment 18

[Dão Gottwald [:dao]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b8c17cd78859
https://hg.mozilla.org/releases/mozilla-beta/rev/bc0e8935942a

Comment 19

[Dan Poltawski]

A big thanks from Moodle for implementing this workaround. We've integrated a fix for the underlying problem in Moodle (http://tracker.moodle.org/browse/MDL-35469), but it'll take sites some time to update with this fix.

Comment 20

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/17.0 Firefox/17.0

Setting to verified in F17 beta 5. Checked on Windows 7, Mac OS 10.7, Ubuntu 12.04. 
Rich text editor displayed as expected and functional on http://demo.moodle.net/user/edit.php?id=5&course=1

The site's layout is broken in Firefox however. Logged bug 810828.

Comment 21

[albert.gasset]

This change breaks another feature, linking Moodle with Mahara using MNet: http://docs.moodle.org/23/en/MNet 

MNet allows users of a Moodle site to roam to a Mahara site. The atuhentication process checks if the User Agent string received by both sites are equal to allow access.

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to albert.gasset from comment #21)
> This change breaks another feature, linking Moodle with Mahara using MNet:
> http://docs.moodle.org/23/en/MNet 
> 
> MNet allows users of a Moodle site to roam to a Mahara site. The
> atuhentication process checks if the User Agent string received by both
> sites are equal to allow access.

That sounds like tech evangelism to Mahara then - as it is a website and not an application, please file a separate bug for Tech Evangelism.

Comment 23

[albert.gasset]

Mahara is an application, it's an open source portfolio system that is commonly used with Moodle. This is causing a serious problem for its users: https://bugs.launchpad.net/mahara/+bug/1082416?comments=all

I understand this workaround was implement to avoid trouble for some users, but for other users this is even worse. Why not fix the problem at the right place (Moodle), to avoid the risk of more side effects?

(In reply to Lukas Blakk [:lsblakk] from comment #22)
> (In reply to albert.gasset from comment #21)
> > This change breaks another feature, linking Moodle with Mahara using MNet:
> > http://docs.moodle.org/23/en/MNet 
> > 
> > MNet allows users of a Moodle site to roam to a Mahara site. The
> > atuhentication process checks if the User Agent string received by both
> > sites are equal to allow access.
> 
> That sounds like tech evangelism to Mahara then - as it is a website and not
> an application, please file a separate bug for Tech Evangelism.

Comment 24

[Dan Poltawski]

Hi Albert,

In Moodle, we have now fixed this ( http://tracker.moodle.org/browse/MDL-35469 ), however i'm afraid its not likely at all that all Moodle sites will be upgraded with this fix.

Comment 25

[Gervase Markham [:gerv]]

(In reply to albert.gasset from comment #21)
> MNet allows users of a Moodle site to roam to a Mahara site. The
> atuhentication process checks if the User Agent string received by both
> sites are equal to allow access.

The obvious question is: what on earth for? Given that you have a proper SSO system, this provides precisely zero added security.

If it's the considered opinion of the Moodle leadership that we should remove the workaround, we will. You have more info than us to make the decision on which problem is worse.

Gerv

Comment 26

[Boris Zbarsky [:bzbarsky]]

Can we simply send the same cooked-up UA to Mahara?  Can we detect when we're dealing with it?

Comment 27

[Matthew N. [:MattN]]

I filed bug 815446 for the MNet SSO fallout.

Comment 28

[Ed Morley [:emorley]]

Partly backed out by bug 815743; rest needs cleanup -> reopening.

Comment 29

[Dão Gottwald [:dao]]

(In reply to Ed Morley [UTC+0; email:edmorley@moco] from comment #28)
> Partly backed out by bug 815743; rest needs cleanup -> reopening.

Could you be more specific? What's the rest and what kind of cleanup is needed?