Opened 10 years ago

Closed 10 years ago

#11343 closed defect (fixed)

Disallow @ symbol in username because of TinyMCE problems

Reported by: kcrisman Owned by: jason, mpatel, was
Priority: blocker Milestone: sage-4.7.2
Component: notebook Keywords: notebook, tinymce, at, sd31
Cc: jason, boothby, was, mpatel, ddrake, ppurka Merged in: sage-4.7.2.rc0
Authors: Karl-Dieter Crisman Reviewers: Jeroen Demeyer
Report Upstream: Reported upstream. Little or no feedback. Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

Please see below for what TinyMCE cells look like if you have an @ in your username. This is since at least 4.6, see this thread.

I'm making this critical, because it removes major piece of functionality. At the very least, we'll need to change the login page to remove the @ availability!!!

Re-enabling this is #11470.


From #8995, a dup of this:

See the thread by Dennis Watson here:

http://groups.google.com/group/sage-support/browse_frm/thread/2acd499a566efce1

In particular, some tinymce javascript files were trying to download from a URL that included the username, like:

http://sagenb.org/home/usernamewith@/javascript/tiny_mce/langs/en.js


Apply trac_11343-fix_at_symbol.patch to the SAGENB repository.

Attachments (5)

sageproblem.JPG (105.5 KB) - added by kcrisman 10 years ago.
trac_11343-fix_at_symbol.patch (2.0 KB) - added by kcrisman 10 years ago.
test.sagenb-failure.png (37.1 KB) - added by kcrisman 10 years ago.
works_fine_without_patch_opera-11.51.png (81.2 KB) - added by ppurka 10 years ago.
Works fine in opera-11.51 even without the patch. (sage-4.7.0)
opera-11.51_sage-4.7.1_does_not_work.png (92.4 KB) - added by ppurka 10 years ago.
Neglect the previous *opera*.png. This is on a fresh .sagenb directory (sage-4.7.1)

Download all attachments as: .zip

Change History (36)

Changed 10 years ago by kcrisman

comment:1 Changed 10 years ago by kcrisman

  • Cc ddrake added
  • Report Upstream changed from N/A to Not yet reported upstream; Will do shortly.

This is also apparently known elsewhere - see more of the same thread. Changing the upstream designation; hopefully a little more digging will find a workaround.

comment:2 Changed 10 years ago by kcrisman

Okay, here is where one would have to at least change this - sagenb/data/sage/html/accounts/registration.html. The other place is in twist.py.

comment:3 Changed 10 years ago by kcrisman

  • Keywords sd31 added
  • Status changed from new to needs_review

Okay, I've got this temporarily disabled. Re-enabling this is #11470.

Please test whether this breaks existing usernames with @!!!

Changed 10 years ago by kcrisman

comment:4 Changed 10 years ago by kcrisman

  • Report Upstream changed from Not yet reported upstream; Will do shortly. to Reported upstream. Little or no feedback.

This is reported upstream at this link.

comment:5 Changed 10 years ago by kcrisman

Confirmed for flask as well, at least at demo2.sagenb.org or sagenb.org (I tried both)

http://demo2.sagenb.org/home/b%40b/0/

is the name but it still causes problems. You can try this with password 'abc123'. Note that the user name at the top (next to Toggle) is still b@b.

comment:6 Changed 10 years ago by kcrisman

What's the status of this? If the Flask notebook isn't going to be done soon, this should really have been in 4.7.1 :( especially as people upgrade their servers.

comment:7 follow-up: Changed 10 years ago by jason

At the very least, we should take "@" out of the explicitly-listed good characters for notebook names on the registration page!

comment:8 in reply to: ↑ 7 Changed 10 years ago by kcrisman

Replying to jason:

At the very least, we should take "@" out of the explicitly-listed good characters for notebook names on the registration page!

Which is what my patch does!!!

comment:9 Changed 10 years ago by jason

Ah, I see I just volunteered to review this :)

comment:10 Changed 10 years ago by dimpase

  • Status changed from needs_review to needs_info

a) cannot repeat this with rkirov's flask version of nb and twisted-11 installed.

b) this is a duplicate of #8995

IMHO this should not be fixed. Let's do the upgrades instead!

comment:11 follow-up: Changed 10 years ago by kcrisman

Fair enough, but WHEN will the upgrades happen? We really should not be releasing "real" releases of Sage that admins are using for nb servers without fixing at least the documentation. I don't know what the current status is. I don't think that the documentation fix was merged in sagenb, despite a number of pesters.

Is there a specific server I can test this on, Dima? If there is a sense that flask will be merged soon AND this fixes it, great. If not...

Also, did you try it with several browsers? I don't think it's browser-dependent but it's worth checking.

