Skip to content

Commit

Permalink
feat: add token expiry to sessions table and increase default expiry …
Browse files Browse the repository at this point in the history
…to 1 month
  • Loading branch information
samerton committed Jun 23, 2024
1 parent 3cc81a3 commit b75b7bf
Show file tree
Hide file tree
Showing 13 changed files with 208 additions and 47 deletions.
27 changes: 19 additions & 8 deletions core/classes/Core/Cookie.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,27 @@ public static function get(string $name)
/**
* Create a new cookie.
*
* @param string $name Name of cookie to create.
* @param string $value Value to store in cookie.
* @param int $expiry When does the cookie expire?
* @param ?bool $secure Create as secure cookie?
* @param ?bool $httpOnly Create as httpOnly cookie?
* @param string $name Name of cookie to create.
* @param string $value Value to store in cookie.
* @param ?int $expiry When does the cookie expire? Null for session
* @param ?bool $secure Create as secure cookie?
* @param ?bool $httpOnly Create as httpOnly cookie?
* @param ?bool $addExpiry Whether to add expiry onto current timestamp or not.
* @return bool Whether cookie was set or not
*/
public static function put(string $name, string $value, int $expiry, ?bool $secure = false, ?bool $httpOnly = false): bool
{
return setcookie($name, $value, time() + $expiry, '/', null, $secure, $httpOnly);
public static function put(
string $name,
string $value,
?int $expiry,
?bool $secure = false,
?bool $httpOnly = false,
?bool $addExpiry = true
): bool {
if ($expiry && $addExpiry) {
$expiry = time() + $expiry;
}

return setcookie($name, $value, $expiry, '/', null, $secure, $httpOnly);
}

/**
Expand Down
41 changes: 38 additions & 3 deletions core/classes/Core/User.php
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,26 @@ private function find(string $value, string $field = 'id'): bool
if ($field !== 'hash') {
$data = $this->_db->get('users', [$field, $value]);
} else {
$data = $this->_db->query('SELECT nl2_users.* FROM nl2_users LEFT JOIN nl2_users_session ON nl2_users.id = user_id WHERE hash = ? AND nl2_users_session.active = 1', [$value]);
$data = $this->_db->query(
<<<'SQL'
SELECT
nl2_users.*
FROM nl2_users
LEFT JOIN nl2_users_session
ON nl2_users.id = nl2_users_session.user_id
WHERE
nl2_users_session.hash = ?
AND nl2_users_session.active = 1
AND (
nl2_users_session.expires_at IS NULL
OR nl2_users_session.expires_at > ?
)
SQL,
[
$value,
time(),
]
);
}

if ($data->count()) {
Expand Down Expand Up @@ -288,6 +307,7 @@ private function _commonLogin(?string $username, ?string $password, bool $rememb
// Valid credentials
// TODO: job to remove old sessions?
$hash = SecureRandom::alphanumeric();
$expiresAt = $remember ? time() + Config::get('remember.cookie_expiry') : null;

$this->_db->insert('users_session', [
'user_id' => $this->data()->id,
Expand All @@ -297,14 +317,14 @@ private function _commonLogin(?string $username, ?string $password, bool $rememb
'login_method' => $is_admin ? 'admin' : $method,
'user_agent' => $_SERVER['HTTP_USER_AGENT'],
'ip' => HttpUtils::getRemoteAddress(),
'expires_at' => $expiresAt,
]);

Session::put($sessionName, $hash);

if ($remember) {
$expiry = $is_admin ? 3600 : Config::get('remember.cookie_expiry');
$cookieName = $is_admin ? ($this->_cookieName . '_adm') : $this->_cookieName;
Cookie::put($cookieName, $hash, $expiry, HttpUtils::getProtocol() === 'https', true);
Cookie::put($cookieName, $hash, $expiresAt, HttpUtils::getProtocol() === 'https', true, false);
}

return true;
Expand Down Expand Up @@ -524,6 +544,21 @@ public function getActiveSessions(): array
)->results();
}

/**
* Log the user out from a selected session.
*
* @param string $sessionId Selected session ID.
*
* @return void
*/
public function logoutSessionById(string $sessionId): void
{
DB::getInstance()->query('UPDATE nl2_users_session SET `active` = 0 WHERE user_id = ? AND id = ?', [
$this->data()->id,
$sessionId,
]);
}

/**
* Log the user out from all other sessions.
*/
Expand Down
1 change: 1 addition & 0 deletions core/classes/Database/DatabaseInitialiser.php
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,7 @@ private function initialiseSettings(): void
private function initialiseTasks(): void
{
GenerateSitemap::schedule(new Language('core', 'en_UK'));
PurgeExpiredSessions::schedule(new Language('core', 'en_UK'));
}

private function initialiseTemplates(): void
Expand Down
4 changes: 2 additions & 2 deletions core/classes/Misc/HttpUtils.php
Original file line number Diff line number Diff line change
Expand Up @@ -257,9 +257,9 @@ public static function getHeader(string $header_name): ?string
return null;
}

