search

plp050452 / simplesamlphp

Automatically exported from code.google.com/p/simplesamlphp
Other
0 stars 0 forks source link

Option to disable transient session #492

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago 
Purpose of code changes on this branch:

Add session.allow_transient option allowing to disable transient sessions, i.e. 
throw exception on transient session. When session store is down, IMO it's 
better to generate error, than user ending in redirect loop because auth state 
can't be loaded.

When reviewing my code changes, please focus on:

After the review, I'll merge this branch into:
/trunk

Original issue reported on code.google.com by comel...@gmail.com on 26 Apr 2012 at 2:36

Attachments:

GoogleCodeExporter commented 8 years ago 
I think you have misunderstood the purpose of transient sessions. While they 
are used when the session store fails, they also serve a different purpose. The 
main goal is to avoid creating sessions for API calls. We do not use it 
anywhere in the SSP distribution, but we do use it in an external module.

I would focus on crashing on session initialization failure, rather than 
disabling transient session. That means that you could simply rethrow the 
exception when the flag is set.

(I would also give the option a different name, e.g. session.disable_fallback)

Original comment by olavmrk@gmail.com on 27 Apr 2012 at 9:17

GoogleCodeExporter commented 8 years ago 
Something like this?

Original comment by comel...@gmail.com on 27 Apr 2012 at 11:10

Attachments:

GoogleCodeExporter commented 8 years ago 
I assumed your goal only was to throw an exception instead of using a transient 
session?

In that case, you could get away with something like this in the 
exception-handler in getInstance():

    if (<fallback disabled?>) {
        throw $e;
    }

Or?

You would still need to handle failures from the error reporter and from the 
logging code though.

Original comment by olavmrk@gmail.com on 27 Apr 2012 at 11:15

GoogleCodeExporter commented 8 years ago 
Hm, when I throw exception without initializing session (call 
useTransientSession), I get PHP fatal error: Uncaught exception 
'SimpleSAML_Error_Error'... I want to display error, and be able to submit 
error report, so I presume I need transient session. I left isTransient() 
method because we have some custom code in error reporting, which needs 
non-transient session to work.

Original comment by comel...@gmail.com on 27 Apr 2012 at 11:35

GoogleCodeExporter commented 8 years ago 
Have you tried to call useTransientSession() before rethrowing the exception?
(What does the stack trace look like when you get the fatal error?)

Original comment by olavmrk@gmail.com on 27 Apr 2012 at 11:42

GoogleCodeExporter commented 8 years ago 
Yes, this is in session-load-1.patch.

"NOTICE: PHP message: PHP Fatal error:  Uncaught exception 
'SimpleSAML_Error_Error' with message 'SESSIONLOADERROR' in 
/home/comel/work/git/simplesamlphp/lib/SimpleSAML/Session.php:1025"
"Stack trace:"
"#0 /home/comel/work/git/simplesamlphp/lib/SimpleSAML/Session.php(259): 
SimpleSAML_Session::getSession()"
"#1 /home/comel/work/git/simplesamlphp/lib/SimpleSAML/Error/Error.php(175): 
SimpleSAML_Session::getInstance()"
"#2 /home/comel/work/git/simplesamlphp/lib/SimpleSAML/Error/Error.php(217): 
SimpleSAML_Error_Error->saveError()"
"#3 /home/comel/work/git/simplesamlphp/www/_include.php(35): 
SimpleSAML_Error_Error->show()"
"#4 [internal function]: 
SimpleSAML_exception_handler(Object(SimpleSAML_Error_Error))"
"#5 {main}"
"  thrown in /home/comel/work/git/simplesamlphp/lib/SimpleSAML/Session.php on 
line 1025"

Original comment by comel...@gmail.com on 27 Apr 2012 at 11:47

GoogleCodeExporter commented 8 years ago 
I am unable to reproduce this. I made the following change to Session.php:

diff --git a/lib/SimpleSAML/Session.php b/lib/SimpleSAML/Session.php
index 4933924..3a4da74 100644
--- a/lib/SimpleSAML/Session.php
+++ b/lib/SimpleSAML/Session.php
@@ -257,6 +257,7 @@ public static function getInstance() {
                        }
                        /* For some reason, we were unable to initialize this session. Use a transient session instead. */
                        self::useTransientSession();
+                       throw $e;
                        return self::$instance;
                }

When testing this, I get the following backtrace:

