lonnieezell / Bonfire

Jumpstart your CodeIgniter web applications with a modular, HMVC-ready, backend.
http://cibonfire.com
1.39k stars 526 forks source link

redirect after login goes to '/' instead of back to previous page #640

Closed sourcejedi closed 11 years ago

sourcejedi commented 11 years ago

Thanks for this report by @i960, taken from a comment on 557de4463e1bca914cb3d4702150385324e77ffd. I still don't see the bug happening on my system, but it's a very interesting report.

"Even with this fix, I found that I am still being redirected to '/' no matter what. I kind of "hacked" my way around it by having my controllers extend Front_Controller instead of Authenticated_Controller and writing my own logic to store the previous page. It was messy and stupid but it sort of worked. But all of the admin pages were experiencing the same issue and I didn't want to hack that to pieces trying to force it to work. So, after spending hours banging my head trying to figure out what was going wrong, it hit me right in the face. In the file /bonfire/application/core_modules/users/libraries/auth.php on line 348 is this:

        $this->logout();

Bingo! That function destroys the session, so the 'previous_page' session var is being wiped out. So not only does it prevent the page from being redirected back to the previous page after login, it also prevents the next line from working, which displays the "You must be logged in to view that page." text on the login page. I'm not sure why that logout function is there considering it won't even run unless you are already logged out. If there is some reason for it that I'm not aware of then I would love to know, but so far everything seems to be working perfectly.

I have no clue how to use git or github to submit patches, so I will let someone else do that if this is a proper fix. I just wanted to point out the problem I was having and what I had to do to fix it."

i960 commented 11 years ago

I'm going to install a fresh copy of bonfire 0.6.1 on an extra domain I have and see if the problem persists. I just want to make certain that there really is a bug since it's not happening for you. I did make some changes to the users module to add in support for bcrypt, but I can't see how that would affect this issue. I will report back with my findings.

sourcejedi commented 11 years ago

Ah, I see it now.

If I try to visit /admin/ when not logged in, I get a login form with "You must log in to view this page" in a red box. I log in, and I get redireted back to /admin/. (Well, /admin/content/). That part works.

If I visit /users/profile - the front-end page, not the admin page - then I get a login form without the message text in, and logging in takes me to '/'.

i960 commented 11 years ago

Yup, that's the exact issue I am seeing, but for me it also happens on admin pages. For instance, if for some reason I hit '/admin/settings/users' without being logged in, I get a login page, then I'm redirected to the home page. I first noticed this because in my application I am sending out emails with links to a new context I created in the admin area. When clicking on the link, the person gets a login page, then ends up on the home page and doesn't know why. So then I began testing the built in contexts as well as front end pages and noticed the same thing.

i960 commented 11 years ago

I just noticed your commit. One other thing I changed was all instances of Template::redirect() to redirect(). I did this because using Template::redirect() causes a very obvious blank page flash before the redirect. Could this be why we are seeing different results? I'm almost done installing a fresh copy of bonfire so I can test with zero changes. I got distracted by something shiny so it took a bit longer, lol.

sourcejedi commented 11 years ago

@i960 Could be!

Check you haven't set "Login Destination" to '/' on the Roles you're using as well. I doubt that's the problem, but it's something an obvious thing to check.

sourcejedi commented 11 years ago

I think 7a48a29791dd2b0fd65552b6016abf5183710bd6 should avoid the flashing you were seeing. So (assuming you're not already running 0.7-dev) you could replace Template::redirect() with the version from here. Then change redirect() back to Template::redirect() in /bonfire/application/core_modules/users/libraries/auth.php on line 350.

i960 commented 11 years ago

Ok, testing complete! On a fresh install of 0.6.1, the admin area redirects properly, so it must have been my changes to Template::redirect() that caused that. Also, f5b273d4914b30c93ae88352920d978bffc16a89 fixes the issue with the profile page redirects, and 7a48a29791dd2b0fd65552b6016abf5183710bd6 fixes the problem with the redirect flash. I even created a new module with a controller that extends Authenticated_Controller, and that works perfectly as well. So I'm all good now. :)

sourcejedi commented 11 years ago

Scratch that. The reason f5b273d fixes the problem I saw wasn't Template::redirect(). Switching back to redirect doesn't change anything. What fixes it is the addition of Template::set_message(). There may be a CI bug where adding a variable to the session undoes the effect of session->sess_destroy()...

sourcejedi commented 11 years ago

Good to hear. Do let us know if anything else strange happens :).

sourcejedi commented 11 years ago

The CI bug I'm thinking of is https://github.com/EllisLab/CodeIgniter/issues/1314. CI 2.1.3 has been released with the fix... and after upgrading to it locally, now the logout() call is a problem. Thanks to @i960, we had advance warning :).

@seandowney. You added the logout() call in c4cbedf033c7ba76d33940ad927a292eadfa0319, but @i960 and I think that's wrong. Can we remove the logout() call you added there? I've checked, and it does solve the problem for me.

If you checkout feature/ci-2.1.3, you should be able to see the problem:

  1. Make sure you're logged out
  2. Try to visit /admin => login form appears
  3. Log in => you are redirected to '/'

Desired behaviour: you should be redirected back to /admin (or ultimately /admin/content/).

i960 commented 11 years ago

Well that explains everything! I neglected to mention that I had upgraded to CI 2.1.3. That's why we were seeing different behavior, and also why the problem didn't seem to exist on the admin side when I did a fresh install of Bonfire. Doh! I think it just slipped my mind, otherwise I would have mentioned it. I seem to have this nasty habit of always wanting the latest and greatest, and sometimes it bites me in the ass. But I guess in this case it helped out the project. :)

sourcejedi commented 11 years ago

A minor detail :). Thanks again.

i960 commented 11 years ago

On side note, I really need to learn how to use github and git in general. I would love to contribute code to projects like this in the proper way. I added bcrypt support to Bonfire in my own project, and I think that would be a very useful addition. It's actually a requirement with the company I work for. Right now I'm using my own home grown archaic method of version control, and it's time I learn to do it the right way.

sourcejedi commented 11 years ago

bcrypt support sounds cool.

The GitHub help page starts with a Bootcamp section, which looks like it might be helpful. Git's not the most user-friendly system :). IMO it's useful to get the concepts of how Git actually works, the sort of thing Git for Computer Scientists introducs. I'm sure the official free Git Book is fine, but I don't know whether or not it's the best if you want to get cracking as soon as possible.

ghost commented 11 years ago

@sourcejedi yes go for it. It's a long time since I looked at that and can't remember why I added it.

Thanks

Sent from my Android phone with K-9 Mail. Please excuse my brevity.