search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.38k stars 471 forks source link

Disallow @ symbol in username because of TinyMCE problems #11343

Closed kcrisman closed 13 years ago

kcrisman commented 13 years ago

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 attachment: trac_11343-fix_at_symbol.patch to the SAGENB repository.

Upstream: Reported upstream. Little or no feedback.

CC: @jasongrout @boothby @williamstein @qed777 @dandrake @ppurka

Component: notebook

Keywords: notebook, tinymce, at, sd31

Author: Karl-Dieter Crisman

Reviewer: Jeroen Demeyer

Merged: sage-4.7.2.rc0

Issue created by migration from https://trac.sagemath.org/ticket/11343

kcrisman commented 13 years ago
comment:1

Attachment: sageproblem.JPG.gz

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.

kcrisman commented 13 years ago

Upstream: Not yet reported upstream; Will do shortly.

kcrisman commented 13 years ago
comment:2

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.

kcrisman commented 13 years ago

Changed keywords from notebook, tinymce, at to notebook, tinymce, at, sd31

kcrisman commented 13 years ago
comment:3

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

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

kcrisman commented 13 years ago

Attachment: trac_11343-fix_at_symbol.patch.gz

kcrisman commented 13 years ago

Changed upstream from Not yet reported upstream; Will do shortly. to Reported upstream. Little or no feedback.

kcrisman commented 13 years ago
comment:4

This is reported upstream at this link.

kcrisman commented 13 years ago
comment:5

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.

kcrisman commented 13 years ago
comment:6

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.

jasongrout commented 13 years ago
comment:7

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

kcrisman commented 13 years ago
comment:8

Replying to @jasongrout:

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!!!

jasongrout commented 13 years ago
comment:9

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

dimpase commented 13 years ago
comment:10

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!

kcrisman commented 13 years ago
comment:11

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.

kcrisman commented 13 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,15 @@
 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](http://groups.google.com/group/sage-notebook/browse_thread/thread/f0ceaa24700f4059).

 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!!!
+
+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
+
dimpase commented 13 years ago
comment:13

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.

kcrisman commented 13 years ago
comment:14

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

kcrisman commented 13 years ago
comment:15

Attachment: test.sagenb-failure.png

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 :)

dimpase commented 13 years ago
comment:16

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.

kcrisman commented 13 years ago
comment:17

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.

dimpase commented 13 years ago
comment:18

Basu, care to try this on Opera?

ppurka commented 13 years ago

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

ppurka commented 13 years ago
comment:19

Attachment: works_fine_without_patch_opera-11.51.png

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 (.).

ppurka commented 13 years ago
comment:20

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.)

ppurka commented 13 years ago

Attachment: opera-11.51_sage-4.7.1_does_not_work.png

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

ppurka commented 13 years ago
comment:21

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.

dimpase commented 13 years ago
comment:22

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?

ppurka commented 13 years ago
comment:23

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

jdemeyer commented 13 years ago
comment:24

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.

dimpase commented 13 years ago
comment:25

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...

kcrisman commented 13 years ago

Description changed:

--- 
+++ 
@@ -1,6 +1,10 @@
 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](http://groups.google.com/group/sage-notebook/browse_thread/thread/f0ceaa24700f4059).

 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:
kcrisman commented 13 years ago

Author: Karl-Dieter Crisman

kcrisman commented 13 years ago
comment:26

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 attachment: 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.

jdemeyer commented 13 years ago

Description changed:

--- 
+++ 
@@ -17,3 +17,6 @@

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

+---
+
+**Apply** [attachment: trac_11343-fix_at_symbol.patch](https://github.com/sagemath/sage-prod/files/10652885/trac_11343-fix_at_symbol.patch.gz) to the SAGENB repository.
jdemeyer commented 13 years ago
comment:28

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

jdemeyer commented 13 years ago

Reviewer: Jeroen Demeyer

jdemeyer commented 13 years ago

Merged: sage-4.7.2.rc0