Backtrace:
0 /home/olavmo/simplesamlphp/www/module.php:180 (N/A)
Caused by: Exception: Oops...
Backtrace:
10 /home/olavmo/simplesamlphp/lib/SimpleSAML/SessionHandlerPHP.php:27 
(SimpleSAML_SessionHandlerPHP::__construct)
9 /home/olavmo/simplesamlphp/lib/SimpleSAML/SessionHandler.php:87 
(SimpleSAML_SessionHandler::createSessionHandler)
8 /home/olavmo/simplesamlphp/lib/SimpleSAML/SessionHandler.php:35 
(SimpleSAML_SessionHandler::getSessionHandler)
7 /home/olavmo/simplesamlphp/lib/SimpleSAML/Session.php:979 
(SimpleSAML_Session::getSession)
6 /home/olavmo/simplesamlphp/lib/SimpleSAML/Session.php:250 
(SimpleSAML_Session::getInstance)
5 /home/olavmo/simplesamlphp/lib/SimpleSAML/Logger.php:219 
(SimpleSAML_Logger::getTrackId)
4 /home/olavmo/simplesamlphp/lib/SimpleSAML/Logger.php:191 
(SimpleSAML_Logger::log_internal)
3 /home/olavmo/simplesamlphp/lib/SimpleSAML/Logger.php:75 
(SimpleSAML_Logger::error)
2 /home/olavmo/simplesamlphp/lib/SimpleSAML/Session.php:256 
(SimpleSAML_Session::getInstance)
1 /home/olavmo/simplesamlphp/modules/core/www/frontpage_welcome.php:6 (require)
0 /home/olavmo/simplesamlphp/www/module.php:135 (N/A)

Original comment by olavmrk@gmail.com on 2 May 2012 at 11:49

GoogleCodeExporter commented 8 years ago 
Oops, when reviewing the stack trace, I see that the useTransientSession() call 
should be moved to before the logging statements. Changing that does not really 
change the behaviour, but it improves the stacktrace:

Backtrace:
0 /home/olavmo/simplesamlphp/www/module.php:180 (N/A)
Caused by: Exception: Oops...
Backtrace:
6 /home/olavmo/simplesamlphp/lib/SimpleSAML/SessionHandlerPHP.php:27 
(SimpleSAML_SessionHandlerPHP::__construct)
5 /home/olavmo/simplesamlphp/lib/SimpleSAML/SessionHandler.php:87 
(SimpleSAML_SessionHandler::createSessionHandler)
4 /home/olavmo/simplesamlphp/lib/SimpleSAML/SessionHandler.php:35 
(SimpleSAML_SessionHandler::getSessionHandler)
3 /home/olavmo/simplesamlphp/lib/SimpleSAML/Session.php:979 
(SimpleSAML_Session::getSession)
2 /home/olavmo/simplesamlphp/lib/SimpleSAML/Session.php:250 
(SimpleSAML_Session::getInstance)
1 /home/olavmo/simplesamlphp/modules/core/www/frontpage_welcome.php:6 (require)
0 /home/olavmo/simplesamlphp/www/module.php:135 (N/A)

Original comment by olavmrk@gmail.com on 2 May 2012 at 11:52

GoogleCodeExporter commented 8 years ago 
We had misunderstanding, I don't get fatal error with session-load-1.patch, 
only when I don't call useTransientSession() before throwing exception. Here is 
new patch (without SESSIONLOADERROR)...

Original comment by comel...@gmail.com on 2 May 2012 at 5:45

Attachments:

GoogleCodeExporter commented 8 years ago 
I do not think you need the $transient flag and the isTransient()-function? 
Other than that it looks good.

Original comment by olavmrk@gmail.com on 3 May 2012 at 10:25

GoogleCodeExporter commented 8 years ago 
We have some custom code in error show function which needs non-transient 
session to work, so I would like to have isTransient() function if you agree? I 
could use getTrackID() == 'XXXXXXXXXX' instead, but it's ugly...

Original comment by comel...@gmail.com on 3 May 2012 at 11:29

GoogleCodeExporter commented 8 years ago 
OK, I can accept that. I just wanted to avoid adding unused code to the 
repository.

Original comment by olavmrk@gmail.com on 3 May 2012 at 11:45

GoogleCodeExporter commented 8 years ago 
OK, committed as r3082.

Original comment by comel...@gmail.com on 3 May 2012 at 11:54