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.

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

OP#40534 only allow user to connect via OAuth #35

Merged
merged 2 commits into from
Feb 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
45 changes: 9 additions & 36 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,7 @@ a search provider for work packages and notifications for changes in active work

### :lock: Authentication

To access data in OpenProject on behalf of the user this app needs to authenticate to OpenProject as the respective user.

This can happen either through a personal access token or the OAuth workflow. Both ways to authenticate have their own advantages and disadvantages.

Using a personal access token enables every NextCloud user to connect to an OpenProject instance of their choice because no configuration needs to be changed by the NextCloud or OpenProject admin (except for installing and enabling this app).
On the other hand every user has to perform a series of manual steps to connect NextCloud to OpenProject the first time. Also, this is the less secure way of authentication. If the API Key gets leaked the attacker can do any actions on OpenProject as if they had been done by the legitimate user. Even though the key can be reset, it usually stays the same for a long time. If an attacker gets even short-time access to the NextCloud system or access to a message transferred between NextCloud and OpenProject he could misuse that knowledge for as long as the API key stays unchanged.

Using the OAuth authentication is much easier for every user, but requires the NextCloud admin and the OpenProject admin to configure both apps.
OAuth is also the much safer option to connect both apps and therefore the recommended way to use this app.
To give access to OpenProject the first time the user needs to log-in with the OpenProject credentials and actively approve the connection. In the result a user-token will be generated automatically and exchanged between NextCloud and OpenProject. This token will be refreshed on a regular basis, so if an attacker gains access to a message transferred between NextCloud and OpenProject it can be only misused till the next refreshing of the token happens.

The account configuration happens in the "Connected accounts" user settings section. A link to the "Connected accounts" user settings section will be displayed in the widget for users who didn't configure an OpenProject account.

#### OAuth
To access data in OpenProject on behalf of the user this app needs to authenticate to OpenProject as the respective user. This happens using the OAuth workflow. (Using a personal access token is deprecated and not possible anymore.)

1. As an OpenProject admin create an OAuth app
1. in OpenProject go to "Administration" -> "Authentication" -> "OAuth applications"
Expand All @@ -31,28 +18,14 @@ The account configuration happens in the "Connected accounts" user settings sect
1. in NextCloud go to "Settings" -> "Personal" -> "Connected accounts"
2. provide the OpenProject address, the Client ID and the Client Secret
3. As an NextCloud user connect to OpenProject
1. in NextCloud go to "Settings" -> "Personal" -> "Connected accounts"
2. provide the OpenProject address (it has to be exactly the same as provided by the administrator in step 2)
3. a new button `Connect to OpenProject` should be visible
4. click `Connect to OpenProject`
5. you will be redirected to OpenProject
6. log-in to OpenProject if you haven't already
7. Authorize the NextCloud App
8. you will be redirected back to NextCloud

#### Personal access token (NOT recommended)

1. As an OpenProject user get an access token (API key)
1. in OpenProject click on your user image in the top right corner
2. go to "My account" -> "Access token"
3. click on "Generate" button in the "API" row. If there is no "Generate" button you have already created an API token for this user and in case you don't know it any-more you can always create a new one by clicking "Reset", but this will invalidate any old token.
4. note down the API token that is displayed
2. As an NextCloud user connect to OpenProject
1. in NextCloud go to "Settings" -> "Personal" -> "Connected accounts"
2. provide the OpenProject address
3. enter or copy the OpenProject API token into the "Access token" field
4. after a short time the app will try to establish the connection to OpenProject and if all worked correctly it will display the status: "Connected as <fullname of user in OpenProject>"

1. click the `Connect to OpenProject` button that you can find on:
- the OpenProject dashboard widget
- the OpenProject tab in the details of every file
- "Settings" -> "Personal" -> "Connected accounts"
2. you will be redirected to OpenProject
3. log-in to OpenProject if you haven't already
4. Authorize the NextCloud App
5. you will be redirected back to NextCloud

#### Background jobs

