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

fix when destroy session can't remove cookie in subdomain #964

Merged
merged 4 commits into from
Apr 22, 2018
Merged

fix when destroy session can't remove cookie in subdomain #964

merged 4 commits into from
Apr 22, 2018

Conversation

chengyumeng
Copy link
Contributor

We'd love to see more contributions

Read how you can contribute to the project.

Please attach an issue link which your PR solves otherwise your work may be rejected.

@chengyumeng
Copy link
Contributor Author

Hi, when I was developing with iris, I discovered that there is a problem that the online environment cannot return to the environment and development environment. That is, there is no way for the online environment to achieve real user exit. The differences between the online environment and the returning environment are: 1. The online environment is two instances on k8s(pods), and the stage environment is only one; 2. The online environment is using domain name, for example: chengyumeng.github.com (note, not the root domain name), and the stage environment uses IP.

According to my research on the iris code, it is found that the session storage of iris only operates from the instance memory, and does not go back to the backup storage (for example, redis, mysql, etc.) each time. As a result, the session between multiple instances is not shared. However, iris official does not think this is a bug, but a feature, although I think that the official should provide a switch to choose whether to read from memory or multi-instance shared redis, but if the official does not think this is a problem that needs to be solved, I will subsequently develop a third-party session module to help users achieve this requirement. (This part is discussed in: https://github.com/kataras/iris/issues/885 talk a lot, I'm not talking about words)

The second problem I found was probably an iris bug. During the deletion of the session, the second-level domain name was not considered (in fact, the second-level domain name was processed during the COOKIE update). I copied this part of the code, used in the delete COOKIE logic.

No new unit tests have been added.

Why now I can‘t watch the issues?

@kataras
Copy link
Owner

kataras commented Apr 11, 2018

OK @chengyumeng , hello again I had some personal issues and I couldn't make my own part in order to accept/decline this wonderful PR, I want to ask you only one think, the code you added on the RemoveCookie is already being used on the sessions, so is better to move that to a function and change the session manager to call that function as well before accept?

As for the #885 , we need to make huge changes in order to accept get/set on each request from the database instead of loading once from db and saving the whole store to db on Set , I will think of it and 99% it will be ready on v11

Copy link
Owner

@kataras kataras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code you use is already on the sessions#updateCookie please make the necessary changes there to export the shared actions, we don't want code duplication without reason. Thank you a lot!

@chengyumeng
Copy link
Contributor Author

@kataras emmm,I have created a new function,please review my code again

@kataras kataras merged commit f2c3a5f into kataras:master Apr 22, 2018
github-actions bot pushed a commit to goproxies/github.com-kataras-iris that referenced this pull request Jul 27, 2020
fix when destroy session can't remove cookie in subdomain

Former-commit-id: 5caf0fa5d92f53ba7b744dc1b3b7d47f688a96db
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.

2 participants