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

No documentation on why there is a defer logout #548

Open
lil5 opened this issue Apr 16, 2024 · 5 comments
Open

No documentation on why there is a defer logout #548

lil5 opened this issue Apr 16, 2024 · 5 comments

Comments

@lil5
Copy link

lil5 commented Apr 16, 2024

defer Logout(res, req)

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
Copy link

Yeicor commented May 16, 2024

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
Copy link

tmstorm commented May 16, 2024

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
Copy link

jessedegans commented Sep 11, 2024

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
Copy link

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.

@Nintron27 Nintron27 mentioned this issue Dec 19, 2024
@sivaplaysmC
Copy link

@markbates Please it will be very helpful if you could explain the purpose of defer Logout().

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

No branches or pull requests

6 participants