-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
🚀 [Feature]: SessionOnly when cookie.Expires is 0 #2152
🚀 [Feature]: SessionOnly when cookie.Expires is 0 #2152
Conversation
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
@pjebs Thank you for reference I also find this definition:
I updated my changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't it rather be adjusted within the session middleware, that as soon as you send 0, the sessionOnly property is used when setting the cookie, instead of adjusting the general feature to set the cookie ?
b25110d
to
0337e3b
Compare
@yvestumushimire can you check my last comment |
Hello @ReneWerner87 can you please guide where " as soon as you send 0" happens I was thinking |
currently we are changing the way cookies are handled throughout fiber so that it is no longer possible to set the expire of a cookie to 0 by doing this we are limiting the usage for all consumers and this may lead to unexpected behavior would rather change the logic in setSession fiber/middleware/session/session.go Line 188 in c187c6a
so that when the expire is set to 0 there instead of maxAge and expire the cookie is marked as session only fiber/middleware/session/session.go Lines 198 to 199 in c187c6a
Line 129 in c187c6a
and maybe this place fiber/middleware/session/session.go Lines 143 to 145 in c187c6a
|
So if I understand you correctly instead of doing this: if int(s.exp.Seconds()) != 0 {
fcookie.SetMaxAge(int(s.exp.Seconds()))
} else if s.exp != 0 {
fcookie.SetExpire(time.Now().Add(s.exp))
} I should set |
yes but in the session middleware files |
Technically, setting Expire to 0 is a deliberate setting to immediately delete the session. |
Yes i know, but maybe someone is doing this instead of the delete and then we produce a breaking change For me the way to solve the bug is in the files of the middleware |
I also saw |
i think the feature is not completely thought out and also not 100% perfectly possible how long should we keep the data in the storage if the cookie is set to the session only setting ? |
The expiration for the session data is a different concept. There should be a separate setting for that since the cookie expiration and session data expiration are different. The only requirement should be cookie expiration duration should be <= session data expiration. |
then we would need another setting and should not mix it with the expire flag and the api creator then decides how long it keeps the data in the storage with sessionOnly? |
@yvestumushimire any progress on the subject ? last statement from me was that we need another setting not to destroy the other because they are actually different settings and we can't control 2 different things with expire expire is used to tell the storage how long it should be kept in them |
Hello @ReneWerner87 I actually get confused and I really need to understand these concepts more. cause the |
"Session only" is set if the expire is set to 0, but then the data is not stored in the server. In my opinion you can't use the expire config setting for the session only feature, because both are different things. Currently expire time controls how long the data is kept in the backend memory/storage and at the same time how long the session is valid or exists in the browser "Session only" means, as you said, that the sessionid referencing the backend data is only present until the browser is closed. So we should make expire controllable for storage and session with more than one configuration property Hope now it is more understandable, otherwise just report again |
0337e3b
to
97aac75
Compare
@ReneWerner87 Does these latest changes make sense? |
What's the status of this @efectn @ReneWerner87. PR lgtm. |
…re/session-only # Conflicts: # middleware/session/session.go
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
Description
When Cookie Expires is not set default to zero (time.Time{}) is the same as
SessionOnly: true
Fixes #2145
Type of change
Please delete options that are not relevant.
Checklist:
Commit formatting:
Use emojis on commit messages so it provides an easy way of identifying the purpose or intention of a commit. Check out the emoji cheatsheet here: https://gitmoji.carloscuesta.me/