markbates / goth

Package goth provides a simple, clean, and idiomatic way to write authentication packages for Go web applications.
https://blog.gobuffalo.io/goth-needs-a-new-maintainer-626cd47ca37b
MIT License
5.64k stars 601 forks source link

No documentation on why there is a defer logout #548

Open lil5 opened 7 months ago

lil5 commented 7 months ago

https://github.com/markbates/goth/blob/f4685f5f6edf65de920b6c6c03fc1ffabcb88e68/gothic/gothic.go#L180

func CompleteUserAuth In this function, why is there a defer to Logout?


I've been trying to figure out why my use of gothic is returning a cookie (_gothic_session) with an expires of "1970-01-01T00:00:01.000Z".

Yeicor commented 6 months ago

I was thinking the same thing and ended up copying the whole function and commenting the defer Logout() line. I also moved the validateState(...) check after FetchUser(...) to let CompleteUserAuth(...) work for already logged-in users like in the main example.

Here's the code:

var CompleteUserAuthNoLogout = func(res http.ResponseWriter, req *http.Request) (goth.User, error) {
    providerName, err := gothic.GetProviderName(req)
    if err != nil {
        return goth.User{}, err
    }

    provider, err := goth.GetProvider(providerName)
    if err != nil {
        return goth.User{}, err
    }

    value, err := gothic.GetFromSession(providerName, req)
    if err != nil {
        return goth.User{}, err
    }
    //HACK: Removed defer gothic.Logout(res, req)
    sess, err := provider.UnmarshalSession(value)
    if err != nil {
        return goth.User{}, err
    }

    user, err := provider.FetchUser(sess)
    if err == nil {
        // user can be found with existing session data
        return user, err
    }

    // HACK: validateState only after FetchUser fails
    err = validateState(req, sess)
    if err != nil {
        return goth.User{}, err
    }

    params := req.URL.Query()
    if params.Encode() == "" && req.Method == "POST" {
        _ = req.ParseForm()
        params = req.Form
    }

    // get new token and retry fetch
    _, err = sess.Authorize(provider, params)
    if err != nil {
        return goth.User{}, err
    }

    err = gothic.StoreInSession(providerName, sess.Marshal(), req, res)

    if err != nil {
        return goth.User{}, err
    }

    gu, err := provider.FetchUser(sess)
    return gu, err
}

func validateState(req *http.Request, sess goth.Session) error {
    rawAuthURL, err := sess.GetAuthURL()
    if err != nil {
        return err
    }

    authURL, err := url.Parse(rawAuthURL)
    if err != nil {
        return err
    }

    reqState := gothic.GetState(req)

    originalState := authURL.Query().Get("state")
    if originalState != "" && (originalState != reqState) {
        return errors.New("state token mismatch")
    }
    return nil
}

Am I doing something wrong? Should I submit this as a PR?

tmstorm commented 6 months ago

Since gothic uses Gorilla Sessions by default it creates a state cookie when starting authentication to verify on callback. The defer logout is meant to delete the state cookie after auth is completed. StoreInSession doesn't really work the way I would like it to. For this reason I am managing the state and session on my own. See this comment I made previously.

I also made my own CompleteUserAuth since I do not want to use the Gorilla Sessions. Since CompleteUserAuth is a var its very easy to make your own function and use that instead.

I also made a function to check the sessions validity and attempt to silently get a new access token on expiration. This is called every time an API call happens.

jessedegans commented 2 months ago

The defer logout makes the whole standard flow of trying to reauth obsolete.

// try to get the user without re-authenticating
  if gothUser, err := gothic.CompleteUserAuth(res, req); err == nil { //this will always be FALSE
  } else {
      gothic.BeginAuthHandler(res, req)
  }

Doesn't make sense to call defer logout.

dotneutron commented 1 month ago

This could be a countermeasure for session fixation.

If you use a Redis / DB session store and return the session ID to the client, you should invalidate the current session on successful login, generate a new one with a different ID and whatever user data, and return the new session ID to the client.