I'm going to mark #8995 as the dup, just because this has more discussion and a patch, though I don't know if it still applies to sagenb.

comment:12 Changed 10 years ago by kcrisman

  • Description modified (diff)

comment:13 in reply to: ↑ 11 Changed 10 years ago by dimpase

Replying to kcrisman:

Fair enough, but WHEN will the upgrades happen? We really should not be releasing "real" releases of Sage that admins are using for nb servers without fixing at least the documentation. I don't know what the current status is. I don't think that the documentation fix was merged in sagenb, despite a number of pesters.

Is there a specific server I can test this on, Dima? If there is a sense that flask will be merged soon AND this fixes it, great. If not...

I'm looking into it now. You can try meanwhile using test.sagenb.org, which runs twisted 11. I'll open a ticket for upgrading twisted to version 11 real soon... The initial spkg and a patch to have it running on Rado's flack notebook is already on http://boxen.math.washington.edu/home/dima/packages/ and http://boxen.math.washington.edu/home/dima/patches/

Also, did you try it with several browsers? I don't think it's browser-dependent but it's worth checking.

no, I didn't.

I'm going to mark #8995 as the dup, just because this has more discussion and a patch, though I don't know if it still applies to sagenb.

comment:14 follow-up: Changed 10 years ago by kcrisman

  • Status changed from needs_info to needs_review

Same problem. See attached screenshot. You can check this out with username abc@abc with password 123123.

Changed 10 years ago by kcrisman

comment:15 Changed 10 years ago by kcrisman

  • Priority changed from critical to blocker

I'm making this a blocker for 4.7.2. This is totally unacceptable that we do not at least change the login screen to avoid @ until this is fixed. Sorry, Leif :)

comment:16 in reply to: ↑ 14 ; follow-up: Changed 10 years ago by dimpase

  • Status changed from needs_review to needs_info

Replying to kcrisman:

Same problem. See attached screenshot. You can check this out with username abc@abc with password 123123.

I tried with stock Firefox, and Chrome on MacOSX 10.6.8... I cannot reproduce it; please state the browser, OS, (and medication ;-)) you are on.

I can reproduce it with the stock Safari on the said platform (this also is not working with the updated version of tiny-mce, locally).

Oh well, we all but banned IE from using the nb, we should ban other closed-source browsers. I seldom use Safari nowadays. Safari sucks. So is IE. Don't use them for serious business.

comment:17 in reply to: ↑ 16 Changed 10 years ago by kcrisman

Same problem. See attached screenshot. You can check this out with username abc@abc with password 123123.

I tried with stock Firefox, and Chrome on MacOSX 10.6.8... I cannot reproduce it; please state the browser, OS, (and medication ;-)) you are on.

I can reproduce it with the stock Safari on the said platform (this also is not working with the updated version of tiny-mce, locally).

Okay, fair enough. Hmm, I thought I had seen it on FF. Checking...

Oh well, we all but banned IE from using the nb, we should ban other closed-source browsers. I seldom use Safari nowadays. Safari sucks. So is IE. Don't use them for serious business.

Well, it does load significantly faster (for me), and is standards-compliant enough for me. What about Opera?

Also, I think it is unreasonable to ban these both longterm. This ticket isn't for old hands, it's so that new users don't think Sage sucks. Even with IE (where I agree with you), most of the world is using it, still, and we don't want people to not use Sage because they don't want to download a new browser or whatever.

Anyway, we need to do something with this issue. If we aren't merging flask, or whatever fixed this in FF, in 4.7.2, then this should still be fixed for now. See #11470 for the followup once we do.

comment:18 follow-up: Changed 10 years ago by dimpase

  • Cc ppurka added

Basu, care to try this on Opera?

Changed 10 years ago by ppurka

Works fine in opera-11.51 even without the patch. (sage-4.7.0)

comment:19 in reply to: ↑ 18 ; follow-up: Changed 10 years ago by ppurka

Replying to dimpase:

Basu, care to try this on Opera?

It works fine in opera even without the patch. But @ should not have been allowed in usernames anyway because of this message that I got while creating the username:

The username must start with a letter and be between 4 and 32 characters long. It can only consist of letters, numbers, underscores, and one dot (.).

comment:20 in reply to: ↑ 19 Changed 10 years ago by ppurka

Replying to ppurka:

Replying to dimpase:

Basu, care to try this on Opera?

It works fine in opera even without the patch. But @ should not have been allowed in usernames anyway because of this message that I got while creating the username:

The username must start with a letter and be between 4 and 32 characters long. It can only consist of letters, numbers, underscores, and one dot (.).