public static function getIpCountry(string $ip): string
public static function getIpCountry(?string $ip): string
{
if (in_array($ip, ['localhost', '127.0.0.1', '::1'])) {
if (!$ip || in_array($ip, ['localhost', '127.0.0.1', '::1'])) {
return 'Unknown';
}

Expand Down
12 changes: 12 additions & 0 deletions core/includes/updates/212.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?php

return new class() extends UpgradeScript {
public function run(): void
{
$this->runMigrations();

PurgeExpiredSessions::schedule(new Language('core', 'en_UK'));

$this->setVersion('2.2.0');
}
};
2 changes: 1 addition & 1 deletion core/installation/steps/database_configuration.php
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
],
'remember' => [
'cookie_name' => 'nl2',
'cookie_expiry' => 604800,
'cookie_expiry' => 2629800,
],
'session' => [
'session_name' => '2user',
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
<?php

declare(strict_types=1);

use Phinx\Migration\AbstractMigration;

final class AddSessionExpiryColumnToUsersSessionTable extends AbstractMigration
{
public function change(): void
{
$this->table('nl2_users_session')
->addColumn('expires_at', 'integer', ['length' => 11, 'null' => true, 'default' => null])
->update();
}
}
16 changes: 12 additions & 4 deletions custom/templates/DefaultRevamp/user/sessions.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,18 @@
{if $CAN_LOGOUT_ALL}
<div class="ui small modal" id="modal-logout-all">
<div class="header">
{$LINK} {$provider_name|ucfirst}
{$LOGOUT}
</div>
<div class="content">
{$OAUTH_MESSAGES[$provider_name]['link_confirm']}
{$LOGOUT_ALL_CONFIRM}
</div>
<div class="actions">
<a class="ui negative button">{$NO}</a>
<a class="ui green button" href="{$provider_data.url}">{$CONFIRM}</a>
<form action="" method="post" style="display: inline;">
<input type="hidden" name="token" value="{$TOKEN}" />
<input type="hidden" name="action" value="logout_other_sessions" />
<input type="submit" class="ui green button" value="{$YES}" />
</form>
</div>
</div>
{/if}
Expand All @@ -102,7 +106,11 @@
</div>
<div class="actions">
<a class="ui negative button">{$NO}</a>
<a class="ui green button" href="{$BASE_URL}user/logout/{$session.id}">{$CONFIRM}</a>
<form action="" method="post" style="display: inline;">
<input type="hidden" name="token" value="{$TOKEN}" />
<input type="hidden" name="session_hash" value="{$session.id}" />
<input type="submit" class="ui green button" value="{$YES}" />
</form>
</div>
</div>
{/foreach}
Expand Down
2 changes: 1 addition & 1 deletion dev/scripts/cli_install.php
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ function getEnvVar(string $name, string $fallback = null, array $valid_values =
],
'remember' => [
'cookie_name' => 'nl2',
'cookie_expiry' => 604800,
'cookie_expiry' => 2629800,
],
'session' => [
'session_name' => '2user',
Expand Down
54 changes: 54 additions & 0 deletions modules/Core/classes/Tasks/PurgeExpiredSessions.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
<?php
class PurgeExpiredSessions extends Task
{
public function run(): string {
$language = $this->_container->get(Language::class);

$cutoff = strtotime('1 month ago');
$count = DB::getInstance()->query(
<<<SQL
SELECT
COUNT(*) c
FROM
`nl2_users_session`
WHERE
`last_seen` < ? OR
(
`last_seen` IS NULL AND
`active` = 0 AND
`expires_at` IS NULL
)
SQL,
[
$cutoff,
]
)->first()->c;

if ($count) {
DB::getInstance()->query(
'DELETE FROM `nl2_users_session` WHERE `last_seen` < ? OR (`last_seen` < ? AND `active` = 0 AND `expires_at` IS NULL)',
[
$cutoff,
]
);
}

$this->setOutput(['result' => $language->get('admin', 'sessions_purged', ['count' => $count])]);
$this->reschedule($language);

return Task::STATUS_COMPLETED;
}

private function reschedule(Language $language) {
Queue::schedule((new PurgeExpiredSessions())->fromNew(
Module::getIdFromName('Core'),
$language->get('admin', 'purge_sessions'),
[],
Date::next()->getTimestamp()
));
}

public static function schedule(Language $language) {
(new self())->reschedule($language);
}
}
6 changes: 5 additions & 1 deletion modules/Core/language/en_UK.json
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,7 @@
"admin/profile_widgets": "Profile Widgets",
"admin/public": "Public",
"admin/purge_errors": "Purge Errors",
"admin/purge_sessions": "Purge Sessions",
"admin/query_error_deleted_successfully": "Query error deleted successfully.",
"admin/query_errors": "Query Errors",
"admin/query_errors_purged_successfully": "Query errors purged successfully.",
Expand Down Expand Up @@ -641,6 +642,7 @@
"admin/server_query_port": "Server Query Port",
"admin/server_query_port_help": "This is the query.port option in your server's server.properties, provided the enable-query option in the same file is set to true.",
"admin/server_updated": "Server updated successfully.",
"admin/sessions_purged": "{{count}} sessions purged successfully.",
"admin/settings": "Settings",
"admin/settings_updated_successfully": "General settings updated successfully.",
"admin/show_ip_on_status_page": "Show IP on status page?",
Expand Down Expand Up @@ -870,7 +872,9 @@
"general/log_out": "Log Out",
"general/log_out_click": "Click here to log out",
"general/log_out_complete": "Logout successful. Click {{linkStart}}here{{linkEnd}} to continue.",
"general/logout_session_successfully": "Session successfully logged out.",
"general/log_out_all_sessions_confirm": "Are you sure you wish to end all other sessions?",
"general/log_out_selected_session_confirm": "Are you sure you wish to end the selected session?",
"general/logout_session_successful": "Session successfully logged out.",
"general/more": "More",
"general/new": "New",
"general/next": "Next",
Expand Down
31 changes: 19 additions & 12 deletions modules/Core/pages/panel/users_sessions.php
Original file line number Diff line number Diff line change
@@ -1,12 +1,21 @@
<?php
/*
* Made by Supercrafter100
* https://github.com/NamelessMC/Nameless/
* NamelessMC version 2.0.3
/**
* StaffCP user sessions page.
*
* License: MIT
* @author Supercrafter100
* @version 2.2.0
* @license MIT
*
* Panel user sessions page
* @var Cache $cache
* @var Language $language
* @var Navigation $cc_nav
* @var Navigation $navigation
* @var Navigation $staffcp_nav
* @var Pages $pages
* @var Smarty $smarty
* @var TemplateBase $template
* @var User $user
* @var Widgets $widgets
*/
if (!$user->handlePanelPageLoad('admincp.users.sessions')) {
require_once(ROOT_PATH . '/403.php');
Expand Down Expand Up @@ -37,21 +46,19 @@
DB::getInstance()->update('users_session', $_POST['sid'], [
'active' => false
]);
$success = $language->get('general', 'logout_session_successfully');
} else if ($_POST['action'] === 'logout_other_sessions') {
$success = $language->get('general', 'logout_session_successful');
} elseif ($_POST['action'] === 'logout_other_sessions') {
$view_user->logoutAllOtherSessions();
$success = $language->get('admin', 'sessions_logged_out');
}
} else {
$errors[] = $language->get('general', 'invalid_token');
}
}
$timeago = new TimeAgo(TIMEZONE);
$timeAgo = new TimeAgo(TIMEZONE);
$sessions = $view_user->getActiveSessions();
$user_sessions_list = [];

// TODO: Should we display all sessions, or just active ones? Over time, the list could get very long if we display all sessions.
// Not really any reason to show inactive ones, since they can't action on them.
foreach ($sessions as $session) {
$agent = new \Jenssegers\Agent\Agent();
$agent->setUserAgent($session->user_agent);
Expand All @@ -65,7 +72,7 @@
'device_browser_version' => $agent->version($agent->browser()),
'method' => $session->login_method,
'last_seen_short' => $session->last_seen
? $timeago->inWords($session->last_seen, $language)
? $timeAgo->inWords($session->last_seen, $language)
: $language->get('admin', 'unknown'),
'last_seen_long' => $session->last_seen
? date(DATE_FORMAT, $session->last_seen)
Expand Down
Loading

0 comments on commit b75b7bf

Please sign in to comment.