Skip to content

Add password confirmation #115

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

Merged
merged 4 commits into from
Oct 17, 2024
Merged

Add password confirmation #115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1d51680
ask password confirmation for sensitive admin settings
julien-nc Oct 16, 2024
5ba4ac0
fix: saving dummy values
julien-nc Oct 16, 2024
5ed3618
change default value for sensitive in the frontend
julien-nc Oct 16, 2024
5b1d2c8
update npm pkgs
julien-nc Oct 16, 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
1 change: 1 addition & 0 deletions appinfo/routes.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
['name' => 'config#oauthRedirect', 'url' => '/oauth-redirect', 'verb' => 'GET'],
['name' => 'config#setConfig', 'url' => '/config', 'verb' => 'PUT'],
['name' => 'config#setAdminConfig', 'url' => '/admin-config', 'verb' => 'PUT'],
['name' => 'config#setSensitiveAdminConfig', 'url' => '/sensitive-admin-config', 'verb' => 'PUT'],
['name' => 'config#popupSuccessPage', 'url' => '/popup-success', 'verb' => 'GET'],

['name' => 'githubAPI#getNotifications', 'url' => '/notifications', 'verb' => 'GET'],
Expand Down
22 changes: 21 additions & 1 deletion lib/Controller/ConfigController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,10 @@
use OCA\Github\Service\GithubAPIService;
use OCA\Github\Service\SecretService;
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\PasswordConfirmationRequired;
use OCP\AppFramework\Http\DataResponse;
use OCP\AppFramework\Http\RedirectResponse;
use OCP\AppFramework\Http\TemplateResponse;
Expand Down Expand Up @@ -100,14 +102,32 @@ public function setConfig(array $values): DataResponse {
* @return DataResponse
*/
public function setAdminConfig(array $values): DataResponse {
foreach ($values as $key => $value) {
if (in_array($key, ['client_id', 'client_secret', 'default_link_token'], true)) {
return new DataResponse([], Http::STATUS_BAD_REQUEST);
} else {
$this->config->setAppValue(Application::APP_ID, $key, $value);
}
}
return new DataResponse([]);
}

/**
* Set admin config values
*
* @param array $values key/value pairs to store in app config
* @return DataResponse
*/
#[PasswordConfirmationRequired]
public function setSensitiveAdminConfig(array $values): DataResponse {
foreach ($values as $key => $value) {
if (in_array($key, ['client_id', 'client_secret', 'default_link_token'], true)) {
$this->secretService->setEncryptedAppValue($key, $value);
} else {
$this->config->setAppValue(Application::APP_ID, $key, $value);
Copy link
Contributor

Choose a reason for hiding this comment

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

let's throw a bad request response here

Copy link
Member Author

Choose a reason for hiding this comment

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

Why? Where?

Copy link
Contributor

Choose a reason for hiding this comment

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

in the else block since this function should only serve to sensitive values. It would be cleaner but is fine either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's fine to set any value with this setSensitiveAdminConfig endpoint.
We just want to prevent using wrong keys in the setAdminConfig endpoint.

}
}
return new DataResponse(1);
return new DataResponse([]);
}

/**
Expand Down
Loading
Loading