Sorry, I am not thinking straight today. I forgot that I am running rkirov-flask on the local installation (installed in $HOME). I tried the system installation also (installed in /) which is sage-4.7.1 and it is using the old notebook and the tiny-mce part works fine. (I tested it from the admin account since I am unable to login as any user, even admin, from the sign-in page. This is a separate issue and is unrelated to this bug.)

Changed 10 years ago by ppurka

Neglect the previous *opera*.png. This is on a fresh .sagenb directory (sage-4.7.1)

comment:21 follow-up: Changed 10 years ago by ppurka

I created a fresh /tmp/a.sagenb directory and ran sage -n on that directory. The bug is there in opera-11.51 with sage-4.7.1, amd64 Gentoo Linux. Sorry for all the noise I created earlier.

I believe my ~/.sage directory was being written to by both sage-4.7 (with rkirov-flask notebook) and sage-4.7.1 (with old notebook) and so I wasn't seeing this bug earlier.

comment:22 in reply to: ↑ 21 Changed 10 years ago by dimpase

Replying to ppurka:

I created a fresh /tmp/a.sagenb directory and ran sage -n on that directory. The bug is there in opera-11.51 with sage-4.7.1, amd64 Gentoo Linux. Sorry for all the noise I created earlier.

I believe my ~/.sage directory was being written to by both sage-4.7 (with rkirov-flask notebook) and sage-4.7.1 (with old notebook) and so I wasn't seeing this bug earlier.

I cannot reproduce this on Opera 11.51, Build 1087, on Mac OS X 10.6.8 on flack notebook with the new tiny_mce 3.4.5. Could you please try getting the tiny_mce upgrade from here: http://boxen.math.washington.edu/home/dima/packages/tiny_mce-3.4.5.tar.bz replace your SAGE_ROOT/devel/....../sagenb/data/tiny_mce/ with the contents of this archive, and try again?

comment:23 Changed 10 years ago by ppurka

In gist, here is the current status of when it works: it works with flask notebook, irrespective of tiny-mce version.

  • sage-4.7.0 + flask nb + tiny-mce (old) + opera-11.51: Works
  • sage-4.7.0 + flask nb + tiny-mce-3.4.5 + opera-11.51: Works
  • sage-4.7.1 + old nb + tiny-mce (old) + opera-11.51: Does not work
  • sage-4.7.1 + old nb + tiny-mce-3.4.5 + opera-11.51: Does not work
  • sage-4.7.1 + old nb + tiny-mce-3.4.5 + firefox-6: Does not work

comment:24 follow-up: Changed 10 years ago by jdemeyer

This ticket is listed as blocker, what is the current status? It seems like disallowing users to register usernames with @ could be a quick and eays solution. But they should still be able to log in with any existing username.

comment:25 in reply to: ↑ 24 Changed 10 years ago by dimpase

  • Priority changed from blocker to major

Replying to jdemeyer:

This ticket is listed as blocker, what is the current status? It seems like disallowing users to register usernames with @ could be a quick and eays solution. But they should still be able to log in with any existing username.

it's should not be a blocker, as a workaround is to use another browser. I therefore take the liberty to make it just major instead...

comment:26 Changed 10 years ago by kcrisman

  • Authors set to Karl-Dieter Crisman
  • Description modified (diff)
  • Priority changed from major to critical
  • Status changed from needs_info to needs_review

Replying to jdemeyer:

This ticket is listed as blocker, what is the current status? It seems like disallowing users to register usernames with @ could be a quick and eays solution. But they should still be able to log in with any existing username.

Which is precisely what trac_11343-fix_at_symbol.patch does. I just checked and it still applies cleanly... I just feel this is better than waiting eternally for the flask or TinyMCE update.

I really disagree that it's major and not blocker. Sage doesn't work properly, and saying "use another browser" is going to keep on being said until we have zero browsers that support it. If ppurka finds it doesn't work with FF or Opera with the new TinyMCE, and it doesn't work on Safari, and we already suggest (though nowhere official!!!) that IE is gone... should we be sending people to lynx?

So critical. But if someone just reviews this patch, it fixes the problem, and that is better. Plus it lights a fire under the flask/sagenb people to finish things off so we don't mess with things any more.

comment:27 Changed 10 years ago by jdemeyer

  • Description modified (diff)

comment:28 Changed 10 years ago by jdemeyer

  • Status changed from needs_review to positive_review

Patch does the job. New registrations with @ are not possible, existing ones can still log in.

comment:29 Changed 10 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer

comment:30 Changed 10 years ago by jdemeyer

  • Priority changed from critical to blocker
  • Summary changed from TinyMCE won't work with @ in the username of notebook to Disallow @ symbol in username because of TinyMCE problems

comment:31 Changed 10 years ago by jdemeyer

  • Merged in set to sage-4.7.2.rc0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.