Issues in UserGlobalStoragesController.php - Fixed Issues - Inspection of "commit transaction after select query" - owncloud/core - Measure and Improve Code Quality continuously with Scrutinizer

Completed

Pull Request — master (#26700)

by Philipp

created 2016-11-23 11:35 UTC

lib/Controller/UserGlobalStoragesController.php (4 issues)

Labels

Bug 4

Severity

Minor 4

Upgrade to new PHP Analysis Engine

These results are based on our legacy PHP analysis, consider migrating to our new PHP analysis engine instead. Learn more

<?php
/**
 * @author Joas Schilling <[email protected]>
 * @author Juan Pablo Villafáñez <[email protected]>
 * @author Robin Appelman <[email protected]>
 * @author Robin McCorkell <[email protected]>
 *
 * @copyright Copyright (c) 2016, ownCloud GmbH.
 * @license AGPL-3.0
 *
 * This code is free software: you can redistribute it and/or modify
 * it under the terms of the GNU Affero General Public License, version 3,
 * as published by the Free Software Foundation.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
 * GNU Affero General Public License for more details.
 *
 * You should have received a copy of the GNU Affero General Public License, version 3,
 * along with this program.  If not, see <http://www.gnu.org/licenses/>
 *
 */

namespace OCA\Files_External\Controller;

use OCA\Files_External\Lib\Auth\AuthMechanism;
use OCA\Files_External\Lib\Auth\IUserProvided;
use OCA\Files_External\Lib\InsufficientDataForMeaningfulAnswerException;
use OCP\ILogger;
use \OCP\IRequest;
use \OCP\IL10N;
use \OCP\AppFramework\Http\DataResponse;
use \OCP\AppFramework\Http;
use OCA\Files_External\Service\UserGlobalStoragesService;
use OCA\Files_External\NotFoundException;
use OCA\Files_External\Lib\StorageConfig;
use \OCA\Files_External\Lib\Backend\Backend;
use OCP\IUserSession;

/**
 * User global storages controller
 */
class UserGlobalStoragesController extends StoragesController {
	/**
	 * @var IUserSession
	 */
	private $userSession;

	/**
	 * Creates a new user global storages controller.
	 *
	 * @param string $AppName application name
	 * @param IRequest $request request object
	 * @param IL10N $l10n l10n service
	 * @param UserGlobalStoragesService $userGlobalStoragesService storage service
	 * @param IUserSession $userSession
	 */
	public function __construct(
		$AppName,
		IRequest $request,
		IL10N $l10n,
		UserGlobalStoragesService $userGlobalStoragesService,
		IUserSession $userSession,
		ILogger $logger
	) {
		parent::__construct(
			$AppName,
			$request,
			$l10n,
			$userGlobalStoragesService,
			$logger
		);
		$this->userSession = $userSession;
	}

	/**
	 * Get all storage entries
	 *
	 * @return DataResponse
	 *
	 * @NoAdminRequired
	 */
	public function index() {
		$storages = $this->service->getUniqueStorages();
abstract class User
{
    /** @return string */
    abstract public function getPassword();
}

class MyUser extends User
{
    public function getPassword()
    {
        // return something
    }

    public function getDisplayName()
    {
        // return some name.
    }
}

class AuthSystem
{
    public function authenticate(User $user)
    {
        $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName()));
        // do something.
    }
}

		// remove configuration data, this must be kept private
		foreach ($storages as $storage) {
			$this->sanitizeStorage($storage);
		}

		return new DataResponse(
			$storages,
			Http::STATUS_OK
		);
	}

	protected function manipulateStorageConfig(StorageConfig $storage) {
		/** @var AuthMechanism */
		$authMechanism = $storage->getAuthMechanism();
		$authMechanism->manipulateStorageConfig($storage, $this->userSession->getUser());
		/** @var Backend */
		$backend = $storage->getBackend();
		$backend->manipulateStorageConfig($storage, $this->userSession->getUser());
	}

	/**
	 * Get an external storage entry.
	 *
	 * @param int $id storage id
	 * @param bool $testOnly whether to storage should only test the connection or do more things
	 * @return DataResponse
	 *
	 * @NoAdminRequired
	 */
	public function show($id, $testOnly = true) {
		try {
			$storage = $this->service->getStorage($id);

			$this->updateStorageStatus($storage, $testOnly);
/** @return stdClass|null */
function mayReturnNull() { }

function doesNotAcceptNull(stdClass $x) { }

// With potential error.
function withoutCheck() {
    $x = mayReturnNull();
    doesNotAcceptNull($x); // Potential error here.
}

// Safe - Alternative 1
function withCheck1() {
    $x = mayReturnNull();
    if ( ! $x instanceof stdClass) {
        throw new \LogicException('$x must be defined.');
    }
    doesNotAcceptNull($x);
}

// Safe - Alternative 2
function withCheck2() {
    $x = mayReturnNull();
    if ($x instanceof stdClass) {
        doesNotAcceptNull($x);
    }
}
		} catch (NotFoundException $e) {
			return new DataResponse(
				[
					'message' => (string)$this->l10n->t('Storage with id "%i" not found', [$id])
				],
				Http::STATUS_NOT_FOUND
			);
		}

		$this->sanitizeStorage($storage);

		return new DataResponse(
			$storage,
			Http::STATUS_OK
		);
	}

	/**
	 * Update an external storage entry.
	 * Only allows setting user provided backend fields
	 *
	 * @param int $id storage id
	 * @param array $backendOptions backend-specific options
	 * @param bool $testOnly whether to storage should only test the connection or do more things
	 *
	 * @return DataResponse
	 *
	 * @NoAdminRequired
	 */
	public function update(
		$id,
		$backendOptions,
		$testOnly = true
	) {
		try {
			$storage = $this->service->getStorage($id);
			$authMechanism = $storage->getAuthMechanism();
			if ($authMechanism instanceof IUserProvided) {
				$authMechanism->saveBackendOptions($this->userSession->getUser(), $id, $backendOptions);
/** @return stdClass|null */
function mayReturnNull() { }

function doesNotAcceptNull(stdClass $x) { }

// With potential error.
function withoutCheck() {
    $x = mayReturnNull();
    doesNotAcceptNull($x); // Potential error here.
}

// Safe - Alternative 1
function withCheck1() {
    $x = mayReturnNull();
    if ( ! $x instanceof stdClass) {
        throw new \LogicException('$x must be defined.');
    }
    doesNotAcceptNull($x);
}

// Safe - Alternative 2
function withCheck2() {
    $x = mayReturnNull();
    if ($x instanceof stdClass) {
        doesNotAcceptNull($x);
    }
}
				$authMechanism->manipulateStorageConfig($storage, $this->userSession->getUser());
			} else {
				return new DataResponse(
					[
						'message' => (string)$this->l10n->t('Storage with id "%i" is not user editable', [$id])
					],
					Http::STATUS_FORBIDDEN
				);
			}
		} catch (NotFoundException $e) {
			return new DataResponse(
				[
					'message' => (string)$this->l10n->t('Storage with id "%i" not found', [$id])
				],
				Http::STATUS_NOT_FOUND
			);
		}

		$this->updateStorageStatus($storage, $testOnly);
/** @return stdClass|null */
function mayReturnNull() { }

function doesNotAcceptNull(stdClass $x) { }

// With potential error.
function withoutCheck() {
    $x = mayReturnNull();
    doesNotAcceptNull($x); // Potential error here.
}

// Safe - Alternative 1
function withCheck1() {
    $x = mayReturnNull();
    if ( ! $x instanceof stdClass) {
        throw new \LogicException('$x must be defined.');
    }
    doesNotAcceptNull($x);
}

// Safe - Alternative 2
function withCheck2() {
    $x = mayReturnNull();
    if ($x instanceof stdClass) {
        doesNotAcceptNull($x);
    }
}
		$this->sanitizeStorage($storage);

		return new DataResponse(
			$storage,
			Http::STATUS_OK
		);

	}

	/**
	 * Remove sensitive data from a StorageConfig before returning it to the user
	 *
	 * @param StorageConfig $storage
	 */
	protected function sanitizeStorage(StorageConfig $storage) {
		$storage->setBackendOptions([]);
		$storage->setMountOptions([]);

		if ($storage->getAuthMechanism() instanceof IUserProvided) {
			try {
				$storage->getAuthMechanism()->manipulateStorageConfig($storage, $this->userSession->getUser());
			} catch (InsufficientDataForMeaningfulAnswerException $e) {
				// not configured yet
			}
		}
	}

}