Expand Down
2 changes: 1 addition & 1 deletion lib/AppInfo/Application.php
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ public function registerNavigation(IUserSession $userSession): void {
$container = $this->getContainer();

if ($this->config->getUserValue($userId, self::APP_ID, 'navigation_enabled', '0') === '1') {
$openprojectUrl = $this->config->getUserValue($userId, self::APP_ID, 'url', '');
$openprojectUrl = $this->config->getAppValue(Application::APP_ID, 'oauth_instance_url', '');
if ($openprojectUrl !== '') {
$container->get(INavigationManager::class)->add(function () use ($container, $openprojectUrl) {
$urlGenerator = $container->get(IURLGenerator::class);
Expand Down
4 changes: 2 additions & 2 deletions lib/Controller/ConfigController.php
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ public function oauthRedirect(string $code = '', string $state = ''): RedirectRe

if ($clientID && $clientSecret && $configState !== '' && $configState === $state) {
$redirect_uri = $this->config->getUserValue($this->userId, Application::APP_ID, 'redirect_uri');
$openprojectUrl = $this->config->getUserValue($this->userId, Application::APP_ID, 'url');
$openprojectUrl = $this->config->getAppValue(Application::APP_ID, 'oauth_instance_url');
$result = $this->openprojectAPIService->requestOAuthAccessToken($openprojectUrl, [
'client_id' => $clientID,
'client_secret' => $clientSecret,
Expand Down Expand Up @@ -178,7 +178,7 @@ private function storeUserInfo(string $accessToken): array {
$refreshToken = $this->config->getUserValue($this->userId, Application::APP_ID, 'refresh_token');
$clientID = $this->config->getAppValue(Application::APP_ID, 'client_id');
$clientSecret = $this->config->getAppValue(Application::APP_ID, 'client_secret');
$openprojectUrl = $this->config->getUserValue($this->userId, Application::APP_ID, 'url');
$openprojectUrl = $this->config->getAppValue(Application::APP_ID, 'oauth_instance_url');

if (!$openprojectUrl || !OpenProjectAPIService::validateOpenProjectURL($openprojectUrl)) {
return ['error' => 'OpenProject URL is invalid'];
Expand Down
2 changes: 1 addition & 1 deletion lib/Controller/OpenProjectAPIController.php
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public function __construct(string $appName,
$this->refreshToken = $config->getUserValue($userId, Application::APP_ID, 'refresh_token');
$this->clientID = $config->getAppValue(Application::APP_ID, 'client_id');
$this->clientSecret = $config->getAppValue(Application::APP_ID, 'client_secret');
$this->openprojectUrl = $config->getUserValue($userId, Application::APP_ID, 'url');
$this->openprojectUrl = $config->getAppValue(Application::APP_ID, 'oauth_instance_url');
}

/**
Expand Down
27 changes: 24 additions & 3 deletions lib/Dashboard/OpenProjectWidget.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@

namespace OCA\OpenProject\Dashboard;

use OCA\OpenProject\Service\OpenProjectAPIService;
use OCP\AppFramework\Services\IInitialState;
use OCP\Dashboard\IWidget;
use OCP\IConfig;
use OCP\IL10N;
use OCP\IURLGenerator;
use OCP\Util;
Expand All @@ -32,17 +35,33 @@

class OpenProjectWidget implements IWidget {

/** @var IL10N */
/**
* @var IL10N
*/
private $l10n;
/**
* @var IURLGenerator
*/
private $url;
/**
* @var IInitialState
*/
private $initialStateService;
/**
* @var IConfig
*/
private $config;

public function __construct(IL10N $l10n,
IURLGenerator $url) {
public function __construct(
IL10N $l10n,
IInitialState $initialStateService,
IURLGenerator $url,
IConfig $config
) {
$this->initialStateService = $initialStateService;
$this->l10n = $l10n;
$this->url = $url;
$this->config = $config;
}

/**
Expand Down Expand Up @@ -86,5 +105,7 @@ public function getUrl(): ?string {
public function load(): void {
Util::addScript(Application::APP_ID, Application::APP_ID . '-dashboard');
Util::addStyle(Application::APP_ID, 'dashboard');
$requestUrl = OpenProjectAPIService::getOpenProjectOauthURL($this->config, $this->url);
$this->initialStateService->provideInitialState('request-url', $requestUrl);
}
}
30 changes: 30 additions & 0 deletions lib/Listener/LoadSidebarScript.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,39 @@

use OCA\Files\Event\LoadSidebar;
use OCA\OpenProject\AppInfo\Application;
use OCA\OpenProject\Service\OpenProjectAPIService;
use OCP\AppFramework\Services\IInitialState;
use OCP\EventDispatcher\Event;
use OCP\EventDispatcher\IEventListener;
use OCP\IConfig;
use OCP\IURLGenerator;
use OCP\Util;

class LoadSidebarScript implements IEventListener {

/**
* @var IURLGenerator
*/
private $url;
/**
* @var IInitialState
*/
private $initialStateService;
/**
* @var IConfig
*/
private $config;

public function __construct(
IInitialState $initialStateService,
IURLGenerator $url,
IConfig $config
) {
$this->initialStateService = $initialStateService;
$this->config = $config;
$this->url = $url;
}

public function handle(Event $event): void {
if (!($event instanceof LoadSidebar)) {
return;
Expand All @@ -46,5 +74,7 @@ public function handle(Event $event): void {
Util::addScript(Application::APP_ID, 'integration_openproject-projectTab');
}
Util::addStyle(Application::APP_ID, 'tab');
$requestUrl = OpenProjectAPIService::getOpenProjectOauthURL($this->config, $this->url);
$this->initialStateService->provideInitialState('request-url', $requestUrl);
}
}
2 changes: 1 addition & 1 deletion lib/Search/OpenProjectSearchProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ public function search(IUser $user, ISearchQuery $query): SearchResult {
? $svgUrl . '?color=ffffff'
: $svgUrl . '?color=000000';

$openprojectUrl = $this->config->getUserValue($user->getUID(), Application::APP_ID, 'url');
$openprojectUrl = $this->config->getAppValue(Application::APP_ID, 'oauth_instance_url');
$accessToken = $this->config->getUserValue($user->getUID(), Application::APP_ID, 'token');
$tokenType = $this->config->getUserValue($user->getUID(), Application::APP_ID, 'token_type');
$refreshToken = $this->config->getUserValue($user->getUID(), Application::APP_ID, 'refresh_token');
Expand Down
22 changes: 21 additions & 1 deletion lib/Service/OpenProjectAPIService.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use DateTimeZone;
use Exception;
use OCP\IL10N;
use OCP\IURLGenerator;
use Psr\Log\LoggerInterface;
use OCP\IConfig;
use OCP\IUserManager;
Expand Down Expand Up @@ -108,7 +109,7 @@ private function checkNotificationsForUser(string $userId): void {
$refreshToken = $this->config->getUserValue($userId, Application::APP_ID, 'refresh_token');
$clientID = $this->config->getAppValue(Application::APP_ID, 'client_id');
$clientSecret = $this->config->getAppValue(Application::APP_ID, 'client_secret');
$openprojectUrl = $this->config->getUserValue($userId, Application::APP_ID, 'url');
$openprojectUrl = $this->config->getAppValue(Application::APP_ID, 'oauth_instance_url');
if ($clientID && $clientSecret && $openprojectUrl) {
$lastNotificationCheck = $this->config->getUserValue($userId, Application::APP_ID, 'last_notification_check');
$lastNotificationCheck = $lastNotificationCheck === '' ? null : $lastNotificationCheck;
Expand Down Expand Up @@ -560,4 +561,23 @@ public function getOpenProjectWorkPackageType(

return $result;
}

/*
* @param IConfig $config
* @param IURLGenerator $urlGenerator
* @return string
* generates an oauth url to OpenProject containing client_id & redirect_uri as parameter
* please note that the state parameter is still missing, that needs to be generated dynamically
* and saved to the DB before calling the OAuth URI
*/
public static function getOpenProjectOauthURL(IConfig $config, IURLGenerator $urlGenerator): string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IURLGenerator could be injected in the constructor of OpenProjectAPIService instead of passing it as a parameter to getOpenProjectOauthURL. This would avoid having to inject IURLGenerator in every class calling this method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but then every class that calls the method would have to initiate the complete OpenProjectAPIService and by that need to get injected even more
e..g from OpenProjectWidget we would need the logger, usermanager, avatarmanager etc.
Or maybe we want to place that function somewhere else? A helper class?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I agree. As long as getOpenProjectOauthURL is a static method, let's keep it that way.
If you feel like it belongs in a new class/file, feel free to move it 😁.

$clientID = $config->getAppValue(Application::APP_ID, 'client_id');
$oauthUrl = $config->getAppValue(Application::APP_ID, 'oauth_instance_url');
$redirectUri = $urlGenerator->linkToRouteAbsolute(Application::APP_ID . '.config.oauthRedirect');
return $oauthUrl .
'/oauth/authorize' .
'?client_id=' . $clientID .
'&redirect_uri=' . urlencode($redirectUri) .
'&response_type=code';
}
}
21 changes: 10 additions & 11 deletions lib/Settings/Personal.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@
use OCP\AppFramework\Http\TemplateResponse;
use OCP\AppFramework\Services\IInitialState;
use OCP\IConfig;
use OCP\IURLGenerator;
use OCP\Settings\ISettings;

use OCA\OpenProject\AppInfo\Application;
use OCA\OpenProject\Service\OpenProjectAPIService;

class Personal implements ISettings {

Expand All @@ -23,45 +25,42 @@ class Personal implements ISettings {
* @var string|null
*/
private $userId;
/**
* @var IURLGenerator
*/
private $url;

public function __construct(
IConfig $config,
IInitialState $initialStateService,
IURLGenerator $url,
?string $userId) {
$this->config = $config;
$this->initialStateService = $initialStateService;
$this->url = $url;
$this->userId = $userId;
}

/**
* @return TemplateResponse
*/
public function getForm(): TemplateResponse {
$login = $this->config->getUserValue($this->userId, Application::APP_ID, 'login');
$token = $this->config->getUserValue($this->userId, Application::APP_ID, 'token');
$userName = $this->config->getUserValue($this->userId, Application::APP_ID, 'user_name');
$url = $this->config->getUserValue($this->userId, Application::APP_ID, 'url');
$searchEnabled = $this->config->getUserValue($this->userId, Application::APP_ID, 'search_enabled', '0');
$notificationEnabled = $this->config->getUserValue($this->userId, Application::APP_ID, 'notification_enabled', '0');
$navigationEnabled = $this->config->getUserValue($this->userId, Application::APP_ID, 'navigation_enabled', '0');

// for OAuth
$clientID = $this->config->getAppValue(Application::APP_ID, 'client_id');
// don't expose the client secret to users
$clientSecret = ($this->config->getAppValue(Application::APP_ID, 'client_secret') !== '');
$oauthUrl = $this->config->getAppValue(Application::APP_ID, 'oauth_instance_url');
$requestUrl = OpenProjectAPIService::getOpenProjectOauthURL($this->config, $this->url);

$userConfig = [
'login' => $login,
'token' => $token,
'url' => $url,
'client_id' => $clientID,
'client_secret' => $clientSecret,
'oauth_instance_url' => $oauthUrl,
'search_enabled' => ($searchEnabled === '1'),
'notification_enabled' => ($notificationEnabled === '1'),
'navigation_enabled' => ($navigationEnabled === '1'),
'user_name' => $userName,
'request_url' => $requestUrl,
];
$this->initialStateService->provideInitialState('user-config', $userConfig);
return new TemplateResponse(Application::APP_ID, 'personalSettings');
Expand Down
49 changes: 49 additions & 0 deletions src/components/OAuthConnectButton.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<template>
<button
class="openproject-oauth"
@click="onOAuthClick">
<span class="icon icon-external" />
{{ t('integration_openproject', 'Connect to OpenProject') }}
</button>
</template>
<script>
import axios from '@nextcloud/axios'
import { generateUrl } from '@nextcloud/router'
import { showError } from '@nextcloud/dialogs'
import { translate as t } from '@nextcloud/l10n'

export default {
name: 'OAuthConnectButton',

props: {
requestUrl: {
type: String,
required: true,
},
},
methods: {
onOAuthClick() {
const oauthState = Math.random().toString(36).substring(3)
const requestUrl = this.requestUrl
+ '&state=' + encodeURIComponent(oauthState)

const req = {
values: {
oauth_state: oauthState,
},
}
const url = generateUrl('/apps/integration_openproject/config')
axios.put(url, req)
.then(() => {
window.location.replace(requestUrl)
})
.catch((error) => {
showError(
t('integration_openproject', 'Failed to save OpenProject OAuth state')
+ ': ' + error.message
)
})
},
},
}
</script>
Loading