[Bug]: \OC::$server->getSession()->set('key', 'value') not saved #34935

Activity

1. the-djmaze

   added

   0. Needs triagePending check for reproducibility or if it fits our roadmapPending check for reproducibility or if it fits our roadmap

   bug

   on Nov 3, 2022
2. szaimen commented on Nov 3, 2022

   

   szaimen

   on Nov 3, 2022

   Contributor

   More actions

   Which NC version?
3. szaimen

   added

   needs info

   on Nov 3, 2022
4. the-djmaze commented on Nov 3, 2022

   

   the-djmaze

   on Nov 3, 2022

   Author

   More actions

   Which NC version?

   24.0.6
5. szaimen

   added

   24-feedback

   and removed

   needs info

   on Nov 3, 2022
6. CarlSchwan commented on Nov 3, 2022

   

   CarlSchwan

   on Nov 3, 2022

   Member

   More actions

   You are probably using lib/private/Session/Memory.php instead of lib/private/Session/Internal.php, unfortunately I have no idea how the Internal session is used and it seems only the Memory session are actually used

   @juliushaertl do you have an idea why we are using the in memory session handling?
7. juliusknorr commented on Nov 3, 2022

   

   juliusknorr

   on Nov 3, 2022

   Member

   More actions

   The only scenario I would have in mind is if your call to getSession is actually taking place before the initSession call of Nextcloud.

   server/lib/base.php

   Line 418 in 11bedf1

   public static function initSession() {

   Maybe you could add some logging to that to see if that is the case. Otherwise maybe you can provide the link to where the setSession in your apps code doesn't take effect, then I could have a quick look.
8. the-djmaze commented on Nov 3, 2022

   

   the-djmaze

   on Nov 3, 2022

   Last edited by the-djmaze

   Author

   More actions

   The code is executed after the App Controller is called.

   Go to: /nextcloud/index.php/apps/snappymail/
   appinfo/routes.php has `page#index`
   Nextcloud calls `OCA\SnappyMail\Controller\PageController::index()`
   That calls SnappyMailHelper::startApp()
   startApp() tries to do a check due complications with the Impersonate app
   

   Here is the code block (with ocSession disabled):
   https://github.com/the-djmaze/snappymail/blob/39a827aa2e4fb11ea7cbea488b768ac2bee1012b/integrations/nextcloud/snappymail/lib/Util/SnappyMailHelper.php#L104-L127
9. juliusknorr commented on Nov 3, 2022

   

   juliusknorr

   on Nov 3, 2022

   Member

   More actions

   You may need to add the @UseSession annotation to your controller method then, as otherwise ISession::set is called after the session has been closed for writing already.

   https://docs.nextcloud.com/server/latest/developer_manual/basics/controllers.html#reading-and-writing-session-variables
10. the-djmaze commented on Nov 3, 2022

    

    the-djmaze

    on Nov 3, 2022

    Author

    More actions

    Well that is a problem.
    As i also have class Provider implements IProvider but the documentation of search has no @UseSession
    https://docs.nextcloud.com/server/latest/developer_manual/digging_deeper/search.html
11. juliusknorr commented on Nov 3, 2022

    

    juliusknorr

    on Nov 3, 2022

    Member

    More actions

    But why would search need to write the session?
12. the-djmaze commented on Nov 3, 2022

    

    the-djmaze

    on Nov 3, 2022

    Last edited by the-djmaze

    Author

    More actions

    Because of the horrible Impersonate app.

    My SnappyMail app has an automatic login system.
    When user logs in Nextcloud it stores password encrypted in the session.

    When doing a unified search or open the app, it tries to login.
    When login succeeds:

    • the unified search shows found emails
    • the app opens without requesting login

    Now the impersonate app is used and you impersonate someone:

    • SnappyMail logged in, so unified search shows mail of original Nextcloud login (not the impersonate)
    • SnappyMail logged in so app shows mail of original Nextcloud login (not the impersonate)

    To prevent this, i want to:

    • When SnappyMail logged in: store the Nextcloud UID in session
    • Nextcloud UID not the same as in session, then someone uses impersonate
    • Force a logout in this case
    • Try to login with current (impersonate) Nextcloud UID

    If the impersonate app properly triggers the logout event OR Nextcloud then all these checks were not needed.
13. the-djmaze commented on Nov 3, 2022

    

    the-djmaze

    on Nov 3, 2022

    Last edited by the-djmaze

    Author

    More actions

    I know that you want to close the session immediately.
    Else there will be a lock on the data and simultaneous requests have to wait for the unlock.

    However, with simultaneous requests there are issues:

    1. Request 1 loads session data and closes
    2. Request 2 loads session data
    3. Request 2 modifies session data and saves
    4. Request 1 still running but with old session data (not the changed by request 2)

    As this issue is always there, a different approach to sessions could be written.

    1. Request 1 loads session data and closes
    2. Request 2 loads session data
    3. Request 2 modifies session data and saves
    4. Request 1 modifies session data

    • Load current session data
    • Set the modified session data
    • Write merged result session data
14. juliusknorr commented on Nov 3, 2022

    

    juliusknorr

    on Nov 3, 2022

    Last edited by juliusknorr

    Member

    More actions

    Ok, so for Nextcloud 25 there would be a way to reopen the session with #32162

    Now I don't have much insight into the impersonate app but with the described behavior I would imagine this also has all kind of issues with external storages where the login data is stored in the session and probably other session-related cases.

    Maybe @blizzz has any further insight as the main contributor to the impersonate app?
15. the-djmaze commented on Nov 3, 2022

    

    the-djmaze

    on Nov 3, 2022

    Last edited by the-djmaze

    Author

    More actions

    Yes that would be better.

    As for Nextcloud my approach for sessions might be easy to implement to improve #32162

    class Internal extends Session {
    	/**
    	 * @param string $name
    	 * @throws \Exception
    	 */
    	public function __construct(string $name) {
    		set_error_handler([$this, 'trapError']);
    		$this->invoke('session_name', [$name]);
    		try {
    			$this->startSession();
    			$this->invoke('session_abort'); // ADDED, close immediately
    		} catch (\Exception $e) {
    			setcookie($this->invoke('session_name'), '', -1, \OC::$WEBROOT ?: '/');
    		}
    		restore_error_handler();
    		if (!isset($_SESSION)) {
    			throw new \Exception('Failed to start session');
    		}
    	}
    
    	public function set(string $key, $value) {
    		// ADDED
    		$closed = $this->sessionClosed;
    		$closed && $this->startSession(); // reopen() ?!?
    //		$closed && $this->invoke('session_reset'); // possible? no idea
    
    
    //		$this->validateSession();
    		$_SESSION[$key] = $value;
    
    		// ADDED
    		$closed && $this->close();
    	}

    This approach has more IOPS when setting multiple.
    So the ->close() could also be done by process/app itself.

    This way the session:

    • has shortest lock ever
    • session_abort closes session without write
    • set() reopens session with lock
    • lock stays active until close()

    This would also solve my problem if the Impersonate (or other app) provides similar issues.
    Then i can write to Session any time i want and (auto)->close() to unlock asap
16. the-djmaze commented on Nov 3, 2022

    

    the-djmaze

    on Nov 3, 2022

    Author

    More actions

    Oh never mind my response. I looked at the first code but the real solution was later on in the thread.
    You all did what i mention using the $this->reopen() 👍

    I will use that if Impersonate can't solve this.

    👍React with 👍1Reacted by Julius Knorr
17. the-djmaze

    closed this as completedon Nov 3, 2022