Pull Request — master (#26700)

lib/Controller/UserGlobalStoragesController.php (4 issues)

Labels

Severity

Introduced By

Upgrade to new PHP Analysis Engine

Available Fixes

1		<?php
2		/**
3		* @author Joas Schilling <[email protected]>
4		* @author Juan Pablo Villafáñez <[email protected]>
5		* @author Robin Appelman <[email protected]>
6		* @author Robin McCorkell <[email protected]>
7		*
8		* @copyright Copyright (c) 2016, ownCloud GmbH.
9		* @license AGPL-3.0
10		*
11		* This code is free software: you can redistribute it and/or modify
12		* it under the terms of the GNU Affero General Public License, version 3,
13		* as published by the Free Software Foundation.
14		*
15		* This program is distributed in the hope that it will be useful,
16		* but WITHOUT ANY WARRANTY; without even the implied warranty of
17		* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
18		* GNU Affero General Public License for more details.
19		*
20		* You should have received a copy of the GNU Affero General Public License, version 3,
21		* along with this program. If not, see <http://www.gnu.org/licenses/>
22		*
23		*/
24
25		namespace OCA\Files_External\Controller;
26
27		use OCA\Files_External\Lib\Auth\AuthMechanism;
28		use OCA\Files_External\Lib\Auth\IUserProvided;
29		use OCA\Files_External\Lib\InsufficientDataForMeaningfulAnswerException;
30		use OCP\ILogger;
31		use \OCP\IRequest;
32		use \OCP\IL10N;
33		use \OCP\AppFramework\Http\DataResponse;
34		use \OCP\AppFramework\Http;
35		use OCA\Files_External\Service\UserGlobalStoragesService;
36		use OCA\Files_External\NotFoundException;
37		use OCA\Files_External\Lib\StorageConfig;
38		use \OCA\Files_External\Lib\Backend\Backend;
39		use OCP\IUserSession;
40
41		/**
42		* User global storages controller
43		*/
44		class UserGlobalStoragesController extends StoragesController {
45		/**
46		* @var IUserSession
47		*/
48		private $userSession;
49
50		/**
51		* Creates a new user global storages controller.
52		*
53		* @param string $AppName application name
54		* @param IRequest $request request object
55		* @param IL10N $l10n l10n service
56		* @param UserGlobalStoragesService $userGlobalStoragesService storage service
57		* @param IUserSession $userSession
58		*/
59	View Code Duplication	public function __construct(
60		$AppName,
61		IRequest $request,
62		IL10N $l10n,
63		UserGlobalStoragesService $userGlobalStoragesService,
64		IUserSession $userSession,
65		ILogger $logger
66		) {
67		parent::__construct(
68		$AppName,
69		$request,
70		$l10n,
71		$userGlobalStoragesService,
72		$logger
73		);
74		$this->userSession = $userSession;
75		}
76
77		/**
78		* Get all storage entries
79		*
80		* @return DataResponse
81		*
82		* @NoAdminRequired
83		*/
84		public function index() {
85		$storages = $this->service->getUniqueStorages();
		0 ignored issues – show Bug introduced 2016-05-13 10:19 UTC by Report Bug Copy Issue Report Show Similar Issues like this It seems like you code against a specific sub-type and not the parent class `OCA\Files_External\Service\StoragesService` as the method `getUniqueStorages()` does only exist in the following sub-classes of `OCA\Files_External\Service\StoragesService`: `OCA\Files_External\Servi...erGlobalStoragesService`. Maybe you want to `instanceof` check for one of these explicitly? Let’s take a look at an example: abstract class User { /** @return string / abstract public function getPassword(); } class MyUser extends User { public function getPassword() { // return something } public function getDisplayName() { // return some name. } } class AuthSystem { public function authenticate(User $user) { $this->logger->info(sprintf('Authenticating %s.', $user->getDisplayName())); // do something. } } In the above example, the authenticate() method works fine as long as you just pass instances of MyUser. However, if you now also want to pass a different sub-classes of User which does not have a getDisplayName() method, the code will break. Available Fixes Change the type-hint for the parameter: class AuthSystem { public function authenticate(MyUser $user) { / ... / } } Add an additional type-check: class AuthSystem { public function authenticate(User $user) { if ($user instanceof MyUser) { $this->logger->info(/* ... /); } // or alternatively if ( ! $user instanceof MyUser) { throw new \LogicException( '$user must be an instance of MyUser, ' .'other instances are not supported.' ); } } } Note:* PHP Analyzer uses reverse abstract interpretation to narrow down the types inside the if block in such a case. Add the method to the parent class: abstract class User { /** @return string / abstract public function getPassword(); /* @return string */ abstract public function getDisplayName(); } Loading history...
86
87		// remove configuration data, this must be kept private
88		foreach ($storages as $storage) {
89		$this->sanitizeStorage($storage);
90		}
91
92		return new DataResponse(
93		$storages,
94		Http::STATUS_OK
95		);
96		}
97
98	View Code Duplication	protected function manipulateStorageConfig(StorageConfig $storage) {
99		/** @var AuthMechanism */
100		$authMechanism = $storage->getAuthMechanism();
101		$authMechanism->manipulateStorageConfig($storage, $this->userSession->getUser());
102		/** @var Backend */
103		$backend = $storage->getBackend();
104		$backend->manipulateStorageConfig($storage, $this->userSession->getUser());
105		}
106
107		/**
108		* Get an external storage entry.
109		*
110		* @param int $id storage id
111		* @param bool $testOnly whether to storage should only test the connection or do more things
112		* @return DataResponse
113		*
114		* @NoAdminRequired
115		*/
116	View Code Duplication	public function show($id, $testOnly = true) {
117		try {
118		$storage = $this->service->getStorage($id);
119
120		$this->updateStorageStatus($storage, $testOnly);
		0 ignored issues – show Bug introduced 2016-02-05 08:40 UTC by Report Bug Copy Issue Report Show Similar Issues like this It seems like `$storage` defined by `$this->service->getStorage($id)` on line `118` can be `null`; however, `OCA\Files_External\Contr...::updateStorageStatus()` does not accept `null`, maybe add an additional type check? Unless you are absolutely sure that the expression can never be null because of other conditions, we strongly recommend to add an additional type check to your code: /** @return stdClass\|null */ function mayReturnNull() { } function doesNotAcceptNull(stdClass $x) { } // With potential error. function withoutCheck() { $x = mayReturnNull(); doesNotAcceptNull($x); // Potential error here. } // Safe - Alternative 1 function withCheck1() { $x = mayReturnNull(); if ( ! $x instanceof stdClass) { throw new \LogicException('$x must be defined.'); } doesNotAcceptNull($x); } // Safe - Alternative 2 function withCheck2() { $x = mayReturnNull(); if ($x instanceof stdClass) { doesNotAcceptNull($x); } } Loading history...
121		} catch (NotFoundException $e) {
122		return new DataResponse(
123		[
124		'message' => (string)$this->l10n->t('Storage with id "%i" not found', [$id])
125		],
126		Http::STATUS_NOT_FOUND
127		);
128		}
129
130		$this->sanitizeStorage($storage);
131
132		return new DataResponse(
133		$storage,
134		Http::STATUS_OK
135		);
136		}
137
138		/**
139		* Update an external storage entry.
140		* Only allows setting user provided backend fields
141		*
142		* @param int $id storage id
143		* @param array $backendOptions backend-specific options
144		* @param bool $testOnly whether to storage should only test the connection or do more things
145		*
146		* @return DataResponse
147		*
148		* @NoAdminRequired
149		*/
150		public function update(
151		$id,
152		$backendOptions,
153		$testOnly = true
154		) {
155		try {
156		$storage = $this->service->getStorage($id);
157		$authMechanism = $storage->getAuthMechanism();
158		if ($authMechanism instanceof IUserProvided) {
159		$authMechanism->saveBackendOptions($this->userSession->getUser(), $id, $backendOptions);
		0 ignored issues – show Bug introduced 2016-01-30 08:40 UTC by Report Bug Copy Issue Report Show Similar Issues like this It seems like `$this->userSession->getUser()` can be `null`; however, `saveBackendOptions()` does not accept `null`, maybe add an additional type check? Unless you are absolutely sure that the expression can never be null because of other conditions, we strongly recommend to add an additional type check to your code: /** @return stdClass\|null */ function mayReturnNull() { } function doesNotAcceptNull(stdClass $x) { } // With potential error. function withoutCheck() { $x = mayReturnNull(); doesNotAcceptNull($x); // Potential error here. } // Safe - Alternative 1 function withCheck1() { $x = mayReturnNull(); if ( ! $x instanceof stdClass) { throw new \LogicException('$x must be defined.'); } doesNotAcceptNull($x); } // Safe - Alternative 2 function withCheck2() { $x = mayReturnNull(); if ($x instanceof stdClass) { doesNotAcceptNull($x); } } Loading history...
160		$authMechanism->manipulateStorageConfig($storage, $this->userSession->getUser());
161		} else {
162		return new DataResponse(
163		[
164		'message' => (string)$this->l10n->t('Storage with id "%i" is not user editable', [$id])
165		],
166		Http::STATUS_FORBIDDEN
167		);
168		}
169		} catch (NotFoundException $e) {
170		return new DataResponse(
171		[
172		'message' => (string)$this->l10n->t('Storage with id "%i" not found', [$id])
173		],
174		Http::STATUS_NOT_FOUND
175		);
176		}
177
178		$this->updateStorageStatus($storage, $testOnly);
		0 ignored issues – show Bug introduced 2016-06-07 15:28 UTC by Report Bug Copy Issue Report Show Similar Issues like this It seems like `$storage` defined by `$this->service->getStorage($id)` on line `156` can be `null`; however, `OCA\Files_External\Contr...::updateStorageStatus()` does not accept `null`, maybe add an additional type check? Unless you are absolutely sure that the expression can never be null because of other conditions, we strongly recommend to add an additional type check to your code: /** @return stdClass\|null */ function mayReturnNull() { } function doesNotAcceptNull(stdClass $x) { } // With potential error. function withoutCheck() { $x = mayReturnNull(); doesNotAcceptNull($x); // Potential error here. } // Safe - Alternative 1 function withCheck1() { $x = mayReturnNull(); if ( ! $x instanceof stdClass) { throw new \LogicException('$x must be defined.'); } doesNotAcceptNull($x); } // Safe - Alternative 2 function withCheck2() { $x = mayReturnNull(); if ($x instanceof stdClass) { doesNotAcceptNull($x); } } Loading history...
179		$this->sanitizeStorage($storage);
180
181		return new DataResponse(
182		$storage,
183		Http::STATUS_OK
184		);
185
186		}
187
188		/**
189		* Remove sensitive data from a StorageConfig before returning it to the user
190		*
191		* @param StorageConfig $storage
192		*/
193		protected function sanitizeStorage(StorageConfig $storage) {
194		$storage->setBackendOptions([]);
195		$storage->setMountOptions([]);
196
197		if ($storage->getAuthMechanism() instanceof IUserProvided) {
198		try {
199		$storage->getAuthMechanism()->manipulateStorageConfig($storage, $this->userSession->getUser());
200		} catch (InsufficientDataForMeaningfulAnswerException $e) {
201		// not configured yet
202		}
203		}
204		}
205
206		}
207

owncloud / core

Pull Request — master (#26700)

lib/Controller/UserGlobalStoragesController.php (4 issues)

Labels

Severity

Introduced By

Upgrade to new PHP Analysis Engine

Available Fixes

Duplication Side-by-Side

Filter issues like