diff --git a/core/includes/install.core.inc b/core/includes/install.core.inc index 5579dbf..9774735 100644 --- a/core/includes/install.core.inc +++ b/core/includes/install.core.inc @@ -141,6 +141,9 @@ function install_drupal($class_loader, $settings = array()) { // installations can send output to the browser or redirect the user to the // next page. if ($state['interactive']) { + if ($session = \Drupal::request()->getSession()) { + $session->save(); + } if ($state['parameters_changed']) { // Redirect to the correct page if the URL parameters have changed. install_goto(install_redirect_url($state)); @@ -596,8 +599,9 @@ function install_run_task($task, &$install_state) { $url = Url::fromUri('base:install.php', ['query' => $install_state['parameters'], 'script' => '']); $response = batch_process($url, clone $url); if ($response instanceof Response) { - // Save $_SESSION data from batch. - \Drupal::service('session')->save(); + if ($session = \Drupal::request()->getSession()) { + $session->save(); + } // Send the response. $response->send(); exit; @@ -1549,12 +1553,11 @@ function install_load_profile(&$install_state) { /** * Performs a full bootstrap of Drupal during installation. - * - * @param $install_state - * An array of information about the current installation state. */ function install_bootstrap_full() { - \Drupal::service('session')->start(); + $session = \Drupal::service('session'); + \Drupal::request()->setSession($session); + $session->start(); } /** diff --git a/core/lib/Drupal/Core/Authentication/Provider/Cookie.php b/core/lib/Drupal/Core/Authentication/Provider/Cookie.php deleted file mode 100644 index 7db5d9b..0000000 --- a/core/lib/Drupal/Core/Authentication/Provider/Cookie.php +++ /dev/null @@ -1,56 +0,0 @@ -sessionConfiguration = $session_configuration; - } - - /** - * {@inheritdoc} - */ - public function applies(Request $request) { - return $request->hasSession() && $this->sessionConfiguration->hasSession($request); - } - - /** - * {@inheritdoc} - */ - public function authenticate(Request $request) { - if ($request->getSession()->start()) { - // @todo Remove global in https://www.drupal.org/node/2228393 - global $_session_user; - return $_session_user; - } - - return NULL; - } - -} diff --git a/core/lib/Drupal/Core/Session/SessionHandler.php b/core/lib/Drupal/Core/Session/SessionHandler.php index 92f6741..d8b3789 100644 --- a/core/lib/Drupal/Core/Session/SessionHandler.php +++ b/core/lib/Drupal/Core/Session/SessionHandler.php @@ -10,7 +10,6 @@ use Drupal\Component\Utility\Crypt; use Drupal\Core\Database\Connection; use Drupal\Core\DependencyInjection\DependencySerializationTrait; -use Drupal\Core\Site\Settings; use Drupal\Core\Utility\Error; use Symfony\Component\HttpFoundation\RequestStack; use Symfony\Component\HttpFoundation\Session\Storage\Proxy\AbstractProxy; @@ -37,13 +36,6 @@ class SessionHandler extends AbstractProxy implements \SessionHandlerInterface { protected $connection; /** - * An associative array of obsolete sessions with session id as key, and db-key as value. - * - * @var array - */ - protected $obsoleteSessionIds = array(); - - /** * Constructs a new SessionHandler instance. * * @param \Symfony\Component\HttpFoundation\RequestStack $request_stack @@ -67,59 +59,28 @@ public function open($save_path, $name) { * {@inheritdoc} */ public function read($sid) { - // @todo Remove global in https://www.drupal.org/node/2228393 - global $_session_user; - - // Handle the case of first time visitors and clients that don't store - // cookies (eg. web crawlers). - $cookies = $this->requestStack->getCurrentRequest()->cookies; - if (empty($sid) || !$cookies->has($this->getName())) { - $_session_user = new UserSession(); + if (empty($sid)) { return ''; } - - $values = $this->connection->query("SELECT u.*, s.* FROM {users_field_data} u INNER JOIN {sessions} s ON u.uid = s.uid WHERE u.default_langcode = 1 AND s.sid = :sid", array( - ':sid' => Crypt::hashBase64($sid), - ))->fetchAssoc(); - - // We found the client's session record and they are an authenticated, - // active user. - if ($values && $values['uid'] > 0 && $values['status'] == 1) { - // Add roles element to $user. - $rids = $this->connection->query("SELECT ur.roles_target_id as rid FROM {user__roles} ur WHERE ur.entity_id = :uid", array( - ':uid' => $values['uid'], - ))->fetchCol(); - $values['roles'] = array_merge(array(AccountInterface::AUTHENTICATED_ROLE), $rids); - $_session_user = new UserSession($values); - } - elseif ($values) { - // The user is anonymous or blocked. Only preserve two fields from the - // {sessions} table. - $_session_user = new UserSession(array( - 'session' => $values['session'], - 'access' => $values['access'], - )); - } else { - // The session has expired. - $_session_user = new UserSession(); + // Read the session data from the database. + $query = $this->connection + ->queryRange('SELECT session FROM {sessions} WHERE sid = :sid', 0, 1, ['sid' => Crypt::hashBase64($sid)]); + return (string) $query->fetchField(); } - - return $_session_user->session; } /** * {@inheritdoc} */ public function write($sid, $value) { - $user = \Drupal::currentUser(); - // The exception handler is not active at this point, so we need to do it // manually. try { + $request = $this->requestStack->getCurrentRequest(); $fields = array( - 'uid' => $user->id(), - 'hostname' => $this->requestStack->getCurrentRequest()->getClientIP(), + 'uid' => $request->getSession()->get('uid', 0), + 'hostname' => $request->getClientIP(), 'session' => $value, 'timestamp' => REQUEST_TIME, ); @@ -127,13 +88,6 @@ public function write($sid, $value) { ->keys(array('sid' => Crypt::hashBase64($sid))) ->fields($fields) ->execute(); - - // Likewise, do not update access time more than once per 180 seconds. - if ($user->isAuthenticated() && REQUEST_TIME - $user->getLastAccessedTime() > Settings::get('session_write_interval', 180)) { - /** @var \Drupal\user\UserStorageInterface $storage */ - $storage = \Drupal::entityManager()->getStorage('user'); - $storage->updateLastAccessTimestamp($user, REQUEST_TIME); - } return TRUE; } catch (\Exception $exception) { @@ -159,21 +113,11 @@ public function close() { * {@inheritdoc} */ public function destroy($sid) { - - // Delete session data. $this->connection->delete('sessions') ->condition('sid', Crypt::hashBase64($sid)) ->execute(); - // Reset $_SESSION and current user to prevent a new session from being - // started in \Drupal\Core\Session\SessionManager::save(). - $_SESSION = array(); - \Drupal::currentUser()->setAccount(new AnonymousUserSession()); - - // Unset the session cookies. - $this->deleteCookie($this->getName()); - return TRUE; } @@ -192,19 +136,4 @@ public function gc($lifetime) { return TRUE; } - /** - * Deletes a session cookie. - * - * @param string $name - * Name of session cookie to delete. - */ - protected function deleteCookie($name) { - $cookies = $this->requestStack->getCurrentRequest()->cookies; - if ($cookies->has($name)) { - $params = session_get_cookie_params(); - setcookie($name, '', REQUEST_TIME - 3600, $params['path'], $params['domain'], $params['secure'], $params['httponly']); - $cookies->remove($name); - } - } - } diff --git a/core/lib/Drupal/Core/Session/SessionManager.php b/core/lib/Drupal/Core/Session/SessionManager.php index 3c71930..1d470e2 100644 --- a/core/lib/Drupal/Core/Session/SessionManager.php +++ b/core/lib/Drupal/Core/Session/SessionManager.php @@ -124,10 +124,6 @@ public function start() { } if (empty($result)) { - // @todo Remove global in https://www.drupal.org/node/2228393 - global $_session_user; - $_session_user = new AnonymousUserSession(); - // Randomly generate a session identifier for this request. This is // necessary because \Drupal\user\SharedTempStoreFactory::get() wants to // know the future session ID of a lazily started session in advance. @@ -184,18 +180,16 @@ protected function startNow() { * {@inheritdoc} */ public function save() { - $user = \Drupal::currentUser(); - if ($this->isCli()) { // We don't have anything to do if we are not allowed to save the session. return; } - if ($user->isAnonymous() && $this->isSessionObsolete()) { + if ($this->isSessionObsolete()) { // There is no session data to store, destroy the session if it was // previously started. if ($this->getSaveHandler()->isActive()) { - session_destroy(); + $this->destroy(); } } else { @@ -215,8 +209,6 @@ public function save() { * {@inheritdoc} */ public function regenerate($destroy = FALSE, $lifetime = NULL) { - $user = \Drupal::currentUser(); - // Nothing to do if we are not allowed to change the session. if ($this->isCli()) { return; @@ -264,6 +256,22 @@ public function delete($uid) { /** * {@inheritdoc} */ + public function destroy() { + session_destroy(); + + // Unset the session cookies. + $session_name = $this->getName(); + $cookies = $this->requestStack->getCurrentRequest()->cookies; + if ($cookies->has($session_name)) { + $params = session_get_cookie_params(); + setcookie($session_name, '', REQUEST_TIME - 3600, $params['path'], $params['domain'], $params['secure'], $params['httponly']); + $cookies->remove($session_name); + } + } + + /** + * {@inheritdoc} + */ public function setWriteSafeHandler(WriteSafeSessionHandlerInterface $handler) { $this->writeSafeHandler = $handler; } diff --git a/core/lib/Drupal/Core/Session/SessionManagerInterface.php b/core/lib/Drupal/Core/Session/SessionManagerInterface.php index d194002..c755687 100644 --- a/core/lib/Drupal/Core/Session/SessionManagerInterface.php +++ b/core/lib/Drupal/Core/Session/SessionManagerInterface.php @@ -23,6 +23,11 @@ public function delete($uid); /** + * Destroys the current session and removes session cookies. + */ + public function destroy(); + + /** * Sets the write safe session handler. * * @todo: This should be removed once all database queries are removed from diff --git a/core/modules/system/src/Tests/Session/MultipleAuthenticationSessionTest.php b/core/modules/system/src/Tests/Session/MultipleAuthenticationSessionTest.php new file mode 100644 index 0000000..e51eeb0 --- /dev/null +++ b/core/modules/system/src/Tests/Session/MultipleAuthenticationSessionTest.php @@ -0,0 +1,101 @@ +user = $this->drupalCreateUser(array('administer site configuration')); + } + + /** + * Check that a basic authentication session does not leak. + * + * Regression test for a bug that caused a session initiated by basic + * authentication to persist over subsequent unauthorized requests. + * + * @see https://www.drupal.org/node/2468873 + */ + public function testSessionFromBasicAuthenticationDoesNotLeak() { + // This route is authorized through basic_auth only, not cookie. + $protected_url = Url::fromRoute('session_test.get_session'); + + // This route is not protected. + $unprotected_url = Url::fromRoute('session_test.get_session_no_auth'); + + // Test that the route is not accessible as an anonymous user. + $this->drupalGet($protected_url); + $this->assertResponse(401, 'An anonymous user cannot access a route protected with basic authentication.'); + + // We should be able to access the route with basic authentication. + $this->basicAuthGet($protected_url); + $this->assertResponse(200, 'A route protected with basic authentication can be accessed by an authenticated user.'); + + // Check that the correct user is logged in. + $this->assertEqual($this->user->id(), json_decode($this->getRawContent())->user, 'The correct user is authenticated on a route with basic authentication.'); + + // If we now try to access a page without basic authentication then we + // should no longer be logged in. + $this->drupalGet($unprotected_url); + $this->assertResponse(200, 'An unprotected route can be accessed without basic authentication.'); + $this->assertFalse(json_decode($this->getRawContent())->user, 'The user is no longer authenticated after visiting a page without basic authentication.'); + + // If we access the protected page again without basic authentication we + // should get 401 Unauthorized. + $this->drupalGet($protected_url); + $this->assertResponse(401, 'A subsequent request to the same route without basic authentication is not authorized.'); + } + + /** + * Retrieves a Drupal path or an absolute path using basic authentication. + * + * @param \Drupal\Core\Url|string $path + * Drupal path or URL to load into the internal browser. + * @param array $options + * Options to be forwarded to the url generator. + * + * @return string + * The retrieved HTML string, also available as $this->getRawContent(). + */ + protected function basicAuthGet($path, array $options = array()) { + // Set up Curl to use basic authentication with the test user's credentials. + $headers = [ + 'Accept: */*', + 'Authorization: Basic ' . base64_encode($this->user->getUsername() . ':' . $this->user->pass_raw), + ]; + + return $this->drupalGet($path, $options, $headers); + } + +} diff --git a/core/modules/system/tests/modules/session_test/session_test.routing.yml b/core/modules/system/tests/modules/session_test/session_test.routing.yml index fce0fc9..ea59385 100644 --- a/core/modules/system/tests/modules/session_test/session_test.routing.yml +++ b/core/modules/system/tests/modules/session_test/session_test.routing.yml @@ -89,3 +89,21 @@ session_test.trace_handler: _controller: '\Drupal\session_test\Controller\SessionTestController::traceHandler' requirements: _access: 'TRUE' + +session_test.get_session: + path: '/session-test/get-session' + defaults: + _title: 'Get session information using basic authentication' + _controller: '\Drupal\session_test\Controller\SessionTestController::getSession' + options: + _auth: ['basic_auth'] + requirements: + _permission: 'administer site configuration' + +session_test.get_session_no_auth: + path: '/session-test/get-session-no-auth' + defaults: + _title: 'Get session information' + _controller: '\Drupal\session_test\Controller\SessionTestController::getSession' + requirements: + _access: 'TRUE' diff --git a/core/modules/system/tests/modules/session_test/src/Controller/SessionTestController.php b/core/modules/system/tests/modules/session_test/src/Controller/SessionTestController.php index 4437743..c87ef63 100644 --- a/core/modules/system/tests/modules/session_test/src/Controller/SessionTestController.php +++ b/core/modules/system/tests/modules/session_test/src/Controller/SessionTestController.php @@ -162,4 +162,15 @@ public function traceHandler() { return new JsonResponse($trace); } + /** + * @param \Symfony\Component\HttpFoundation\Request $request + * The request object. + * + * @return \Symfony\Component\HttpFoundation\JsonResponse + * A response object containing the session values and the user ID. + */ + public function getSession(Request $request) { + return new JsonResponse(['session' => $request->getSession()->all(), 'user' => $this->currentUser()->id()]); + } + } diff --git a/core/modules/user/src/Authentication/Provider/Cookie.php b/core/modules/user/src/Authentication/Provider/Cookie.php new file mode 100644 index 0000000..59bfd55 --- /dev/null +++ b/core/modules/user/src/Authentication/Provider/Cookie.php @@ -0,0 +1,109 @@ +sessionConfiguration = $session_configuration; + $this->connection = $connection; + } + + /** + * {@inheritdoc} + */ + public function applies(Request $request) { + return $request->hasSession() && $this->sessionConfiguration->hasSession($request); + } + + /** + * {@inheritdoc} + */ + public function authenticate(Request $request) { + $session = $request->getSession(); + if ($session->start()) { + return $this->getUserFromSession($session); + } + return NULL; + } + + /** + * Returns the user entity for the session. + * + * @param \Symfony\Component\HttpFoundation\Session\SessionInterface $session + * The session. + * + * @return \Drupal\Core\Session\AccountInterface + * The current user object. + */ + protected function getUserFromSession(SessionInterface $session) { + $uid = $session->get('uid'); + if (empty($uid)) { + // Do not disturb database for anonymous users. + return new AnonymousUserSession(); + } + + // @todo Load User entity in handler https://www.drupal.org/node/2345611 + $values = $this->connection + ->query('SELECT * FROM {users_field_data} u WHERE u.uid = :uid AND u.default_langcode = 1', [':uid' => $uid]) + ->fetchAssoc(); + + if ($values && $values['uid'] > 0 && $values['status'] == 1) { + // UserSession::getLastAccessedTime() returns session save timestamp, + // while User::getLastAccessedTime() returns the user 'access' timestamp. + // This ensures they are synchronized. + $values['timestamp'] = $values['access']; + + // We found the client's session record and they are an authenticated, + // active user. + $rids = $this->connection + ->query('SELECT roles_target_id FROM {user__roles} WHERE entity_id = :uid', [':uid' => $values['uid']]) + ->fetchCol(); + // Add user's roles. + $values['roles'] = array_merge([AccountInterface::AUTHENTICATED_ROLE], $rids); + return new UserSession($values); + } + // The session has expired or user is blocked or anonymous. + return new AnonymousUserSession(); + } + +} diff --git a/core/modules/user/src/EventSubscriber/UserRequestSubscriber.php b/core/modules/user/src/EventSubscriber/UserRequestSubscriber.php new file mode 100644 index 0000000..2dd73b0 --- /dev/null +++ b/core/modules/user/src/EventSubscriber/UserRequestSubscriber.php @@ -0,0 +1,75 @@ +account = $account; + $this->entityManager = $entity_manager; + } + + /** + * Updates the current user's last access time. + * + * @param \Symfony\Component\HttpKernel\Event\PostResponseEvent $event + * The event to process. + */ + public function onKernelTerminate(PostResponseEvent $event) { + if ($this->account->isAuthenticated() && REQUEST_TIME - $this->account->getLastAccessedTime() > Settings::get('session_write_interval', 180)) { + // Do that no more than once per 180 seconds. + /** @var \Drupal\user\UserStorageInterface $storage */ + $storage = $this->entityManager->getStorage('user'); + $storage->updateLastAccessTimestamp($this->account, REQUEST_TIME); + } + } + + /** + * {@inheritdoc} + */ + public static function getSubscribedEvents() { + // Should go before other subscribers start to write their caches. And + // specifically before \Drupal\Core\EventSubscriber\KernelDestructionSubscriber + // to prevent instantiation of destructed services. + $events[KernelEvents::TERMINATE][] = ['onKernelTerminate', 300]; + return $events; + } + +} diff --git a/core/modules/user/user.module b/core/modules/user/user.module index 5bcea7c..3ca4698 100644 --- a/core/modules/user/user.module +++ b/core/modules/user/user.module @@ -528,7 +528,7 @@ function user_login_finalize(UserInterface $account) { // fails or incorrectly does a redirect which would leave the old session // in place. \Drupal::service('session')->migrate(); - + \Drupal::service('session')->set('uid', $account->id()); \Drupal::moduleHandler()->invokeAll('user_login', array($account)); } @@ -1386,7 +1386,8 @@ function user_logout() { // Session::invalidate(). Regrettably this method is currently broken and may // lead to the creation of spurious session records in the database. // @see https://github.com/symfony/symfony/issues/12375 - session_destroy(); + \Drupal::service('session_manager')->destroy(); + $user->setAccount(new AnonymousUserSession()); } /** diff --git a/core/modules/user/user.services.yml b/core/modules/user/user.services.yml index 0cf12ef..0a55b6e 100644 --- a/core/modules/user/user.services.yml +++ b/core/modules/user/user.services.yml @@ -16,8 +16,8 @@ services: tags: - { name: access_check, applies_to: _user_is_logged_in } authentication.cookie: - class: Drupal\Core\Authentication\Provider\Cookie - arguments: ['@session_configuration'] + class: Drupal\user\Authentication\Provider\Cookie + arguments: ['@session_configuration', '@database'] tags: - { name: authentication_provider, priority: 0 } user.data: @@ -35,6 +35,11 @@ services: arguments: ['@current_user', '@url_generator'] tags: - { name: event_subscriber } + user_last_access_subscriber: + class: Drupal\user\EventSubscriber\UserRequestSubscriber + arguments: ['@current_user', '@entity.manager'] + tags: + - { name: event_subscriber } theme.negotiator.admin_theme: class: Drupal\user\Theme\AdminNegotiator arguments: ['@current_user', '@config.factory', '@entity.manager', '@router.admin_context']