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

Not able to delete sessions by setting MaxAge = -1 #78

Closed
YGKtech opened this issue May 25, 2016 · 18 comments
Closed

Not able to delete sessions by setting MaxAge = -1 #78

YGKtech opened this issue May 25, 2016 · 18 comments
Assignees
Labels

Comments

@YGKtech
Copy link

YGKtech commented May 25, 2016

It's possible I'm doing something wrong, but I can't seem to find any documentation that shows a different approach.

Here is my initialization code:
(1)

  cookieStore = sessions.NewCookieStore(
    []byte(securecookie.GenerateRandomKey(64))[:64],
    []byte(securecookie.GenerateRandomKey(32))[:32])
  cookieStore.Options = &sessions.Options{
    MaxAge:   86400 * 7, // Session valid for one week.
    HttpOnly: true,
  }

Here's how sessions are added to the cookie store:
(2)


  c := appengine.NewContext(r)
  s, _ := cookieStore.Get(r, SessionName)

  if s.IsNew {

    _, ok := s.Values[sessionUserKey]

    if !ok {

      log.Debugf(c, "adding user to session")
      s.Values[sessionUserKey] = *u
      err := s.Save(r, w)
      if err != nil {
        log.Errorf(c, "Cannot save session: %s", err)
      }

    } else {
      log.Debugf(c, "user already exists in session")
    }
  }

and here is the code that fails to delete the users session:
(3)

  c := appengine.NewContext(r)

  s, _ := cookieStore.Get(r, SessionName)
  s.Options = &sessions.Options{
    MaxAge: -1,
  } 
  // s.Options.MaxAge = -1
  // Alternate syntax I've tried, exactly the same behavior.
  err := s.Save(r, w)
  if err != nil {
    log.Errorf(appengine.NewContext(r), "Cannot save session: %s", err)
  }

  v, ok := s.Values[sessionUserKey]
  if !ok {
    log.Debugf(c, "no user found in current session")
  } else {
    log.Debugf(c, "found user: %v", v)
  }

  http.SetCookie(w, &http.Cookie{Name: GtokenCookieName, MaxAge: -1})

The behavior is oddly inconsistent, if I attempt to delete the session immediately after creating it, it will deleted, but if I then create another session by signing in to my site with the same user account, subsequent attempts to delete the session will fail. If repeatedly run code segment (3) it will log a user object every time.

I have found a few circumstances in which the code will appear to delete the session, but upon running it again the session will reappear, Not sure what that says about the underlying issue, but it is peculiar.

@elithrar
Copy link
Contributor

session.Get will create a new session if one doesn't exist, which means
that making another request will generate a new session.

(I'll take a deeper look at this later today if you are still having
issues!)
On Wed, May 25, 2016 at 10:22 AM YGKtech notifications@github.com wrote:

It's possible I'm doing something wrong, but I can't seem to find any
documentation that shows a different approach.

Here is my initialization code:
(1)

cookieStore = sessions.NewCookieStore(
[]byte(securecookie.GenerateRandomKey(64))[:64],
[]byte(securecookie.GenerateRandomKey(32))[:32])
cookieStore.Options = &sessions.Options{
MaxAge: 86400 * 7, // Session valid for one week.
HttpOnly: true,
}

Here's how sessions are added to the cookie store:
(2)

c := appengine.NewContext(r)
s, _ := cookieStore.Get(r, SessionName)

if s.IsNew {

_, ok := s.Values[sessionUserKey]

if !ok {

  log.Debugf(c, "adding user to session")
  s.Values[sessionUserKey] = *u
  err := s.Save(r, w)
  if err != nil {
    log.Errorf(c, "Cannot save session: %s", err)
  }

} else {
  log.Debugf(c, "user already exists in session")
}

}

and here is the code that fails to delete the users session:
(3)

c := appengine.NewContext(r)

