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

Don't allow to update settings #221

Merged
merged 1 commit into from
Jun 26, 2023
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
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);
}