Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Apply code best practices PHP8+ #907

Merged
merged 2 commits into from
Dec 3, 2024
Merged

refactor: Apply code best practices PHP8+ #907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2390d7
refactor: Apply code best practices PHP8+
solracsf Nov 21, 2024
27d09bb
fix: revert removed line
solracsf Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/Command/ConfigCreate.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@
use Symfony\Component\Console\Output\OutputInterface;

class ConfigCreate extends Base {
private SAMLSettings $samlSettings;

public function __construct(SAMLSettings $samlSettings) {
public function __construct(
private SAMLSettings $samlSettings,
) {
parent::__construct();
$this->samlSettings = $samlSettings;
}

protected function configure(): void {
Expand Down
6 changes: 3 additions & 3 deletions lib/Command/ConfigDelete.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@
use Symfony\Component\Console\Output\OutputInterface;

class ConfigDelete extends Base {
private SAMLSettings $samlSettings;

public function __construct(SAMLSettings $samlSettings) {
public function __construct(
private SAMLSettings $samlSettings,
) {
parent::__construct();
$this->samlSettings = $samlSettings;
}

protected function configure(): void {
Expand Down
6 changes: 3 additions & 3 deletions lib/Command/ConfigGet.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@
use Symfony\Component\Console\Output\OutputInterface;

class ConfigGet extends Base {
private SAMLSettings $samlSettings;

public function __construct(SAMLSettings $samlSettings) {
public function __construct(
private SAMLSettings $samlSettings,
) {
parent::__construct();
$this->samlSettings = $samlSettings;
}

protected function configure(): void {
Expand Down
6 changes: 3 additions & 3 deletions lib/Command/ConfigSet.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@
use Symfony\Component\Console\Output\OutputInterface;

class ConfigSet extends Base {
private SAMLSettings $samlSettings;

public function __construct(SAMLSettings $samlSettings) {
public function __construct(
private SAMLSettings $samlSettings,
) {
parent::__construct();
$this->samlSettings = $samlSettings;
}

protected function configure(): void {
Expand Down
11 changes: 6 additions & 5 deletions lib/Command/GetMetadata.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,10 @@
class GetMetadata extends Command {
use TXmlHelper;

private SAMLSettings $SAMLSettings;

public function __construct(
SAMLSettings $SAMLSettings,
private SAMLSettings $samlSettings,
) {
parent::__construct();
$this->SAMLSettings = $SAMLSettings;
}

protected function configure(): void {
Expand All @@ -49,7 +46,11 @@ protected function configure(): void {

protected function execute(InputInterface $input, OutputInterface $output): int {
$idp = (int)$input->getArgument('idp');
$settings = new Settings($this->SAMLSettings->getOneLoginSettingsArray($idp));
$settingsArray = $this->samlSettings->getOneLoginSettingsArray($idp);
if ($settingsArray === null) {
throw new \InvalidArgumentException('Settings cannot be null');
}
$settings = new Settings($settingsArray);
$metadata = $settings->getSPMetadata();
$errors = $this->callWithXmlEntityLoader(function () use ($settings, $metadata) {
return $settings->validateMetadata($metadata);
Expand Down
16 changes: 3 additions & 13 deletions lib/Controller/SettingsController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,24 +17,14 @@

class SettingsController extends Controller {

/** @var IConfig */
private $config;
/** @var Admin */
private $admin;
/** @var SAMLSettings */
private $samlSettings;

public function __construct(
$appName,
IRequest $request,
IConfig $config,
Admin $admin,
SAMLSettings $samlSettings,
private IConfig $config,
private Admin $admin,
private SAMLSettings $samlSettings,
) {
parent::__construct($appName, $request);
$this->config = $config;
$this->admin = $admin;
$this->samlSettings = $samlSettings;
}

public function getSamlProviderIds(): DataResponse {
Expand Down
20 changes: 6 additions & 14 deletions lib/Controller/TimezoneController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,22 +16,14 @@

class TimezoneController extends Controller {

/** @var IConfig */
private $config;
/** @var string */
private $userId;
/** @var ISession */
private $session;

public function __construct($appName,
public function __construct(
$appName,
IRequest $request,
IConfig $config,
$userId,
ISession $session) {
private IConfig $config,
private $userId,
private ISession $session,
) {
parent::__construct($appName, $request);
$this->config = $config;
$this->userId = $userId;
$this->session = $session;
}

/**
Expand Down
17 changes: 6 additions & 11 deletions lib/DavPlugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,22 +15,17 @@
use Sabre\HTTP\ResponseInterface;

class DavPlugin extends ServerPlugin {
private $session;
private $config;
private $auth;
/** @var Server */
private $server;
/** @var SAMLSettings */
private $samlSettings;

public function __construct(ISession $session, IConfig $config, array $auth, SAMLSettings $samlSettings) {
$this->session = $session;
$this->config = $config;
$this->auth = $auth;
$this->samlSettings = $samlSettings;
public function __construct(
private ISession $session,
private IConfig $config,
private array $auth,
private SAMLSettings $samlSettings,
) {
}


public function initialize(Server $server) {
// before auth
$server->on('beforeMethod:*', [$this, 'beforeMethod'], 9);
Expand Down
50 changes: 13 additions & 37 deletions lib/GroupManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,46 +20,22 @@
use OCP\IGroup;
use OCP\IGroupManager;
use OCP\IUser;
use OCP\Server;
use Psr\Log\LoggerInterface;

class GroupManager {
public const LOCAL_GROUPS_CHECK_FOR_MIGRATION = 'localGroupsCheckForMigration';
public const STATE_MIGRATION_PHASE_EXPIRED = 'EXPIRED';

/**
* @var IDBConnection $db
*/
protected $db;

/** @var IGroupManager */
private $groupManager;
/** @var GroupBackend */
private $ownGroupBackend;
/** @var IConfig */
private $config;
/** @var IEventDispatcher */
private $dispatcher;
/** @var IJobList */
private $jobList;
/** @var SAMLSettings */
private $settings;


public function __construct(
IDBConnection $db,
IGroupManager $groupManager,
GroupBackend $ownGroupBackend,
IConfig $config,
IEventDispatcher $dispatcher,
IJobList $jobList,
SAMLSettings $settings,
protected IDBConnection $db,
private IGroupManager $groupManager,
private GroupBackend $ownGroupBackend,
private IConfig $config,
private IEventDispatcher $dispatcher,
private IJobList $jobList,
private SAMLSettings $settings,
) {
$this->db = $db;
$this->groupManager = $groupManager;
$this->ownGroupBackend = $ownGroupBackend;
$this->config = $config;
$this->dispatcher = $dispatcher;
$this->jobList = $jobList;
$this->settings = $settings;
}

/**
Expand All @@ -70,7 +46,7 @@ public function __construct(
private function getGroupsToRemove(array $samlGroupNames, array $assignedGroups): array {
$groupsToRemove = [];
foreach ($assignedGroups as $group) {
\OCP\Log\logger('user_saml')->debug('Checking group {group} for removal', ['group' => $group->getGID()]);
Server::get(LoggerInterface::class)->debug('Checking group {group} for removal', ['app' => 'user_saml', 'group' => $group->getGID()]);
if (in_array($group->getGID(), $samlGroupNames, true)) {
continue;
}
Expand All @@ -92,7 +68,7 @@ private function getGroupsToRemove(array $samlGroupNames, array $assignedGroups)
private function getGroupsToAdd(array $samlGroupNames, array $assignedGroupIds): array {
$groupsToAdd = [];
foreach ($samlGroupNames as $groupName) {
\OCP\Log\logger('user_saml')->debug('Checking group {group} for addition', ['group' => $groupName]);
Server::get(LoggerInterface::class)->debug('Checking group {group} for addition', ['app' => 'user_saml', 'group' => $groupName]);
$group = $this->groupManager->get($groupName);
// if user is not assigned to the group or the provided group has a non SAML backend
if (!in_array($groupName, $assignedGroupIds) || !$this->hasSamlBackend($group)) {
Expand Down Expand Up @@ -298,9 +274,9 @@ protected function mayModifyGroup(?IGroup $group): bool {
&& $this->isGroupInTransitionList($group->getGID());

if ($isInTransition) {
\OCP\Log\logger('user_saml')->debug('Checking group {group} for foreign members', ['group' => $group->getGID()]);
Server::get(LoggerInterface::class)->debug('Checking group {group} for foreign members', ['app' => 'user_saml', 'group' => $group->getGID()]);
$hasOnlySamlUsers = !$this->hasGroupForeignMembers($group);
\OCP\Log\logger('user_saml')->debug('Completed checking group {group} for foreign members', ['group' => $group->getGID()]);
Server::get(LoggerInterface::class)->debug('Completed checking group {group} for foreign members', ['app' => 'user_saml', 'group' => $group->getGID()]);
if (!$hasOnlySamlUsers) {
$this->updateCandidatePool([$group->getGID()]);
}
Expand Down
26 changes: 5 additions & 21 deletions lib/Jobs/MigrateGroups.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,33 +32,17 @@ class MigrateGroups extends QueuedJob {

protected const BATCH_SIZE = 1000;

/** @var IConfig */
private $config;
/** @var IGroupManager */
private $groupManager;
/** @var IDBConnection */
private $dbc;
/** @var GroupBackend */
private $ownGroupBackend;
/** @var LoggerInterface */
private $logger;

public function __construct(
protected GroupMigration $groupMigration,
protected GroupManager $ownGroupManager,
IConfig $config,
IGroupManager $groupManager,
IDBConnection $dbc,
GroupBackend $ownGroupBackend,
LoggerInterface $logger,
private IConfig $config,
private IGroupManager $groupManager,
private IDBConnection $dbc,
private GroupBackend $ownGroupBackend,
private LoggerInterface $logger,
ITimeFactory $timeFactory,
) {
parent::__construct($timeFactory);
$this->config = $config;
$this->groupManager = $groupManager;
$this->dbc = $dbc;
$this->ownGroupBackend = $ownGroupBackend;
$this->logger = $logger;
}

protected function run($argument) {
Expand Down
15 changes: 3 additions & 12 deletions lib/Middleware/OnlyLoggedInMiddleware.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,12 @@
* @package OCA\User_SAML\MiddleWare
*/
class OnlyLoggedInMiddleware extends Middleware {
/** @var IControllerMethodReflector */
private $reflector;
/** @var IUserSession */
private $userSession;
/** @var IURLGenerator */
private $urlGenerator;

public function __construct(
IControllerMethodReflector $reflector,
IUserSession $userSession,
IURLGenerator $urlGenerator,
private IControllerMethodReflector $reflector,
private IUserSession $userSession,
private IURLGenerator $urlGenerator,
) {
$this->reflector = $reflector;
$this->userSession = $userSession;
$this->urlGenerator = $urlGenerator;
}

/**
Expand Down
11 changes: 2 additions & 9 deletions lib/Migration/RememberLocalGroupsForPotentialMigrations.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,10 @@

class RememberLocalGroupsForPotentialMigrations implements IRepairStep {

/** @var IGroupManager */
private $groupManager;
/** @var IConfig */
private $config;

public function __construct(
IGroupManager $groupManager,
IConfig $config,
private IGroupManager $groupManager,
private IConfig $config,
) {
$this->groupManager = $groupManager;
$this->config = $config;
}

public function getName(): string {
Expand Down
22 changes: 5 additions & 17 deletions lib/SAMLSettings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,29 +65,17 @@ class SAMLSettings {

public const DEFAULT_GROUP_PREFIX = 'SAML_';

/** @var IURLGenerator */
private $urlGenerator;
/** @var IConfig */
private $config;
/** @var ISession */
private $session;
/** @var array<int, array<string, string>> */
private $configurations = [];
/** @var int */
private $configurationsLoadedState = self::LOADED_NONE;
/** @var ConfigurationsMapper */
private $mapper;

public function __construct(
IURLGenerator $urlGenerator,
IConfig $config,
ISession $session,
ConfigurationsMapper $mapper,
private IURLGenerator $urlGenerator,
private IConfig $config,
private ISession $session,
private ConfigurationsMapper $mapper,
) {
$this->urlGenerator = $urlGenerator;
$this->config = $config;
$this->session = $session;
$this->mapper = $mapper;
}

/**
Expand Down Expand Up @@ -132,7 +120,7 @@ public function getOneLoginSettingsArray(int $idp): array {

$settings = [
'strict' => true,
'debug' => $this->config->getSystemValue('debug', false),
'debug' => $this->config->getSystemValueBool('debug', false),
'baseurl' => $this->urlGenerator->linkToRouteAbsolute('user_saml.SAML.base'),
'security' => [
'nameIdEncrypted' => ($this->configurations[$idp]['security-nameIdEncrypted'] ?? '0') === '1',
Expand Down
Loading
Loading