s, _ := cookieStore.Get(r, SessionName)
s.Options = &sessions.Options{
MaxAge: -1,
}
// s.Options.MaxAge = -1
// Alternate syntax I've tried, exactly the same behavior.
err := s.Save(r, w)
if err != nil {
log.Errorf(appengine.NewContext(r), "Cannot save session: %s", err)
}

v, ok := s.Values[sessionUserKey]
if !ok {
log.Debugf(c, "no user found in current session")
} else {
log.Debugf(c, "found user: %v", v)
}

http.SetCookie(w, &http.Cookie{Name: GtokenCookieName, MaxAge: -1})

The behavior is oddly inconsistent, if I attempt to delete the session
immediately after creating it, it will deleted, but if I then create
another session by signing in to my site with the same user account,
subsequent attempts to delete the session will fail. If repeatedly run code
segment (3) it will log a user object every time.

I have found a few circumstances in which the code will appear to delete
the session, but upon running it again the session will reappear, Not sure
what that says about the underlying issue, but it is peculiar.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#78

@YGKtech
Copy link
Author

YGKtech commented May 25, 2016

Thanks for the input, though I don't think that is the issue I'm facing.

It would probably be fine if a new session were being created, but I'm getting back the same session, or at least a new one with exactly the same properties.

What exactly is the expected behavior when reading from a session immediately after setting it's MaxAge to -1? Or, to look at it from another perspective, how/when is the session actually removed from memory after it's age is set. I've tried every permutation of calling sessions.Save(), s.Save(), and cookieStore.Save() (as well as not saving at all), but the session seems unaffected.

@elithrar
Copy link
Contributor

To clarify: are you saying it's not deleted on the next request, or that in the same request, after calling Save, that the session still exists?

sessions are only written when the handler writes the response headers. sessions aren't persisted in memory across requests.

@YGKtech
Copy link
Author

YGKtech commented May 25, 2016

sessions are only written when the handler writes the response headers

That's a detail I never found in the documentation. I'll have to re-do some of my testing with that information in mind, but I'm working on something else at the moment so I may not have an answer for you until later tonight.

Thanks for the help.

@elithrar
Copy link
Contributor

Just to be clear: this is the case because a cookie is a response header
(Set-Cookie).

If you call Save and then expect the cookie to be deleted immediately then
I would rethink your design (saving/deleting the session should be the last
thing you do before returning).

On Wed, May 25, 2016 at 12:52 PM YGKtech notifications@github.com wrote:

sessions are only written when the handler writes the response headers
That's a detail I never found in the documentation. I'll have to re-do
some of my testing with that information in mind, but I'm working on
something else at the moment so I may not have an answer for you until
later tonight.

Thanks for the help.


You are receiving this because you commented.

Reply to this email directly or view it on GitHub
#78 (comment)

@YGKtech
Copy link
Author

YGKtech commented May 26, 2016

I've confirmed that the session object is not being deleted after the response is written. The cookie is being deleted, but the session is not.

After I set MaxAge = -1 and write the headers, i hit the endpoint with a new request, then s, err := cookieStore.Get(r, SessionName) returns a non-new session object containing all the details of the user object from the previous request, and a nil error object.

@elithrar
Copy link
Contributor

Can you provide a minimal, compilable example I can run locally to replicate?

@elithrar elithrar self-assigned this May 26, 2016
@YGKtech
Copy link
Author

YGKtech commented May 26, 2016

I'll try and put something together tonight, but I'm seriously considering refactoring to use a new package due to time constraints on the project.

@elithrar
Copy link
Contributor

I'm happy to look at your code tonight, but as no one else has run into
this case before, I suspect the issue lies between your usage and the
project documentation.

A subsequent request should not share any state with the preceeding one.
On Thu, May 26, 2016 at 1:44 PM YGKtech notifications@github.com wrote:

I'll try and put something together tonight, but I'm seriously considering
refactoring to use a new package due to time constraints on the project.


You are receiving this because you were assigned.

Reply to this email directly or view it on GitHub
#78 (comment)

@elithrar
Copy link
Contributor

elithrar commented May 26, 2016

I cannot replicate the issue: the session is empty after setting MaxAge = -1 and saving it (on the next request).

