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

🐛 The middleware has session operation, and there is no setting for the first visit #1184

Closed
da4z opened this issue Feb 22, 2021 · 10 comments

Comments

@da4z
Copy link

da4z commented Feb 22, 2021

Fiber version
v2.5.0
Issue description
The session middleware has session operation, and there is no setting for the first visit
Code snippet

package main

import (
	"github.com/gofiber/fiber/v2"
	"github.com/gofiber/fiber/v2/middleware/session"
)

func main() {
	store := session.New()

	app := fiber.New()

	app.Use(func(c *fiber.Ctx) error {
		sess, err := store.Get(c)
		if err != nil {
			return err
		}
		defer sess.Save()

		sess.Set("id", "1")

		return c.Next()
	})

	app.Get("/", func(c *fiber.Ctx) error {
		sess, err := store.Get(c)
		if err != nil {
			return err
		}
		defer sess.Save()

		//EXEC SET

		sess.Set("name", "john")

		return c.JSON(fiber.Map{
			"id":   sess.Get("id"),
			"name": sess.Get("name"),
		})
	})

	app.Get("/id", func(c *fiber.Ctx) error {
		sess, err := store.Get(c)
		if err != nil {
			return err
		}
		defer sess.Save()

		//NO SET

		return c.JSON(fiber.Map{
			"id": sess.Get("id"),
		})
	})

	app.Listen("127.0.0.1:3008")
}

x

@ReneWerner87
Copy link
Member

the reason should be that you execute the "save" function at the end, i.e. after the "next" and therefore after the next
handler.

image

probably everything works as you expect it, if you take away the defer for the "save" call

i will test it again, but the assumption looks like this for now

@da4z
Copy link
Author

da4z commented Feb 22, 2021

@ReneWerner87
If the middleware "Save" is removed, it will not be saved if it encounters a route without session

package main

import (
	"github.com/gofiber/fiber/v2"
	"github.com/gofiber/fiber/v2/middleware/session"
)

func main() {
	store := session.New()

	app := fiber.New()

	app.Use(func(c *fiber.Ctx) error {
		sess, err := store.Get(c)
		if err != nil {
			return err
		}
		//defer sess.Save()

		sess.Set("id", "1")

		return c.Next()
	})

	app.Get("/", func(c *fiber.Ctx) error {
		return c.Redirect("/id")
	})

	app.Get("/id", func(c *fiber.Ctx) error {
		sess, err := store.Get(c)
		if err != nil {
			return err
		}
		defer sess.Save()

		return c.JSON(fiber.Map{
			"id": sess.Get("id"),
		})
	})

	app.Listen("127.0.0.1:3008")
}

@ReneWerner87
Copy link
Member

ReneWerner87 commented Feb 22, 2021

package main

import (
	"github.com/gofiber/fiber/v2"
	"github.com/gofiber/fiber/v2/middleware/session"
)

func main() {
	store := session.New()

	app := fiber.New()

	app.Use(func(c *fiber.Ctx) error {
		sess, err := store.Get(c)
		if err != nil {
			return err
		}
	        
		sess.Set("id", "1")
                sess.Save()

		return c.Next()
	})

	app.Get("/", func(c *fiber.Ctx) error {
		sess, err := store.Get(c)
		if err != nil {
			return err
		}
	       

		//EXEC SET

		sess.Set("name", "john")
                sess.Save()

		return c.JSON(fiber.Map{
			"id":   sess.Get("id"),
			"name": sess.Get("name"),
		})
	})

	app.Get("/id", func(c *fiber.Ctx) error {
		sess, err := store.Get(c)
		if err != nil {
			return err
		}
	        sess.Save()

		//NO SET

		return c.JSON(fiber.Map{
			"id": sess.Get("id"),
		})
	})

	app.Listen("127.0.0.1:3008")
}

maybe i misspoke, i meant you should not use "defer" and make the save yourself at the end

