Skip to content

Commit

Permalink
feat!: [torrust#144] don't allow to update settings
Browse files Browse the repository at this point in the history
Without restarting the application. This feature was using the `config.toml` file. That approach is not good for theses reasons:

- If you use env vars to inject the settings, there is no `config.toml` file.
- In dockerized (clouds) envs it's harder to mount a file than injecting env vars. Sometimes it's only allowed to mount a single file.
  • Loading branch information
josecelano committed Jun 26, 2023
1 parent f0e28a6 commit f85e153
Show file tree
Hide file tree
Showing 8 changed files with 6 additions and 120 deletions.
28 changes: 0 additions & 28 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -372,34 +372,6 @@ impl Configuration {
fs::write(config_path, toml_string).expect("Could not write to file!");
}

/// Update the settings file based upon a supplied `new_settings`.
///
/// # Errors
///
/// Todo: Make an error if the save fails.
///
/// # Panics
///
/// Will panic if the configuration file path is not defined. That happens
/// when the configuration was loaded from the environment variable.
pub async fn update_settings(&self, new_settings: TorrustBackend) -> Result<(), ()> {
match &self.config_path {
Some(config_path) => {
let mut settings = self.settings.write().await;
*settings = new_settings;

drop(settings);

let _ = self.save_to_file(config_path).await;

Ok(())
}
None => panic!(
"Cannot update settings when the config file path is not defined. For example: when it's loaded from env var."
),
}
}

pub async fn get_all(&self) -> TorrustBackend {
let settings_lock = self.settings.read().await;

Expand Down
19 changes: 0 additions & 19 deletions src/services/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,25 +37,6 @@ impl Service {
Ok(self.configuration.get_all().await)
}

/// It updates all the settings.
///
/// # Errors
///
/// It returns an error if the user does not have the required permissions.
pub async fn update_all(&self, torrust_backend: TorrustBackend, user_id: &UserId) -> Result<TorrustBackend, ServiceError> {
let user = self.user_repository.get_compact(user_id).await?;

// Check if user is administrator
// todo: extract authorization service
if !user.administrator {
return Err(ServiceError::Unauthorized);
}

let _ = self.configuration.update_settings(torrust_backend).await;

Ok(self.configuration.get_all().await)
}

/// It gets only the public settings.
///
/// # Errors
Expand Down
33 changes: 2 additions & 31 deletions src/web/api/v1/contexts/settings/handlers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,12 @@
//! context.
use std::sync::Arc;

use axum::extract::{self, State};
use axum::extract::State;
use axum::response::{IntoResponse, Json, Response};

use crate::common::AppData;
use crate::config::TorrustBackend;
use crate::web::api::v1::extractors::bearer_token::Extract;
use crate::web::api::v1::responses::{self};
use crate::web::api::v1::responses;

/// Get all settings.
///
Expand Down Expand Up @@ -46,31 +45,3 @@ pub async fn get_site_name_handler(State(app_data): State<Arc<AppData>>) -> Resp

Json(responses::OkResponseData { data: site_name }).into_response()
}

/// Update all the settings.
///
/// # Errors
///
/// This function will return an error if:
///
/// - The user does not have permission to update the settings.
/// - The settings could not be updated because they were loaded from env vars.
/// See <https://github.com/torrust/torrust-index-backend/issues/144.>
#[allow(clippy::unused_async)]
pub async fn update_handler(
State(app_data): State<Arc<AppData>>,
Extract(maybe_bearer_token): Extract,
extract::Json(torrust_backend): extract::Json<TorrustBackend>,
) -> Response {
let user_id = match app_data.auth.get_user_id_from_bearer_token(&maybe_bearer_token).await {
Ok(user_id) => user_id,
Err(error) => return error.into_response(),
};

let new_settings = match app_data.settings_service.update_all(torrust_backend, &user_id).await {
Ok(new_settings) => new_settings,
Err(error) => return error.into_response(),
};

Json(responses::OkResponseData { data: new_settings }).into_response()
}
7 changes: 3 additions & 4 deletions src/web/api/v1/contexts/settings/routes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,16 @@
//! Refer to the [API endpoint documentation](crate::web::api::v1::contexts::settings).
use std::sync::Arc;

use axum::routing::{get, post};
use axum::routing::get;
use axum::Router;

use super::handlers::{get_all_handler, get_public_handler, get_site_name_handler, update_handler};
use super::handlers::{get_all_handler, get_public_handler, get_site_name_handler};
use crate::common::AppData;

/// Routes for the [`category`](crate::web::api::v1::contexts::category) API context.
pub fn router(app_data: Arc<AppData>) -> Router {
Router::new()
.route("/", get(get_all_handler).with_state(app_data.clone()))
.route("/name", get(get_site_name_handler).with_state(app_data.clone()))
.route("/public", get(get_public_handler).with_state(app_data.clone()))
.route("/", post(update_handler).with_state(app_data))
.route("/public", get(get_public_handler).with_state(app_data))
}
8 changes: 1 addition & 7 deletions tests/common/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ use serde::Serialize;

use super::connection_info::ConnectionInfo;
use super::contexts::category::forms::{AddCategoryForm, DeleteCategoryForm};
use super::contexts::settings::form::UpdateSettings;
use super::contexts::tag::forms::{AddTagForm, DeleteTagForm};
use super::contexts::torrent::forms::UpdateTorrentFrom;
use super::contexts::torrent::requests::InfoHash;
Expand All @@ -17,8 +16,7 @@ pub struct Client {
}

impl Client {
// todo: forms in POST requests can be passed by reference. It's already
// changed for the `update_settings` method.
// todo: forms in POST requests can be passed by reference.

fn base_path() -> String {
"/v1".to_string()
Expand Down Expand Up @@ -104,10 +102,6 @@ impl Client {
self.http_client.get("/settings", Query::empty()).await
}

pub async fn update_settings(&self, update_settings_form: &UpdateSettings) -> TextResponse {
self.http_client.post("/settings", &update_settings_form).await
}

// Context: torrent

pub async fn get_torrents(&self, params: Query) -> TextResponse {
Expand Down
3 changes: 0 additions & 3 deletions tests/common/contexts/settings/form.rs

This file was deleted.

1 change: 0 additions & 1 deletion tests/common/contexts/settings/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
pub mod form;
pub mod responses;

use serde::{Deserialize, Serialize};
Expand Down
27 changes: 0 additions & 27 deletions tests/e2e/web/api/v1/contexts/settings/contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,30 +65,3 @@ async fn it_should_allow_admins_to_get_all_the_settings() {

assert_json_ok_response(&response);
}

#[tokio::test]
async fn it_should_allow_admins_to_update_all_the_settings() {
let mut env = TestEnv::new();
env.start(api::Version::V1).await;

if !env.is_isolated() {
// This test can't be executed in a non-isolated environment because
// it will change the settings for all the other tests.
return;
}

let logged_in_admin = new_logged_in_admin(&env).await;
let client = Client::authenticated(&env.server_socket_addr().unwrap(), &logged_in_admin.token);

let mut new_settings = env.server_settings().unwrap();

new_settings.website.name = "UPDATED NAME".to_string();

let response = client.update_settings(&new_settings).await;

let res: AllSettingsResponse = serde_json::from_str(&response.body).unwrap();

assert_eq!(res.data, new_settings);

assert_json_ok_response(&response);
}

0 comments on commit f85e153

Please sign in to comment.