package main

import (
    "fmt"
    "log"
    "net/http"

    "github.com/gorilla/mux"
    "github.com/gorilla/sessions"
)

var store = sessions.NewCookieStore([]byte("key-1"), nil)

func main() {
    mux := mux.NewRouter()
    mux.HandleFunc("/set", SetHandler)
    mux.HandleFunc("/delete", DeleteHandler)

    log.Fatal(http.ListenAndServe("localhost:8000", mux))
}

func SetHandler(w http.ResponseWriter, r *http.Request) {
    sess, err := store.Get(r, "app")
    if err != nil {
        http.Error(w, err.Error(), 400)
        return
    }

    log.Printf("value before: %v", sess.Values["user"])

    sess.Values["user"] = "gorilla!"
    err = sess.Save(r, w)
    if err != nil {
        http.Error(w, err.Error(), 400)
        return
    }

    fmt.Fprintf(w, "Cookie set to %v", sess.Values)
}

func DeleteHandler(w http.ResponseWriter, r *http.Request) {
    sess, err := store.Get(r, "app")
    if err != nil {
        http.Error(w, err.Error(), 400)
        return
    }

    sess.Options.MaxAge = -1

    err = sess.Save(r, w)
    if err != nil {
        http.Error(w, err.Error(), 400)
        return
    }
}

t=0 http://localhost:8000/set
t=1 2016/05/26 16:34:26 value before:
t=2 Cookie set to map[user:gorilla!]
t=3 http://localhost:8000/delete
t=4 2016/05/26 16:35:32 value before:
t=5 Cookie set to map[user:gorilla!]

@YGKtech
Copy link
Author

YGKtech commented May 26, 2016

Thanks for all the help, I will compare my implementation to yours and try to determine what I'm doing differently to encounter this behavior.

@YGKtech
Copy link
Author

YGKtech commented May 27, 2016

OK, I don't understand exactly what the problem was, but I've gotten it working, thank you for helping.

As far as I can tell my handler function for deleting the session was getting a different session object than the routes that requested data from the session, or it was somehow getting read-only access to the session. The solution for me was to have my client-side code send a request to my SetHandler equivalent, with a query param which would cause it to pass the request off to the DeleteHandler equivalent. It's a hackey solution, but it does work.

