Skip to content

refactor: Fix many psalm issues #1003

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

Open
CarlSchwan wants to merge 2 commits into
base: master
Choose a base branch
from
Open

refactor: Fix many psalm issues #1003

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
6e09be1
refactor: Fix many psalm issues
CarlSchwan Dec 8, 2025
f3a29b5
refactor: Port more stuff to IAppConfig
CarlSchwan Dec 8, 2025
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
235 changes: 235 additions & 0 deletions LICENSES/AGPL-3.0-only.txt
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion lib/AppInfo/Application.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
use OCP\AppFramework\Bootstrap\IBootstrap;
use OCP\AppFramework\Bootstrap\IRegistrationContext;
use OCP\AppFramework\Http\Events\BeforeTemplateRenderedEvent;
use OCP\AppFramework\Services\IAppConfig;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\IConfig;
use OCP\IDBConnection;
Expand Down Expand Up @@ -55,7 +56,7 @@ public function register(IRegistrationContext $context): void {
$context->registerEventListener(SabrePluginAddEvent::class, SabrePluginEventListener::class);
$context->registerService(DavPlugin::class, fn (ContainerInterface $c) => new DavPlugin(
$c->get(ISession::class),
$c->get(IConfig::class),
$c->get(IAppConfig::class),
$_SERVER,
$c->get(SAMLSettings::class)
));
Expand Down
4 changes: 2 additions & 2 deletions lib/Command/UserAdd.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ protected function configure(): void {
protected function execute(InputInterface $input, OutputInterface $output): int {
$uid = $input->getArgument('uid');

if ($this->userManager->userExists($uid)) {
$user = $this->userManager->get($uid);
if ($user === null) {
$output->writeln('<error>The account "' . $uid . '" already exists.</error>');
return 1;
}
Expand All @@ -70,7 +71,6 @@ protected function execute(InputInterface $input, OutputInterface $output): int
$this->backend->setDisplayName($uid, $input->getOption('display-name'));
$email = $input->getOption('email');
if (!empty($email)) {
$user = $this->userManager->get($uid);
$user->setSystemEMailAddress($email);
}
} catch (\Exception $e) {
Expand Down
78 changes: 39 additions & 39 deletions lib/Controller/SAMLController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@
use OCA\User_SAML\UserResolver;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\Attribute\NoAdminRequired;
use OCP\AppFramework\Http\Attribute\NoCSRFRequired;
use OCP\AppFramework\Http\Attribute\PublicPage;
use OCP\AppFramework\Http\Attribute\UseSession;
use OCP\AppFramework\Services\IAppConfig;
use OCP\IConfig;
use OCP\IL10N;
use OCP\IRequest;
Expand Down Expand Up @@ -50,6 +55,7 @@
private SAMLSettings $samlSettings,
private UserBackend $userBackend,
private IConfig $config,
private IAppConfig $appConfig,
private IURLGenerator $urlGenerator,
private LoggerInterface $logger,
private IL10N $l,
Expand Down Expand Up @@ -100,13 +106,12 @@
}
$uid = $this->userData->getOriginalUid();
$uid = $this->userData->testEncodedObjectGUID($uid);
if (!$userExists && !$autoProvisioningAllowed) {
if (!$autoProvisioningAllowed) {
throw new NoUserFoundException('Auto provisioning not allowed and user ' . $uid . ' does not exist');
} elseif (!$userExists && $autoProvisioningAllowed) {
$this->userBackend->createUserIfNotExists($uid, $auth);
$this->userBackend->updateAttributes($uid);
return;
}

$this->userBackend->createUserIfNotExists($uid, $auth);
$this->userBackend->updateAttributes($uid);
}

/**
Expand Down Expand Up @@ -138,20 +143,19 @@
}

/**
* @PublicPage
* @UseSession
* @OnlyUnauthenticatedUsers
* @NoCSRFRequired
*
* @throws Exception
*/
#[PublicPage]
#[UseSession]
#[NoCSRFRequired]
public function login(int $idp = 1): Http\RedirectResponse|Http\TemplateResponse {
$originalUrl = (string)$this->request->getParam('originalUrl', '');
if (!$this->trustedDomainHelper->isTrustedUrl($originalUrl)) {
$originalUrl = '';
}

$type = $this->config->getAppValue($this->appName, 'type');
$type = $this->appConfig->getAppValueString('type');
switch ($type) {
case 'saml':
$settings = $this->samlSettings->getOneLoginSettingsArray($idp);
Expand Down Expand Up @@ -274,10 +278,10 @@
}

/**
* @PublicPage
* @NoCSRFRequired
* @throws Error
*/
#[PublicPage]
#[NoCSRFRequired]
public function getMetadata(int $idp = 1): Http\DataDownloadResponse {
$settings = new Settings($this->samlSettings->getOneLoginSettingsArray($idp));
$metadata = $settings->getSPMetadata();
Expand All @@ -293,16 +297,16 @@
}

/**
* @PublicPage
* @NoCSRFRequired
* @UseSession
* @OnlyUnauthenticatedUsers
* @NoSameSiteCookieRequired
*
* @return Http\RedirectResponse
* @throws Error
* @throws ValidationError
*/
#[PublicPage]
#[NoCSRFRequired]
#[UseSession]
public function assertionConsumerService(): Http\RedirectResponse {
// Fetch and decrypt the cookie
$cookie = $this->request->getCookie('saml_data');
Expand Down Expand Up @@ -355,7 +359,7 @@
foreach ($errors as $error) {
$this->logger->error($error, ['app' => $this->appName]);
}
$this->logger->error($auth->getLastErrorReason(), ['app' => $this->appName]);
$this->logger->error($auth->getLastErrorReason() ?? 'No last error reason found', ['app' => $this->appName]);
}

if (!$auth->isAuthenticated()) {
Expand Down Expand Up @@ -424,12 +428,12 @@
}

/**
* @PublicPage
* @NoAdminRequired
* @NoCSRFRequired
* @UseSession
* @throws Error
*/
#[PublicPage]
#[NoAdminRequired]
#[UseSession]
#[NoCSRFRequired]
public function singleLogoutService(): Http\RedirectResponse {
$isFromGS = ($this->config->getSystemValueBool('gs.enabled', false)
&& $this->config->getSystemValueString('gss.mode', '') === 'master');
Expand Down Expand Up @@ -457,6 +461,7 @@
$idp = $decoded['idp'] ?? null;
$pass = true;
} catch (Exception) {
$pass = false;
}
} else {
// standard request : need read CRSF check
Expand All @@ -474,7 +479,7 @@
foreach ($errors as $error) {
$this->logger->error($error, ['app' => $this->appName]);
}
$this->logger->error($auth->getLastErrorReason(), ['app' => $this->appName]);

Check failure on line 482 in lib/Controller/SAMLController.php

View workflow job for this annotation

GitHub Actions / static-psalm-analysis dev-stable30

PossiblyNullArgument 

lib/Controller/SAMLController.php:482:28: PossiblyNullArgument: Argument 1 of Psr\Log\LoggerInterface::error cannot be null, possibly null value provided (see https://psalm.dev/078)

Check failure on line 482 in lib/Controller/SAMLController.php

View workflow job for this annotation

GitHub Actions / static-psalm-analysis dev-master

PossiblyNullArgument 

lib/Controller/SAMLController.php:482:28: PossiblyNullArgument: Argument 1 of Psr\Log\LoggerInterface::error cannot be null, possibly null value provided (see https://psalm.dev/078)

Check failure on line 482 in lib/Controller/SAMLController.php

View workflow job for this annotation

GitHub Actions / static-psalm-analysis dev-stable31

PossiblyNullArgument 

lib/Controller/SAMLController.php:482:28: PossiblyNullArgument: Argument 1 of Psr\Log\LoggerInterface::error cannot be null, possibly null value provided (see https://psalm.dev/078)

Check failure on line 482 in lib/Controller/SAMLController.php

View workflow job for this annotation

GitHub Actions / static-psalm-analysis dev-stable32

PossiblyNullArgument 

lib/Controller/SAMLController.php:482:28: PossiblyNullArgument: Argument 1 of Psr\Log\LoggerInterface::error cannot be null, possibly null value provided (see https://psalm.dev/078)
}
} else {
$this->logger->error('Error while handling SLO request: missing session data, and request is not satisfied by any configuration');
Expand All @@ -494,7 +499,7 @@
$this->userSession->logout();
}
}
if (!empty($targetUrl) && !$auth->getLastErrorReason()) {
if (!empty($targetUrl) && $auth && !$auth->getLastErrorReason()) {
$this->userSession->logout();
}
}
Expand All @@ -506,7 +511,7 @@
}

/**
* @returns [?string, ?Auth]
* @return array{0: ?string, 1: ?Auth}
*/
private function tryProcessSLOResponse(?int $idp): array {
$idps = ($idp !== null) ? [$idp] : array_keys($this->samlSettings->getListOfIdps());
Expand All @@ -532,28 +537,28 @@
}

/**
* @PublicPage
* @NoCSRFRequired
* @OnlyUnauthenticatedUsers
*/
#[PublicPage]
#[NoCSRFRequired]
public function notProvisioned(): Http\TemplateResponse {
return new Http\TemplateResponse($this->appName, 'notProvisioned', [], 'guest');
}

/**
* @PublicPage
* @NoCSRFRequired
* @OnlyUnauthenticatedUsers
*/
#[PublicPage]
#[NoCSRFRequired]
public function notPermitted(): Http\TemplateResponse {
return new Http\TemplateResponse($this->appName, 'notPermitted', [], 'guest');
}

/**
* @PublicPage
* @NoCSRFRequired
* @OnlyUnauthenticatedUsers
*/
#[PublicPage]
#[NoCSRFRequired]
public function genericError(string $message): Http\TemplateResponse {
if (empty($message)) {
$message = $this->l->t('Unknown error, please check the log file for more details.');
Expand All @@ -562,17 +567,17 @@
}

/**
* @PublicPage
* @NoCSRFRequired
* @OnlyUnauthenticatedUsers
*/
#[PublicPage]
#[NoCSRFRequired]
public function selectUserBackEnd(string $redirectUrl = ''): Http\TemplateResponse {
$attributes = ['loginUrls' => []];

if ($this->samlSettings->allowMultipleUserBackEnds()) {
$displayName = $this->l->t('Direct log in');

$customDisplayName = $this->config->getAppValue('user_saml', 'directLoginName', '');
$customDisplayName = $this->appConfig->getAppValueString('directLoginName');
if ($customDisplayName !== '') {
$displayName = $customDisplayName;
}
Expand All @@ -584,10 +589,8 @@
}

$attributes['loginUrls']['ssoLogin'] = $this->getIdps($redirectUrl);

$attributes['useCombobox'] = count($attributes['loginUrls']['ssoLogin']) > 4;


return new Http\TemplateResponse($this->appName, 'selectUserBackEnd', $attributes, 'guest');
}

Expand Down Expand Up @@ -651,17 +654,14 @@
* get Nextcloud login URL
*/
private function getDirectLoginUrl(string $redirectUrl): string {
$directUrl = $this->urlGenerator->linkToRouteAbsolute('core.login.tryLogin', [
return $this->urlGenerator->linkToRouteAbsolute('core.login.tryLogin', [
'direct' => '1',
'redirect_url' => $redirectUrl,
]);
return $directUrl;
}

/**
* @PublicPage
* @NoCSRFRequired
*/
#[PublicPage]
#[NoCSRFRequired]
public function base(): Http\TemplateResponse {
$message = $this->l->t('This page should not be visited directly.');
return new Http\TemplateResponse($this->appName, 'error', ['message' => $message], 'guest');
Expand Down
22 changes: 17 additions & 5 deletions lib/Controller/SettingsController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,24 +10,32 @@
use OCA\User_SAML\SAMLSettings;
use OCA\User_SAML\Settings\Admin;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\DataResponse;
use OCP\AppFramework\Http\Response;
use OCP\IConfig;
use OCP\AppFramework\Services\IAppConfig;
use OCP\IRequest;
use OneLogin\Saml2\Constants;

/**
* @psalm-api
*/
class SettingsController extends Controller {

public function __construct(
$appName,
string $appName,
IRequest $request,
private readonly IConfig $config,
private readonly IAppConfig $appConfig,
private readonly Admin $admin,
private readonly SAMLSettings $samlSettings,
) {
parent::__construct($appName, $request);
}

/**
* @return DataResponse<Http::STATUS_OK, array{providerIds: string}, array{}>
* @throws \OCP\DB\Exception
*/
public function getSamlProviderIds(): DataResponse {
$keys = array_keys($this->samlSettings->getListOfIdps());
return new DataResponse([ 'providerIds' => implode(',', $keys)]);
Expand All @@ -51,6 +59,7 @@ public function getSamlProviderSettings(int $providerId): array {
'passthroughParameters' => ['required' => false],
];
/* Fetch all config values for the given providerId */
$settings = [];

// initialize settings with default value for option box (others are left empty)
$settings['sp']['name-id-format'] = Constants::NAMEID_UNSPECIFIED;
Expand Down Expand Up @@ -81,7 +90,7 @@ public function getSamlProviderSettings(int $providerId): array {

if (isset($details['global']) && $details['global']) {
// Read legacy data from oc_appconfig
$settings[$category][$setting] = $this->config->getAppValue('user_saml', $key, '');
$settings[$category][$setting] = $this->appConfig->getAppValueString($key, '');
} else {
$settings[$category][$setting] = $storedSettings[$key] ?? '';
}
Expand All @@ -90,7 +99,7 @@ public function getSamlProviderSettings(int $providerId): array {
return $settings;
}

public function deleteSamlProviderSettings($providerId): Response {
public function deleteSamlProviderSettings(int $providerId): Response {
$this->samlSettings->delete($providerId);
return new Response();
}
Expand All @@ -102,6 +111,9 @@ public function setProviderSetting(int $providerId, string $configKey, string $c
return new Response();
}

/*
* @return DataResponse<Http::STATUS_OK, array{id: int}, array{}>
*/
public function newSamlProviderSettingsId(): DataResponse {
return new DataResponse(['id' => $this->samlSettings->getNewProviderId()]);
}
Expand Down
10 changes: 5 additions & 5 deletions lib/DavPlugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
namespace OCA\User_SAML;

use OCA\DAV\Connector\Sabre\Auth;
use OCP\IConfig;
use OCP\AppFramework\Services\IAppConfig;
use OCP\ISession;
use Sabre\DAV\Server;
use Sabre\DAV\ServerPlugin;
Expand All @@ -21,21 +21,21 @@ class DavPlugin extends ServerPlugin {

public function __construct(
private readonly ISession $session,
private readonly IConfig $config,
private readonly IAppConfig $config,
private array $auth,
private readonly SAMLSettings $samlSettings,
) {
}

public function initialize(Server $server) {
public function initialize(Server $server): void {
// before auth
$server->on('beforeMethod:*', $this->beforeMethod(...), 9);
$this->server = $server;
}

public function beforeMethod(RequestInterface $request, ResponseInterface $response) {
public function beforeMethod(RequestInterface $request, ResponseInterface $response): void {
if (
$this->config->getAppValue('user_saml', 'type') === 'environment-variable'
$this->config->getAppValueString('type') === 'environment-variable'
&& !$this->session->exists('user_saml.samlUserData')
) {
$uidMapping = $this->samlSettings->get(1)['general-uid_mapping'];
Expand Down
Loading
Loading