fuelphp-storage / auth

FuelPHP Framework - Authentication library
4 stars 0 forks source link

Strange behaviour when trying to log in or log out #2

Open sagikazarmark opened 10 years ago

sagikazarmark commented 10 years ago

When I try to login/logout and I redirect the request right after doing any of these actions, nothing happens (meaning login/logout fails). If I stop the request (aka. exit) and refresh the page then it succeeds. Is it a session or an auth issue?

Example:

    public function actionLogout()
    {
        $this->getAuth()->logout();

        return \Response::forge('redirect', 'admin/login');
    }
sagikazarmark commented 10 years ago

The problem is that the store method on Persistence object is not called when using Redirect.

sagikazarmark commented 10 years ago

Well, I cannot fix it by calling directly the destructor, so might be something else.

@WanWizard any idea how to manually persist a login/logout?

WanWizard commented 10 years ago

Auth wasn't finished up to that point yet. I only tested the individual methods from the commandline.

In v1 sessions are written in a shutdown event, which always runs, even when you redirect. The event instance is created by the Application, but Auth is not integrated yet.

On th other hand, a class destructor should always be called. Is it called? And if so, what does it write?

sagikazarmark commented 10 years ago

I know that you didn't finish it, but I hope reporting bugs will help your work.

Not sure what happens here. I am using the File drivers (for storage and persistence as well). Does it use any session at all?

It seems that the File Persistence driver's destructor is NOT called when using Redirect.

WanWizard commented 10 years ago

No, there is no session support at all at the moment. There is only one persistence driver, with is the file driver. Which is very basic, as it stores state based on REMOTE_ADDR, or localhost if no address exists. I wrote the File drivers to be able to test the API without dependencies (to database, session, or other packages).

sagikazarmark commented 10 years ago

That's what I thought. I still can't decide if it is an issue with redirect or the persistence driver.

WanWizard commented 10 years ago

I find it very strange that in case of a redirect, the class destructor isn't called. That should be part of the PHP cleanup...

sagikazarmark commented 10 years ago

I'll try to debug what happens.

sagikazarmark commented 10 years ago

Something stupid is going on here. The problem only happens when I redirect to the same controller. If I do a redirection to any other controller and go back to the "protected" one, than it works.

sagikazarmark commented 10 years ago

GOT IT. Opcode cache. I also have to note that I currently use php built-in webserver for testing. Turning opcode cache off solved the problem.

It should be tested with apache as well, but I think the result will be the same.

I don't know how it could/should be solved. Probably saving data into non-php files?

WanWizard commented 10 years ago

It's the main reason I always have all cache switched off in development and staging environments (the other important one is being able to see the apps true performance).

Even without Opcache you can bump into issues. I spent most of last week trying to debug an issue with a task being fired by a queue worker. It wouldn't run an updated version of the task, because it still had the old class in memory.

sagikazarmark commented 10 years ago

Well, I always have problems with this. If I leave opcache turned on, I can solve these issues during the development, however I usually forget to blame opcache.

Anyway, I agree with your points.

Any idea for solving the issue? While I don't think File driver will be used in production environment, it should still be possible.

WanWizard commented 10 years ago

I don't have a clue how to solve it, if calling a class destructor is dependent on having opcache active or not, I'm inclined to say that it's a PHP bug...

sagikazarmark commented 10 years ago

How about using another format instead of php? Or letting the file driver choose you a file format?

WanWizard commented 10 years ago

Ah, you mean for the file store itself?

Sorry, just now understand what the issue is, and this can be solved quite easily, we did that for v1 too, see https://github.com/fuel/core/commit/8e3f9bf283cfcc6925bca827a4b8a63fef23d687 and https://github.com/fuel/core/commit/389f8227243c66f959e75065817e52a4f73294be

sagikazarmark commented 10 years ago

Yeah, I remember, I was the one who reported the issue.

However that wouldn't be a real solution. Configs are rarely modified by the code itself, so invalidating the cache some times is a viable way to solve that problem, but doing it ON EVERY request is not an option (and the File drivers do exactly the same). That invalidates the existence of the cache itself. I would rather let people use a non-php file as storage in production environment. IMO it could be easily implemented.

WanWizard commented 10 years ago

Maybe we have to come up with a new file extension as an alias for .php, to indicate it stores an array in php format.

Would .phps be cached?

The PHPS file type is primarily associated with 'PHP Source' by The PHP Group. Generally, PHP files will get interpreted by the Web server and PHP executable, and you will never see the code behind the PHP file. If you make the file extension .PHPS, a properly-configured server will output a color-formated version of the source instead of the HTML that would normally be generated. Not all servers are so configured.

WanWizard commented 10 years ago

As to the file type, it's probably best that the file driver is written as an extension of Fuel\Config\Container, which would directly implement the saving and loading in different formats.