diff --git a/core/includes/install.core.inc b/core/includes/install.core.inc index 9b947ca..5579dbf 100644 --- a/core/includes/install.core.inc +++ b/core/includes/install.core.inc @@ -141,10 +141,6 @@ 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 a session has been initiated in this request, make sure to save it. - 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)); @@ -600,9 +596,8 @@ 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) { - if ($session = \Drupal::request()->getSession()) { - $session->save(); - } + // Save $_SESSION data from batch. + \Drupal::service('session')->save(); // Send the response. $response->send(); exit; @@ -1554,13 +1549,12 @@ 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() { - // Store the session on the request object and start it. - /** @var \Symfony\Component\HttpFoundation\Session\SessionInterface $session */ - $session = \Drupal::service('session'); - \Drupal::request()->setSession($session); - $session->start(); + \Drupal::service('session')->start(); } /** diff --git a/core/lib/Drupal/Core/Authentication/Provider/Cookie.php b/core/lib/Drupal/Core/Authentication/Provider/Cookie.php new file mode 100644 index 0000000..7db5d9b --- /dev/null +++ b/core/lib/Drupal/Core/Authentication/Provider/Cookie.php @@ -0,0 +1,56 @@ +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 ec1e430..92f6741 100644 --- a/core/lib/Drupal/Core/Session/SessionHandler.php +++ b/core/lib/Drupal/Core/Session/SessionHandler.php @@ -10,6 +10,7 @@ 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; @@ -36,6 +37,13 @@ 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 @@ -59,27 +67,59 @@ public function open($save_path, $name) { * {@inheritdoc} */ public function read($sid) { - $data = ''; - if (!empty($sid)) { - // Read the session data from the database. - $query = $this->connection - ->queryRange('SELECT session FROM {sessions} WHERE sid = :sid', 0, 1, ['sid' => Crypt::hashBase64($sid)]); - $data = (string) $query->fetchField(); + // @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(); + 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); } - return $data; + 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(); + } + + 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' => $request->getSession()->get('uid', 0), - 'hostname' => $request->getClientIP(), + 'uid' => $user->id(), + 'hostname' => $this->requestStack->getCurrentRequest()->getClientIP(), 'session' => $value, 'timestamp' => REQUEST_TIME, ); @@ -87,6 +127,13 @@ 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) { @@ -112,11 +159,21 @@ 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; } @@ -135,4 +192,19 @@ 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 1d470e2..3c71930 100644 --- a/core/lib/Drupal/Core/Session/SessionManager.php +++ b/core/lib/Drupal/Core/Session/SessionManager.php @@ -124,6 +124,10 @@ 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. @@ -180,16 +184,18 @@ 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 ($this->isSessionObsolete()) { + if ($user->isAnonymous() && $this->isSessionObsolete()) { // There is no session data to store, destroy the session if it was // previously started. if ($this->getSaveHandler()->isActive()) { - $this->destroy(); + session_destroy(); } } else { @@ -209,6 +215,8 @@ 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; @@ -256,22 +264,6 @@ 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 c755687..d194002 100644 --- a/core/lib/Drupal/Core/Session/SessionManagerInterface.php +++ b/core/lib/Drupal/Core/Session/SessionManagerInterface.php @@ -23,11 +23,6 @@ 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/basic_auth/src/Tests/BasicAuthTestTrait.php b/core/modules/basic_auth/src/Tests/BasicAuthTestTrait.php new file mode 100644 index 0000000..fe0f5ad --- /dev/null +++ b/core/modules/basic_auth/src/Tests/BasicAuthTestTrait.php @@ -0,0 +1,37 @@ +getRawContent(). + */ + protected function basicAuthGet($path, array $options = array(), $username, $password) { + // Set up Curl to use basic authentication with the test user's credentials. + $headers = ['Authorization: Basic ' . base64_encode("$username:$password")]; + + return $this->drupalGet($path, $options, $headers); + } + +} diff --git a/core/modules/system/src/Tests/Session/SessionAuthenticationTest.php b/core/modules/system/src/Tests/Session/SessionAuthenticationTest.php new file mode 100644 index 0000000..1a75cb2 --- /dev/null +++ b/core/modules/system/src/Tests/Session/SessionAuthenticationTest.php @@ -0,0 +1,81 @@ +user = $this->drupalCreateUser(['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. + */ + public function testSessionFromBasicAuthenticationDoesNotLeak() { + // This route is authorized through basic_auth only, not cookie. + $protected_url = Url::fromRoute('session_test.get_session_basic_auth'); + + // 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->user->getUsername(), $this->user->pass_raw); + $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.'); + } + +} 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..0cedf4f 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_basic_auth: + 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..41fe7ed 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,17 @@ public function traceHandler() { return new JsonResponse($trace); } + /** + * Returns the values stored in the active session and the user ID. + * + * @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 deleted file mode 100644 index bac9e15..0000000 --- a/core/modules/user/src/Authentication/Provider/Cookie.php +++ /dev/null @@ -1,103 +0,0 @@ -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) { - return $this->getUserFromSession($request->getSession()); - } - - /** - * Returns the UserSession object for the given session. - * - * @param \Symfony\Component\HttpFoundation\Session\SessionInterface $session - * The session. - * - * @return \Drupal\Core\Session\AccountInterface|NULL - * The UserSession object for the current user, or NULL if this is an - * anonymous session. - */ - protected function getUserFromSession(SessionInterface $session) { - if ($uid = $session->get('uid')) { - // @todo Load the User entity in SessionHandler so we don't need queries. - // @see 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(); - - // Check if the user data was found and the user is active. - if (!empty($values) && $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']; - - // Add the user's roles. - $rids = $this->connection - ->query('SELECT roles_target_id FROM {user__roles} WHERE entity_id = :uid', [':uid' => $values['uid']]) - ->fetchCol(); - $values['roles'] = array_merge([AccountInterface::AUTHENTICATED_ROLE], $rids); - - return new UserSession($values); - } - } - - // This is an anonymous session. - return NULL; - } - -} diff --git a/core/modules/user/src/EventSubscriber/UserRequestSubscriber.php b/core/modules/user/src/EventSubscriber/UserRequestSubscriber.php deleted file mode 100644 index 6f35a2b..0000000 --- a/core/modules/user/src/EventSubscriber/UserRequestSubscriber.php +++ /dev/null @@ -1,75 +0,0 @@ -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. Notably - // 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 3ca4698..5bcea7c 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,8 +1386,7 @@ 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 - \Drupal::service('session_manager')->destroy(); - $user->setAccount(new AnonymousUserSession()); + session_destroy(); } /** diff --git a/core/modules/user/user.services.yml b/core/modules/user/user.services.yml index 0a55b6e..0cf12ef 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\user\Authentication\Provider\Cookie - arguments: ['@session_configuration', '@database'] + class: Drupal\Core\Authentication\Provider\Cookie + arguments: ['@session_configuration'] tags: - { name: authentication_provider, priority: 0 } user.data: @@ -35,11 +35,6 @@ 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']