Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sagenb: notebook cookies #8249

Closed
williamstein opened this issue Feb 12, 2010 · 18 comments
Closed

sagenb: notebook cookies #8249

williamstein opened this issue Feb 12, 2010 · 18 comments

Comments

@williamstein
Copy link
Contributor

This is a followup to #6353. That ticket improved cookie naming a bit. However, it appears to be not enough.

On Thu, Feb 11, 2010 at 7:21 PM, Marshall Hampton <> wrote:
> Just for the record, this has happened to me quite a bit recently.
>
> I use a lot of different sage servers, often running different
> versions, so I don't usually report this kind of stuff since I think I
> am something of an extreme case.  But most of the servers I use are
> now running 4.3.2 and I am pretty sure I have seen the cookie message
> more than before.
>

Here's the relevant ticket I was remembering:

     https://github.com/sagemath/sage-prod/issues/6353

It is definitely in sage-4.3.2 (since it is merged into sagenb-0.7.4).   

Looking at that patch show that:

  (1) it addresses a related issue,

  (2) it might not solve the issue we're discussing, since it merely includes the *port* in the cookie name -- some unique id for the notebook (e.g., the URL or something else) is maybe also needed to fix the problem we're discussing.

So somebody should look at ticket 6353, see if a small modification of it would give a real fix, and make said modification.      Alex Leone: this would be a good project for you, if you're looking for something to do on the notebook. 

 -- William

See sage-support.

CC: @sagetrac-acleone @qed777 @sagetrac-mhampton

Component: notebook

Author: Mitesh Patel

Reviewer: Alex Leone, Tim Dumol

Merged: sagenb-0.8

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

@sagetrac-acleone
Copy link
Mannequin

sagetrac-acleone mannequin commented Feb 12, 2010

comment:1

I couldn't reproduce his by signing in/out of sagenb.org and demo.sagenb.org. Cookies are stored by domain (sagenb.org and demo.sagenb.org are two seperate sites), so differentiating by port should be all that's necessary.

I made sure all instances of 'cookie' in twist.py were updated from #6353 (they were), so I really have no idea what is causing this. It could be a subtle bug in the 'cookie_test' cookie.

@qed777

This comment has been minimized.

@qed777
Copy link
Mannequin

qed777 mannequin commented Feb 14, 2010

comment:4

Has anyone been able to reproduce the problem reliably? It would help greatly to have specific instructions.

@qed777
Copy link
Mannequin

qed777 mannequin commented Feb 14, 2010

Expire cookies on logout. sagenb repo.

@qed777
Copy link
Mannequin

qed777 mannequin commented Feb 14, 2010

comment:5

Attachment: trac_8249-notebook_cookies.patch.gz

I've attached a patch that should delete both the test and notebook session cookies, when a user logs out.

@qed777
Copy link
Mannequin

qed777 mannequin commented Feb 14, 2010

comment:6

The patch may depend weakly on #6069.

@qed777
Copy link
Mannequin

qed777 mannequin commented Feb 14, 2010

Author: Mitesh Patel

@qed777 qed777 mannequin added s: needs review and removed p: minor / 4 labels Feb 14, 2010
@sagetrac-acleone
Copy link
Mannequin

sagetrac-acleone mannequin commented Feb 14, 2010

comment:8

LGTM: I can't produce any cookie errors through normal use.

However, selenium errors when logging out - I'm working on a new patch to fix the tests. (See attached for log)

@sagetrac-acleone
Copy link
Mannequin

sagetrac-acleone mannequin commented Feb 14, 2010

Selenium errors with patch. Same errors when the patch for #6069 -missing_pub_ws.3 is applied.

@qed777
Copy link
Mannequin

qed777 mannequin commented Feb 14, 2010

comment:9

Attachment: trac_8249-notebook_cookies-selenium_errors.log

Thanks for catching the Se test errors. I should have checked.

The patch fixes for me the one cookie-related problem I could reproduce reliably: Logging out in Chrom* displays a browser error page:

This webpage has a redirect loop.

The webpage at http://localhost:8000/home/admin/ has resulted in too many redirects. Clearing your cookies for this site or allowing third-party cookies may fix the problem. If not, it is possibly a server configuration issue and not a problem with your computer.

But with the patch, clicking on "Sign Out" just returns me to the login page. I'm not sure if it helps with the reported problems, but making the cookies expire on logout seems logical.

@sagetrac-acleone
Copy link
Mannequin

sagetrac-acleone mannequin commented Feb 15, 2010

comment:10

For some reason Selenium doesn't like HTTP redirect responses. I keep getting this 'Problem loading page' error (in firefox):

The page isn't redirecting properly
      
Firefox has detected that the server is redirecting the request for this address in a way that will never complete.

    *   This problem can sometimes be caused by disabling or refusing to accept
          cookies.

This always happens when the selenium gets the redirect HTTP response. Perhaps we should change it back to a dedicated logout page, but add a <meta http-equiv="Refresh" tag so the page redirects after a second.

@qed777
Copy link
Mannequin

qed777 mannequin commented Feb 15, 2010

Adjust close_callback to make Se tests pass. Apply only this patch.

@qed777
Copy link
Mannequin

qed777 mannequin commented Feb 15, 2010

comment:11

Attachment: trac_8249-notebook_cookies.2.patch.gz

V2 replaces '/' with '/home/' + user_name in notebook_lib.js's close_callback. Strangely, this seems to work.

@qed777 qed777 mannequin added s: needs review and removed s: needs work labels Feb 16, 2010
@TimDumol
Copy link
Mannequin

TimDumol mannequin commented Mar 19, 2010

comment:13

I'm signing this off since it's a good idea, and may help. I'm unable to replicate the cookie issue though, with or without this patch, but it may be related to the performance issues of the sagenb server.

@TimDumol
Copy link
Mannequin

TimDumol mannequin commented Mar 19, 2010

Reviewer: Tim Dumol

@TimDumol
Copy link
Mannequin

TimDumol mannequin commented Mar 19, 2010

comment:15

Woops. Forgot to add Alex.

@TimDumol
Copy link
Mannequin

TimDumol mannequin commented Mar 19, 2010

Changed reviewer from Tim Dumol to Alex Leone, Tim Dumol

@TimDumol
Copy link
Mannequin

TimDumol mannequin commented Apr 24, 2010

Merged: sagenb-0.8

@TimDumol TimDumol mannequin removed the s: positive review label May 4, 2010
@TimDumol TimDumol mannequin closed this as completed May 4, 2010
@TimDumol TimDumol mannequin mentioned this issue May 3, 2010
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant