alexedwards / scs

HTTP Session Management for Go
MIT License
2.09k stars 166 forks source link

Get Session ID only from request #87

Closed aight8 closed 2 years ago

aight8 commented 4 years ago

The only way to get the session token ("ID") is while the commit...

token, expiry, err = Manager.Commit(s.ctx)
alexedwards commented 4 years ago

This is intentional. By only surfacing the session token at the point of commit, we know that the session token and information have been persisted in the database.

If we create and surface the session token before then, there is the possibility that the session token and data may never actually be committed and you would have a 'ghost' session token which doesn't correspond to anything on future requests.

I know this was a couple of months ago, but can you describe the scenario where you needed this?

jum commented 4 years ago

I had to access the session ID in oauth2 workflows and instead added a function to data.go instead to circumvent this:

MergeSession addition

alexedwards commented 3 years ago

It's not clear to me what action, if any, needs to be taken here.

Is there a problem with calling Manager.Commit() to get the token? As mentioned above, I don't want to expose a Token() method as that would risk returning a session token which isn't actually ever committed to the database.

If we did add a Token() method, I guess it could return an additional boolean value which indicates whether the session token has been committed to the database or not. Then people can use it at their own risk.

jum commented 3 years ago

In my case, I am happy with the MergeSession logic. In my case the need for it was due to the following flow:

alexedwards commented 3 years ago

@aight8 I'd like to finalize what (if anything) needs to be done here to close this issue. As far as I can see there are 3 options:

  1. Leave the package as-is.
  2. Add a Token() method which exposes the session token, with no other changes. This would return the empty string "" if the session hasn't been committed yet.
  3. Change the behavior of SCS so that session tokens are generated when the session is first created in newSession(), rather than at the point of commit. Then add a Token() method which exposes the session token along with a boolean to indicate whether the session has been committed or not.

I would prefer not to implement the third option, as it would mean making quite a lot of changes to the package and it's also wasteful, because in some scenarios the package will be making crypto/rand calls to create a session token that is never actually committed.

alexedwards commented 2 years ago

Commit e529b5c877aea52bae4a87e9feff2b9f2298f2cc adds a new Token() method which exposes the session token. It uses approach number 2 in the list above, and returns the empty string "" if the session hasn't been committed yet.

I think this issue can probably now be closed.

scippio commented 10 months ago

I'm sorry for writing to closed issue, but when I call the sessionManager.Token(request.Context()) I get only panic with: "scs: no session data in context" error. I expected there empty string because: "Please note that this will return the empty string "" if it is called before the session has been committed to the store."

It's because I call it before LoadAndSave()? @alexedwards

jum commented 10 months ago

Which is expected, as Token gets the session data from the context. The context is established by LoadAndSave, so if you want to fiddle with the session data before LoadAndSave, you would have to fetch the data yourself. I do not thing that would be a recommendable thing to do, as it would fetch the session data twice from storage.

scippio commented 10 months ago

I see... thank you. I have something like this now:

sessionManager.LoadAndSave(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
    if _, err := r.Cookie(sessionManager.Cookie.Name); err == http.ErrNoCookie {
        var sessionID string
        sessionManager.Put(r.Context(), "new", 1)
        if tkn, _, err := sessionManager.Commit(r.Context()); err == nil {
            sessionID = tkn
        } else {
            log.WithError(err).WithContext(r.Context()).Error("failed to create session")
        }
        log.Debugf("session token: %s", sessionID)
    }

    appHandler.ServeHTTP(w, r)

})).ServeHTTP(w, r)
alexedwards commented 10 months ago

@scippio Exactly. You always need to call the LoadAndSave middleware on any routes that read or write session data.