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

"Remember me" function #211

Closed
deste360 opened this issue Apr 25, 2020 · 10 comments
Closed

"Remember me" function #211

deste360 opened this issue Apr 25, 2020 · 10 comments

Comments

@deste360
Copy link

First of all, excuse me for every mistake I'll can do: I'm not a very capable developer ;-)
I am testing CodeIgniter 4 and myth:auth library; I am not able to let "remember me" option to work. I have done some debug and "remember" cookie is not setted up by rememberUser function inside Authentication\AuthenticationBase.php.

Now, after several test I am able to let it work if I add a $response->send(); after $response->setCookie, between lines 251 and 252. Is it possible that "remember me" have this little bug that missing to send HTTP Response? Or it should it in other way automatically? In this case, why don't work for me?

@michalsn
Copy link
Contributor

michalsn commented May 2, 2020

Good catch. You're right.

You have to add $response->send() (or just chain the method ->send()) because otherwise cookie won't be actually set by Response class. In the AuthController we use redirect() helper which returns different class instance: RedirectResponse and not Response, so class RedirectResponse has no idea about cookies that were prepared to set earlier.

It would be great if you could send a PR with a fix.

@deste360
Copy link
Author

deste360 commented May 2, 2020

Thank you @michalsn !
I have just created a pull request on develop branch.

@lonnieezell
Copy link
Owner

Specifying $response->send() isn't necessary at that point. The authentication service is a shared instance, so we don't send the cookie immediately. The controller will send it when it sends the rest of it's response to the client.

I just setup a brand new CI & Myth:Auth install to verify, created the user, then logged in with remember me checked. When I check my cookies in the browser the remember cookie is there as it should be. So it sounds like something else might be going on here.

Do you use exit() anywhere within the controllers that would stop the normal flow of execution within the controllers? Also - double-check your cookie settings to make sure they wouldn't be ignored (like HTTPS-only when you're not on a secure connection).

@michalsn
Copy link
Contributor

michalsn commented May 5, 2020

So... "remember me" cookie is set correctly in your case? Watching at the code I would say it's a bit surprising.

I know that authentication uses shared response service but in the controller, we use redirect() helper that creates a different class instance: RedirectResponse and immediately return it, so I guess there is no chance that shared response service will fire send() later at some point.

I started testing it in different environments because it seems strange to me and these are the results:

  • Apache on Ubuntu - cookie not set
  • Nginx on Ubuntu - cookie not set
  • Build-in PHP server on Mac - cookie not set
  • Apache on Windows - cookie not set
  • Build-in PHP server on Windows - cookie not set

Idk... maybe I'm missing something here? I was using CI v4.0.3 and Myth:Auth dev-develop.

@deste360
Copy link
Author

deste360 commented May 5, 2020

I'm also trying to test different cases scenario.

I have re-installed from the start latest stable CodeIgniter framework (4.0.3) by composer and Myth/Auth (dev-develop branch, but also tryed 1.0-beta.2), on Windows Apache and PHP 7.4.5. Basic configuration of CodeIgniter .env (app.baseURL, database, email.. No secure https forcing) and basic Myth/Auth config (set allowRemembering to true and set my /app/Config/Filter with global login filter). The issue still persist.

But if @lonnieezell has tested it with success, I suppose it can be an environment or configuration related issue...
I'm happy that also you @michalsn have encountered this problem..

@lonnieezell
Copy link
Owner

Nope - you guys are correct. I must have been seeing a remember cookie from a previous app running on localhost. I just deleted the cookie and tried again and it's not there.

I think a good solution here is not for us to address it in Myth:Auth, since you could run into headers already sent errors, I would think. Instead, I think it might make sense to copy the headers from the shared response instance over into the redirectResponse instance. This can't be the only situation where people would need, this, I wouldn't think.

Can anyone see an issue with doing this?

@deste360
Copy link
Author

deste360 commented May 5, 2020

Forgive me Lonnie: I think I understand a little what the problem is, but unfortunately I do not have the necessary knowledge to say my opinion about the solution you propose, not being able to understand any advantages and disadvantages.

I think you'll be able to share your point of view with Michal... I'm available if there's any way I can help with evidence or anything.
Thank you guys.

@michalsn
Copy link
Contributor

michalsn commented May 5, 2020

Initially, I was thinking about making a copy of the cookies only, but I guess headers also could be handy.

I wonder if assigning these headers previously set by response service can brake something but I can't think of anything for now.

Also, the question is should we make a copy of everything upon RedirectResponse class initialization or rather create additional methods like withCookies() and withHeaders()? I think I would feel better with these additional methods since we wouldn't be changing the way this class works for people.

@lonnieezell
Copy link
Owner

You are correct. Additional methods like that would be best. I like it.

@lonnieezell
Copy link
Owner

I've updated CI with the new methods discussed here. I won't have time tonight to update this, so if someone beats me to it, I'm fine with that :)

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

3 participants