fix: Short url fixes (#4773)

src/common/utils/auth.rs

@@ -406,6 +406,7 @@ impl FromRequest for AuthExtractor {
                 || (method.eq("LIST") && path.contains("pipelines"))
                 || path.contains("clusters")
                 || path.contains("query_manager")
+                || path.contains("/short")
             {
                 return ready(Ok(AuthExtractor {
                     auth: auth_str.to_owned(),

src/common/utils/redirect_response.rs

@@ -13,7 +13,7 @@
 // You should have received a copy of the GNU Affero General Public License
 // along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-use std::fmt;
+use std::{collections::HashMap, fmt};
 
 use actix_web::{HttpResponse, ResponseError};
 
@@ -21,33 +21,46 @@ const DEFAULT_REDIRECT_RELATIVE_URI: &str = "/web/";
 
 #[derive(Debug)]
 pub struct RedirectResponse {
-    pub redirect_relative_uri: String,
+    redirect_relative_uri: String,
+    query_params: HashMap<String, String>,
 }
 
 impl RedirectResponse {
-    /// Create a new `RedirectResponse` with the given redirection URL.
-    ///
-    /// If no URL is provided, a default URL (`/web`) will be used.
-    pub fn new(redirect_relative_uri: &str) -> Self {
-        RedirectResponse {
-            redirect_relative_uri: redirect_relative_uri.to_string(),
-        }
-    }
-
-    pub fn redirect(&self) -> HttpResponse {
+    pub fn redirect_http(&self) -> HttpResponse {
         self.build_redirect_response()
     }
 
+    fn build_full_redirect_uri(&self) -> String {
+        let mut redirect_uri = self.redirect_relative_uri.clone();
+
+        // If there are query parameters, append them to the URI.
+        if !self.query_params.is_empty() {
+            let query_string: String = self
+                .query_params
+                .iter()
+                .map(|(key, value)| format!("{}={}", key, value))
+                .collect::<Vec<String>>()
+                .join("&");
+
+            redirect_uri = format!("{}?{}", redirect_uri, query_string);
+        }
+
+        redirect_uri
+    }
+
     fn build_redirect_response(&self) -> HttpResponse {
+        let redirect_uri = self.build_full_redirect_uri();
+
         HttpResponse::Found()
-            .append_header(("Location", self.redirect_relative_uri.clone()))
+            .append_header(("Location", redirect_uri))
             .finish()
     }
 }
 
 impl fmt::Display for RedirectResponse {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(f, "Redirecting to {}", self.redirect_relative_uri)
+        let redirect_uri = self.build_full_redirect_uri();
+        write!(f, "Redirecting to {}", redirect_uri)
     }
 }
 
@@ -58,10 +71,104 @@ impl ResponseError for RedirectResponse {
     }
 }
 
-impl Default for RedirectResponse {
-    fn default() -> Self {
+pub struct RedirectResponseBuilder {
+    redirect_relative_uri: String,
+    query_params: HashMap<String, String>,
+}
+
+impl RedirectResponseBuilder {
+    /// Create a new `RedirectResponseBuilder` with a given redirection URL.
+    ///
+    /// If no URL is provided, a default URL (`/web`) will be used.
+    pub fn new(redirect_relative_uri: &str) -> Self {
+        RedirectResponseBuilder {
+            redirect_relative_uri: redirect_relative_uri.to_string(),
+            query_params: HashMap::new(),
+        }
+    }
+
+    pub fn with_query_param(mut self, key: &str, value: &str) -> Self {
+        self.query_params.insert(key.to_string(), value.to_string());
+        self
+    }
+
+    pub fn build(self) -> RedirectResponse {
         RedirectResponse {
-            redirect_relative_uri: DEFAULT_REDIRECT_RELATIVE_URI.to_string(),
+            redirect_relative_uri: self.redirect_relative_uri,
+            query_params: self.query_params,
         }
     }
 }
+
+impl Default for RedirectResponseBuilder {
+    fn default() -> Self {
+        RedirectResponseBuilder {
+            redirect_relative_uri: DEFAULT_REDIRECT_RELATIVE_URI.to_string(),
+            query_params: HashMap::new(),
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use actix_web::{http::header::LOCATION, HttpResponse};
+
+    use super::*;
+
+    #[test]
+    fn test_redirect_with_short_url() {
+        // Build a RedirectResponse with a short_url query parameter
+        let redirect_response = RedirectResponseBuilder::new("/web")
+            .with_query_param(
+                "redirect_url",
+                "http://localhost:5080/api/default/short/1234",
+            )
+            .build();
+
+        // Check if the constructed URI is correct
+        let expected_uri = "/web?redirect_url=http://localhost:5080/api/default/short/1234";
+        assert_eq!(redirect_response.build_full_redirect_uri(), expected_uri);
+
+        // Check if the HTTP response contains the correct "Location" header
+        let http_response: HttpResponse = redirect_response.redirect_http();
+        assert_eq!(http_response.status(), actix_web::http::StatusCode::FOUND);
+        assert_eq!(http_response.headers().get(LOCATION).unwrap(), expected_uri);
+    }
+
+    #[test]
+    fn test_redirect_without_query_params() {
+        // Build a RedirectResponse without any query parameters
+        let redirect_response = RedirectResponseBuilder::new("/web").build();
+
+        // Check if the constructed URI is correct (no query params)
+        let expected_uri = "/web";
+        assert_eq!(redirect_response.build_full_redirect_uri(), expected_uri);
+
+        // Check if the HTTP response contains the correct "Location" header
+        let http_response: HttpResponse = redirect_response.redirect_http();
+        assert_eq!(http_response.status(), actix_web::http::StatusCode::FOUND);
+        assert_eq!(http_response.headers().get(LOCATION).unwrap(), expected_uri);
+    }
+
+    #[test]
+    fn test_redirect_with_multiple_query_params() {
+        // Build a RedirectResponse with multiple query parameters
+        let redirect_response = RedirectResponseBuilder::new("/web")
+            .with_query_param(
+                "redirect_url",
+                "http://localhost:5080/api/default/short/1234",
+            )
+            .with_query_param("user_id", "42")
+            .build();
+
+        // Check if the constructed URI is correct with multiple query parameters
+        let expected_uri =
+            "/web?redirect_url=http://localhost:5080/api/default/short/1234&user_id=42";
+        assert_eq!(redirect_response.build_full_redirect_uri(), expected_uri);
+
+        // Check if the HTTP response contains the correct "Location" header
+        let http_response: HttpResponse = redirect_response.redirect_http();
+        assert_eq!(http_response.status(), actix_web::http::StatusCode::FOUND);
+        assert_eq!(http_response.headers().get(LOCATION).unwrap(), expected_uri);
+    }
+}

src/handler/http/auth/validator.rs

@@ -33,7 +33,7 @@ use crate::{
         },
         utils::{
             auth::{get_hash, is_root_user, AuthExtractor},
-            redirect_response::RedirectResponse,
+            redirect_response::RedirectResponseBuilder,
         },
     },
     service::{db, users},
@@ -643,35 +643,34 @@ fn get_user_details(decoded: String) -> Option<(String, String)> {
 ///
 /// If the authentication is invalid, it returns an `ErrorUnauthorized` error.
 pub async fn oo_validator(
-    req: ServiceRequest,
-    auth_info: AuthExtractor,
-) -> Result<ServiceRequest, (Error, ServiceRequest)> {
-    let path_prefix = "/api/";
-    oo_validator_internal(req, auth_info, path_prefix).await
-}
-
-/// Validates the authentication result and redirects on failure for requests with the `/short/`
-/// prefix.
-///
-/// This function is a proxy for the `oo_validator_internal` function, setting the `path_prefix` to
-/// "/short/".
-///
-/// # Errors
-/// If authentication fails, the function logs the failure and returns an error that causes a
-/// redirect to a predefined URL.
-pub async fn validate_short_or_redirect(
     req: ServiceRequest,
     auth_result: Result<AuthExtractor, Error>,
 ) -> Result<ServiceRequest, (Error, ServiceRequest)> {
-    match auth_result {
-        Ok(auth_info) => {
-            let path_prefix = "/short/";
-            match oo_validator_internal(req, auth_info, path_prefix).await {
-                Ok(req) => Ok(req),
-                Err((err, err_req)) => Err(handle_auth_failure_for_redirect(err_req, &err)),
+    let path_prefix = "/api/";
+    let path = extract_relative_path(req.request().path(), path_prefix);
+    let path_columns = path.split('/').collect::<Vec<&str>>();
+    let is_short_url = is_short_url_path(&path_columns);
+
+    let auth_info = match auth_result {
+        Ok(info) => info,
+        Err(e) => {
+            return if is_short_url {
+                Err(handle_auth_failure_for_redirect(req, &e))
+            } else {
+                Err((e, req))
+            };
+        }
+    };
+
+    match oo_validator_internal(req, auth_info, path_prefix).await {
+        Ok(service_req) => Ok(service_req),
+        Err((err, err_req)) => {
+            if is_short_url {
+                Err(handle_auth_failure_for_redirect(err_req, &err))
+            } else {
+                Err((err, err_req))
             }
         }
-        Err(err) => Err(handle_auth_failure_for_redirect(req, &err)),
     }
 }
 
@@ -800,20 +799,55 @@ pub(crate) async fn list_objects_for_user(
     }
 }
 
+/// Helper function to extract the relative path after the base URI and path prefix
+fn extract_relative_path(full_path: &str, path_prefix: &str) -> String {
+    let base_uri = config::get_config().common.base_uri.clone();
+    let full_prefix = format!("{}{}", base_uri, path_prefix);
+    full_path
+        .strip_prefix(&full_prefix)
+        .unwrap_or(full_path)
+        .to_string()
+}
+
+/// Helper function to check if the path corresponds to a short URL
+fn is_short_url_path(path_columns: &[&str]) -> bool {
+    path_columns
+        .get(1)
+        .map_or(false, |&segment| segment.to_lowercase() == "short")
+}
+
 /// Handles authentication failure by logging the error and returning a redirect response.
 ///
 /// This function is responsible for logging the authentication failure and returning a redirect
 /// response. It takes in the request and the error message, and returns a tuple containing the
 /// redirect response and the service request.
 fn handle_auth_failure_for_redirect(req: ServiceRequest, error: &Error) -> (Error, ServiceRequest) {
-    let redirect = RedirectResponse::default();
+    let full_url = extract_full_url(&req);
+    let redirect_http = RedirectResponseBuilder::default()
+        .with_query_param("short_url", &full_url)
+        .build();
     log::warn!(
-        "Authentication failed for path: {}, err: {}, redirecting to {}",
+        "Authentication failed for path: {}, err: {}, {}",
         req.path(),
         error,
-        &redirect.redirect_relative_uri,
+        &redirect_http,
     );
-    (redirect.into(), req)
+    (redirect_http.into(), req)
+}
+
+/// Extracts the full URL from the request.
+fn extract_full_url(req: &ServiceRequest) -> String {
+    let connection_info = req.connection_info();
+    let scheme = connection_info.scheme();
+    let host = connection_info.host();
+    let path = req
+        .request()
+        .uri()
+        .path_and_query()
+        .map(|pq| pq.as_str())
+        .unwrap_or("");
+
+    format!("{}://{}{}", scheme, host, path)
 }
 
 #[cfg(test)]

src/handler/http/request/short_url/mod.rs

@@ -15,10 +15,17 @@
 
 use std::io::Error;
 
+use actix_http::StatusCode;
 use actix_web::{get, post, web, HttpRequest, HttpResponse};
 use config::meta::short_url::ShortenUrlResponse;
 
-use crate::{common::utils::redirect_response::RedirectResponse, service::short_url};
+use crate::{
+    common::{
+        meta::{self, http::HttpResponse as MetaHttpResponse},
+        utils::redirect_response::RedirectResponseBuilder,
+    },
+    service::short_url,
+};
 
 /// Shorten a URL
 #[utoipa::path(
@@ -47,9 +54,13 @@ use crate::{common::utils::redirect_response::RedirectResponse, service::short_u
     tag = "Short Url"
 )]
 #[post("/{org_id}/short")]
-pub async fn shorten(_org_id: web::Path<String>, body: web::Bytes) -> Result<HttpResponse, Error> {
-    let req: config::meta::short_url::ShortenUrlRequest = serde_json::from_slice(&body)?;
-    match short_url::shorten(&req.original_url).await {
+pub async fn shorten(org_id: web::Path<String>, body: web::Bytes) -> Result<HttpResponse, Error> {
+    let req: config::meta::short_url::ShortenUrlRequest = match serde_json::from_slice(&body) {
+        Ok(v) => v,
+        Err(e) => return Ok(MetaHttpResponse::bad_request(e)),
+    };
+
+    match short_url::shorten(&org_id, &req.original_url).await {
         Ok(short_url) => {
             let response = ShortenUrlResponse {
                 short_url: short_url.clone(),
@@ -59,7 +70,12 @@ pub async fn shorten(_org_id: web::Path<String>, body: web::Bytes) -> Result<Htt
         }
         Err(e) => {
             log::error!("Failed to shorten URL: {:?}", e);
-            Ok(HttpResponse::InternalServerError().finish())
+            Ok(
+                HttpResponse::InternalServerError().json(meta::http::HttpResponse::error(
+                    StatusCode::INTERNAL_SERVER_ERROR.into(),
+                    e.to_string(),
+                )),
+            )
         }
     }
 }
@@ -79,22 +95,24 @@ pub async fn shorten(_org_id: web::Path<String>, body: web::Bytes) -> Result<Htt
     ),
     tag = "Short Url"
 )]
-#[get("/{short_id}")]
+#[get("/{org_id}/short/{short_id}")]
 pub async fn retrieve(
     req: HttpRequest,
-    short_id: web::Path<String>,
+    path: web::Path<(String, String)>,
 ) -> Result<HttpResponse, Error> {
     log::info!(
         "short_url::retrieve handler called for path: {}",
         req.path()
     );
-    let short_id = short_id.into_inner();
+    let (_org_id, short_id) = path.into_inner();
     let original_url = short_url::retrieve(&short_id).await;
 
     if let Some(url) = original_url {
-        let redirect_response = RedirectResponse::new(&url);
-        Ok(redirect_response.redirect())
+        let redirect_http = RedirectResponseBuilder::new(&url).build().redirect_http();
+        Ok(redirect_http)
     } else {
-        Ok(HttpResponse::NotFound().body("Short URL not found"))
+        let redirect = RedirectResponseBuilder::default().build();
+        log::error!("Short URL not found, {}", &redirect);
+        Ok(redirect.redirect_http())
     }
 }

src/handler/http/router/mod.rs

@@ -226,15 +226,6 @@ pub fn get_basic_routes(cfg: &mut web::ServiceConfig) {
             .service(status::flush_node),
     );
 
-    cfg.service(
-        web::scope("/short")
-            .wrap(HttpAuthentication::with_fn(
-                super::auth::validator::validate_short_or_redirect,
-            ))
-            .wrap(cors.clone())
-            .service(short_url::retrieve),
-    );
-
     if get_config().common.swagger_enabled {
         cfg.service(
             SwaggerUi::new("/swagger/{_:.*}")
@@ -497,7 +488,8 @@ pub fn get_service_routes(cfg: &mut web::ServiceConfig) {
             .service(search::multi_streams::_search_partition_multi)
             .service(search::multi_streams::around_multi)
             .service(stream::delete_stream_cache)
-            .service(short_url::shorten),
+            .service(short_url::shorten)
+            .service(short_url::retrieve),
     );
 }
 

src/service/alerts/alert.rs

@@ -804,7 +804,7 @@ async fn process_dest_template(
     };
 
     // Shorten the alert url
-    let alert_url = match short_url::shorten(&alert_url).await {
+    let alert_url = match short_url::shorten(&alert.org_id, &alert_url).await {
         Ok(short_url) => short_url,
         Err(e) => {
             log::error!("Error shortening alert url: {e}");

src/service/dashboards/reports.rs

@@ -618,7 +618,7 @@ async fn generate_report(
     log::debug!("done with headless browser");
 
     // convert to short_url
-    let email_dashb_url = match short_url::shorten(&email_dashb_url).await {
+    let email_dashb_url = match short_url::shorten(org_id, &email_dashb_url).await {
         Ok(short_url) => short_url,
         Err(e) => {
             log::error!("Error shortening email dashboard url: {e}");

src/service/short_url.rs

@@ -29,15 +29,25 @@ pub fn get_base_url() -> String {
     format!("{}{}", config.common.web_url, config.common.base_uri)
 }
 
-async fn store_short_url(short_id: &str, original_url: &str) -> Result<String, anyhow::Error> {
-    let entry = ShortUrlRecord::new(short_id, original_url);
-    db::short_url::set(short_id, entry).await?;
-    Ok(format!(
-        "{}{}{}",
+fn construct_short_url(org_id: &str, short_id: &str) -> String {
+    format!(
+        "{}/{}/{}{}{}",
         get_base_url(),
+        "api",
+        org_id,
         SHORT_URL_WEB_PATH,
         short_id
-    ))
+    )
+}
+
+async fn store_short_url(
+    org_id: &str,
+    short_id: &str,
+    original_url: &str,
+) -> Result<String, anyhow::Error> {
+    let entry = ShortUrlRecord::new(short_id, original_url);
+    db::short_url::set(short_id, entry).await?;
+    Ok(construct_short_url(org_id, short_id))
 }
 
 fn generate_short_id(original_url: &str, timestamp: Option<i64>) -> String {
@@ -51,21 +61,16 @@ fn generate_short_id(original_url: &str, timestamp: Option<i64>) -> String {
 }
 
 /// Shortens the given original URL and stores it in the database
-pub async fn shorten(original_url: &str) -> Result<String, anyhow::Error> {
+pub async fn shorten(org_id: &str, original_url: &str) -> Result<String, anyhow::Error> {
     let mut short_id = generate_short_id(original_url, None);
 
     if let Ok(existing_url) = db::short_url::get(&short_id).await {
         if existing_url == original_url {
-            return Ok(format!(
-                "{}{}{}",
-                get_base_url(),
-                SHORT_URL_WEB_PATH,
-                short_id
-            ));
+            return Ok(construct_short_url(org_id, &short_id));
         }
     }
 
-    let result = store_short_url(&short_id, original_url).await;
+    let result = store_short_url(org_id, &short_id, original_url).await;
     match result {
         Ok(url) => Ok(url),
         Err(e) => {
@@ -74,7 +79,7 @@ pub async fn shorten(original_url: &str) -> Result<String, anyhow::Error> {
                     Error::DbError(DbError::UniqueViolation) => {
                         let timestamp = Utc::now().timestamp();
                         short_id = generate_short_id(original_url, Some(timestamp));
-                        store_short_url(&short_id, original_url).await
+                        store_short_url(org_id, &short_id, original_url).await
                     }
                     _ => Err(e),
                 }
@@ -90,27 +95,27 @@ pub async fn retrieve(short_id: &str) -> Option<String> {
     db::short_url::get(short_id).await.ok()
 }
 
-/// Extracts the short ID from the shortened URL
-pub fn get_short_id_from_url(short_url: &str) -> Option<String> {
-    let prefix = format!("{}{}", get_base_url(), SHORT_URL_WEB_PATH);
-    short_url.strip_prefix(&prefix).map(|s| s.to_string())
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;
 
+    /// Extracts the short ID from the shortened URL
+    fn get_short_id_from_url(org_id: &str, short_url: &str) -> Option<String> {
+        let prefix = format!("{}api/{}{}", get_base_url(), org_id, SHORT_URL_WEB_PATH);
+        short_url.strip_prefix(&prefix).map(|s| s.to_string())
+    }
+
     #[tokio::test]
     #[ignore]
     async fn test_shorten_and_retrieve() {
         let original_url = "https://www.example.com/some/long/url";
-        let short_url = shorten(original_url).await.unwrap();
-        let short_id = get_short_id_from_url(&short_url).unwrap();
+        let short_url = shorten("default", original_url).await.unwrap();
+        let short_id = get_short_id_from_url("default", &short_url).unwrap();
 
         let retrieved_url = retrieve(&short_id).await.expect("Failed to retrieve URL");
         assert_eq!(retrieved_url, original_url);
 
-        let short_id = get_short_id_from_url(&short_url).unwrap();
+        let short_id = get_short_id_from_url("default", &short_url).unwrap();
         assert_eq!(short_id.len(), 16);
     }
 
@@ -126,8 +131,8 @@ mod tests {
     async fn test_unique_original_urls() {
         let original_url = "https://www.example.com/some/long/url";
 
-        let short_url1 = shorten(original_url).await.unwrap();
-        let short_url2 = shorten(original_url).await.unwrap();
+        let short_url1 = shorten("default", original_url).await.unwrap();
+        let short_url2 = shorten("default", original_url).await.unwrap();
 
         // Should return the same short_id
         assert_eq!(short_url1, short_url2);

web/src/router/index.ts

@@ -71,16 +71,18 @@ export default function (store: any) {
         to.path != "/web/cb" &&
         sessionUserInfo === null
       ) {
-        if (to.path !== "/logout" &&
+        if (
+          to.path !== "/logout" &&
           to.path !== "/cb" &&
-          to.path != "/web/cb") {
+          to.path != "/web/cb"
+        ) {
           // if query params contains redirect_url, store that URL in session storage
           // else store the current URL in session storage
           // this conditions added specifically for short URL feature where user will be redirected to backend API endpoint
           // if user is not logged in, then user will be redirected to login page and after successful login, user will be redirected to the short URL
           if (Object.hasOwn(to.query, "short_url")) {
             window.sessionStorage.setItem("redirectURI", to.query.short_url);
-          } 
+          }
           else {
             window.sessionStorage.setItem("redirectURI", window.location.href);
           }