defer makes sure that it is executed at the end, so also after the ctx.Next, but in the ctx.Next the next middleware handler is executed, because your middlewares build on each other in the use of the session, you should save before the next middleware is reached

but as i said, i will have a closer look in a couple of hours

@ReneWerner87
Copy link
Member

package main

import (
	"github.com/gofiber/fiber/v2"
	"github.com/gofiber/fiber/v2/middleware/session"
)

func main() {
	store := session.New()

	app := fiber.New()

	app.Use(func(c *fiber.Ctx) error {
		sess, err := store.Get(c)
		if err != nil {
			return err
		}

		sess.Set("id", "1")
		sess.Save()

		return c.Next()
	})

	app.Get("/", func(c *fiber.Ctx) error {
		sess, err := store.Get(c)
		if err != nil {
			return err
		}


		//EXEC SET

		sess.Set("name", "john")

		err = c.JSON(fiber.Map{
			"id":   sess.Get("id"),
			"name": sess.Get("name"),
		})
		if err != nil {
			return err
		}
		return sess.Save()
	})

	app.Get("/id", func(c *fiber.Ctx) error {
		sess, err := store.Get(c)
		if err != nil {
			return err
		}

		err = c.JSON(fiber.Map{
			"id": sess.Get("id"),
		})
		if err != nil {
			return err
		}
		return sess.Save()
	})

	app.Listen("127.0.0.1:3008")
}

this will work

hmm, unfortunately it's not that self-explanatory, the save method leaves the memory free and writes the content to the storage at the same time

in the first variant there was the problem that the "defer" keyword makes the save be used after the middleware executed in the next, but in the next handler it tries to fetch a new session based on the context, which of course contains the last data from the storage and not the one from the previous handler

Unfortunately you can not take the variant that came before the comment, because if you use "save" the memory which is assigned to the context is released and the "get" then gets nothing more, also do not know yet how to make this good and more usable for all

pull requests or suggestions are welcome
@kiyonlin @hi019 any cool ideas ?

@ReneWerner87
Copy link
Member

@chinaapus could you give feedback

@da4z
Copy link
Author

da4z commented Feb 25, 2021

@ReneWerner87
It’s still an old problem, the first time I can’t store them at the same time

x

@ReneWerner87
Copy link
Member

ReneWerner87 commented Feb 25, 2021

OK, I have looked again and now I know what the reason is.

in the first middleware, if a session id does not yet exist in the request cookies, one is generated and packed into the response cookies when saving.
https://github.com/gofiber/fiber/blob/master/middleware/session/store.go#L41

with the second call via get using the context, the request cookies are checked again to see if a session exists, in this case none exists because the information is only in the area of the response, here we have to rework and not only search the request for the sessionId of the cookie, but also the response cookies.

with the second middleware, unfortunately, a completely different sessionId is created and this then overwrites the current one in the response header and marks the session as fresh.
https://github.com/gofiber/fiber/blob/master/middleware/session/store.go#L57

https://github.com/gofiber/fiber/blob/master/middleware/session/store.go#L45
https://github.com/gofiber/fiber/blob/master/middleware/session/config.go#L53


However, we should make sure that we only set the fresh state on the basis of the request header,
because in the second middleware it should still be a new session,
but we have to fetch the data if the sessionId is in the cookies of the response or request.
https://github.com/gofiber/fiber/blob/master/middleware/session/store.go#L57

if i have time this weekend i will do a fix there if anyone else wants to do it, pull requests are of course welcome

@da4z
Copy link
Author

da4z commented Feb 26, 2021

@ReneWerner87
Thanks for finding out the cause and looking forward to repairing. Since I am a golang beginner, I cannot submit a PR yet

@tianjipeng
Copy link
Contributor

I will submit a pr this weekend

@ReneWerner87
Copy link
Member

fix will come with the next fiber version (thx to @tianjipeng )

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

No branches or pull requests

3 participants