Exactly why this works is still a bit of a mystery to me. Conceptually my code was doing the same thing as yours (which functions as intended on my machine, so it's nothing about the environment). If I can find the spare time I will try to isolate what the source of the problem was, but I've already lost a lot of time to this.

Thanks again for all the help.

@elithrar
Copy link
Contributor

Glad I could help! One common issue can be that the Path in the cookie
isn't broad enough and results in handlers not "seeing" cookies.
On Fri, May 27, 2016 at 1:13 PM YGKtech notifications@github.com wrote:

OK, I don't understand exactly what the problem was, but I've gotten it
working, thank you for helping.

As far as I can tell my handler function for deleting the session was
getting a different session object than the routes that requested data from
the session, or it was somehow getting read-only access to the session. The
solution for me was to have my client-side code send a request to my
SetHandler equivalent, with a query param which would cause it to pass the
request off to the DeleteHandler equivalent. It's a hackey solution, but it
does work.

Exactly why this works is still a bit of a mystery to me. Conceptually my
code was doing the same thing as yours (which functions as intended on my
machine, so it's nothing about the environment). If I can find the spare
time I will try to isolate what the source of the problem was, but I've
already lost a lot of time to this.

Thanks again for all the help.


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#78 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AABIcFJLdl9u_-64JZF0Kg_Hqz8JDb-kks5qF1BigaJpZM4ImxLn
.

@elithrar
Copy link
Contributor

elithrar commented Jun 4, 2016

@YGKtech Let me know if you solved this. Happy to add more documentation if it helps others, but closing for now.

@elithrar elithrar closed this as completed Jun 4, 2016
@YGKtech
Copy link
Author

YGKtech commented Jun 6, 2016

@elithrar Oops, kinda forgot about this once the problem was taken care of.

I have solved the problem, at least to the extend I need to solve it. I was generating the session in a call to one api endpoint and trying to remove it in another, which simply didn't work. Once I moved the delete logic into the same route it began functioning properly. It's my understanding that this shouldn't be necessary if everything is set up correctly, but it is a viable solution for me.

I think a more detailed documentation of how cookies are scoped, and an official example of the lifecycle of a cookie/session system would be tremendously helpful, not just for people facing issues like mine, but for those setting up a session management system for the first time as well. I get the impression the documentation was written with experienced server developers in mind, so it skips over a lot of things that might not be so obvious to people who have not implemented their own session management before.

@elithrar
Copy link
Contributor

Always looking to improve documentation! What particular pieces about
lifecycle, how cookies work, etc would be helpful? I don't want to rehash
what a cookie is or how they work (I think that's covered in many other
places) but if there's anything about how that interacts with Go that's
confusing, I'd like to fix it.

On Mon, Jun 6, 2016 at 8:42 AM YGKtech notifications@github.com wrote:

@elithrar https://github.com/elithrar Oops, kinda forgot about this
once the problem was taken care of.

I have solved the problem, at least to the extend I need to solve it. I
was generating the session in a call to one api endpoint and trying to
remove it in another, which simply didn't work. Once I moved the delete
logic into the same route it began functioning properly. It's my
understanding that this shouldn't be necessary if everything is set up
correctly, but it is a viable solution for me.

I think a more detailed documentation of how cookies are scoped, and an
official example of the lifecycle of a cookie/session system would be
tremendously helpful, not just for people facing issues like mine, but for
those setting up a session management system for the first time as well. I
get the impression the documentation was written with experienced server
developers in mind, so it skips over a lot of things that might not be so
obvious to people who have not implemented their own session management
before.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#78 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AABIcAHuJCjbwE72FnIh9s7ys69_YAdTks5qJD_egaJpZM4ImxLn
.

@YGKtech
Copy link
Author

YGKtech commented Jun 10, 2016

I agree that you don't want to explain the basics of everything in your documentation, that would just make it harder to find information specific to your package. But I think it might be wise to include a comment along the lines of "gorilla follows a lot of standard conventions around the use of cookies, if you can't find an answer to a question here, consider looking at a more generalized guide to cookies and sessions", just so readers know what to expect from your documentation.

I think a specific section covering the process of deleting or modifying a session would be helpful. If you look at the current documentation there isn't a section about either of these, they are just footnotes in other sections. There should also be some discussion of the scoping of sessions in relation to routing, since it's recommended to use the sessions package in conjunction with muxRouter it would make sense to discuss the relationship between a session/cookie and the route they are created under.

@elithrar
Copy link
Contributor

OK, great! I think it makes sense to add some more structure to the docs:

  • Setting up the store
  • Creating a session
  • Adding, modifying and retrieving values from a session
  • Expiring a session
  • Common pitfalls (the cookie Path attribute being a big one)

That would make the docs easier to parse and address the typical workflow
more directly.

On Fri, Jun 10, 2016 at 10:05 AM YGKtech notifications@github.com wrote:

I agree that you don't want to explain the basics of everything in your
documentation, that would just make it harder to find information specific
to your package. But I think it might be wise to include a comment along
the lines of "gorilla follows a lot of standard conventions around the use
of cookies, if you can't find an answer to a question here, consider
looking at a more generalized guide to cookies and sessions", just so
readers know what to expect from your documentation.

I think a specific section covering the process of deleting or modifying a
session would be helpful. If you look at the current documentation there
isn't a section about either of these, they are just footnotes in other
sections. There should also be some discussion of the scoping of sessions
in relation to routing, since it's recommended to use the sessions package
in conjunction with muxRouter it would make sense to discuss the
relationship between a session/cookie and the route they are created under.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#78 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AABIcAOKlb1sLju_4ceZeWonn26bpjy4ks5qKZlqgaJpZM4ImxLn
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants