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

refactor: Provided an Option to Set Cookie using *GinJWTMiddleware #335

Merged

Conversation

maniSHarma7575
Copy link
Contributor

@maniSHarma7575 maniSHarma7575 commented Jul 14, 2024

Problem

  1. DRY Code: The SetCookie code is redundant we’ve duplicated the logic for setting cookies in both the LoginHandler and RefreshToken.
  2. We do have the func (*GinJWTMiddleware) TokenGenerator method for clients to generate JWT tokens independently of the LoginHandler or RefreshToken methods. However, without a dedicated SetCookie method, cookie setting remains tightly coupled with these methods. Introducing a SetCookie method would enhance scalability by enabling cookies to be set independently, eliminating the need for manual cookie handling where such functionality is currently absent.

Context:

This library looks very useful for us. However at the moment I don't think it supports oauth? Basically I don't want my users to register with a username/password, but instead I'd like them to use SSO, maybe provided by Facebook, Google or our built inhouse elitmus.com. Once the oauth flow is complete, I'd like to use my own JWT tokens though. I'd like to use the user info I get from SSO to build the token.

So far I have something like this:

I am using the TokenGenerator method to generate the jwt token

tokenString, expire, err := auth.TokenGenerator(&User)

And in our case we want to store the token in the cookie and we don't have an method to do that. As that method is tightly coupled with the LoginHandler method.

To make the method to set the cookie for us I have written the method in our application code.

type GinJWTMiddleware struct {
	*jwt.GinJWTMiddleware
}

func NewAuthMiddleware() (*jwt.GinJWTMiddleware, error) {
  ...
}

func (mw *GinJWTMiddleware) SetCookie(c *gin.Context, tokenString string) {
	if mw.SendCookie {
		expireCookie := mw.TimeFunc().Add(mw.CookieMaxAge)
		maxage := int(expireCookie.Unix() - mw.TimeFunc().Unix())

		if mw.CookieSameSite != 0 {
			c.SetSameSite(mw.CookieSameSite)
		}

		c.SetCookie(
			mw.CookieName,
			tokenString,
			maxage,
			"/",
			mw.CookieDomain,
			mw.SecureCookie,
			mw.CookieHTTPOnly,
		)
	}
}

Solution Implementation:

  1. SetCookie Method: This method is added to the *GinJWTMiddleware and is responsible for setting the cookie in the response. It takes the context, token as parameters.
  2. Refactored LoginHandler and RefreshToken: Both methods now call SetCookie to set the cookie, eliminating the duplicated logic and centralizing cookie handling in one place.

Fixes: #336

@maniSHarma7575 maniSHarma7575 changed the title Integrated an option to Set Cookie Code refactoring and Provided an Option to Set Cookie using *GinJWTMiddleware Jul 14, 2024
@maniSHarma7575
Copy link
Contributor Author

@appleboy Could you please review this one?

@appleboy appleboy changed the title Code refactoring and Provided an Option to Set Cookie using *GinJWTMiddleware refactor: Provided an Option to Set Cookie using *GinJWTMiddleware Jul 14, 2024
@appleboy appleboy merged commit d36b890 into appleboy:master Jul 14, 2024
3 of 9 checks passed
@maniSHarma7575 maniSHarma7575 deleted the integrated-method-to-set-cookie branch July 14, 2024 15:54
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

Successfully merging this pull request may close these issues.

DRY Code and Provide an Option to Set Cookie using *GinJWTMiddleware
2 participants