less memory allocations during Session cache handling (#607)

sebadob · Nov 8, 2024 · fb86b9d · fb86b9d
1 parent bbf0cea
commit fb86b9d
Show file tree

Hide file tree

Showing 3 changed files with 44 additions and 76 deletions.
diff --git a/src/api/src/oidc.rs b/src/api/src/oidc.rs
@@ -15,7 +15,7 @@ use rauthy_api_types::sessions::SessionState;
 use rauthy_common::constants::{
     APPLICATION_JSON, AUTH_HEADERS_ENABLE, AUTH_HEADER_EMAIL, AUTH_HEADER_EMAIL_VERIFIED,
     AUTH_HEADER_FAMILY_NAME, AUTH_HEADER_GIVEN_NAME, AUTH_HEADER_GROUPS, AUTH_HEADER_MFA,
-    AUTH_HEADER_ROLES, AUTH_HEADER_USER, COOKIE_MFA, COOKIE_SESSION_FED_CM,
+    AUTH_HEADER_ROLES, AUTH_HEADER_USER, COOKIE_MFA, COOKIE_SESSION, COOKIE_SESSION_FED_CM,
     DEVICE_GRANT_CODE_LIFETIME, DEVICE_GRANT_POLL_INTERVAL, DEVICE_GRANT_RATE_LIMIT,
     EXPERIMENTAL_FED_CM_ENABLE, GRANT_TYPE_DEVICE_CODE, HEADER_HTML, HEADER_RETRY_NOT_BEFORE,
     OPEN_USER_REG, SESSION_LIFETIME,
@@ -530,7 +530,7 @@ pub async fn post_device_auth(
     HttpResponse::Ok().json(resp)
 }
 
-/// POST for vertifying an OAuth 2.0 Device Authorization Grant flow
+/// POST for verifying an OAuth 2.0 Device Authorization Grant flow
 #[utoipa::path(
     post,
     path = "/oidc/device/verify",
@@ -580,11 +580,11 @@ pub async fn post_device_verify(
     }
 }
 
-// Logout HTML page
-//
-// Returns an HTML page which can be used for logging the user out. Invalidates the session and deletes
-// all possibly existing refresh tokens from the database. Does an automatic logout if the
-// `id_token_hint` is given.
+/// Logout HTML page
+///
+/// Returns an HTML page which can be used for logging the user out. Invalidates the session and deletes
+/// all possibly existing refresh tokens from the database. Does an automatic logout if the
+/// `id_token_hint` is given.
 #[utoipa::path(
     get,
     path = "/oidc/logout",
@@ -623,9 +623,9 @@ pub async fn get_logout(
         }
     };
 
-    return HttpResponse::build(StatusCode::OK)
+    HttpResponse::build(StatusCode::OK)
         .append_header(HEADER_HTML)
-        .body(body);
+        .body(body)
 }
 
 /// Send the logout confirmation
@@ -647,14 +647,16 @@ pub async fn post_logout(
     req_data: actix_web_validator::Query<LogoutRequest>,
     principal: ReqPrincipal,
 ) -> Result<HttpResponse, ErrorResponse> {
-    let mut session = principal.get_session()?.clone();
+    let session = principal.get_session()?.clone();
     let cookie_fed_cm = ApiCookie::build_with_same_site(
         COOKIE_SESSION_FED_CM,
         Cow::from(&session.id),
         0,
         SameSite::None,
     );
-    let cookie = session.invalidate().await?;
+    let sid = session.id.clone();
+    let cookie = ApiCookie::build(COOKIE_SESSION, &sid, 0);
+    session.invalidate().await?;
 
     if req_data.post_logout_redirect_uri.is_some() {
         let state = if req_data.state.is_some() {
@@ -674,10 +676,10 @@ pub async fn post_logout(
             .finish());
     }
 
-    return Ok(HttpResponse::build(StatusCode::OK)
+    Ok(HttpResponse::build(StatusCode::OK)
         .cookie(cookie)
         .cookie(cookie_fed_cm)
-        .finish());
+        .finish())
 }
 
 /// Rotate JWKs
@@ -951,32 +953,6 @@ pub async fn post_token_introspect(
     }
 }
 
-// // TODO remove?
-// /// DEPRECATED
-// ///
-// /// This is an older endpoint for refreshing tokens manually. This is not being used anymore an will
-// /// be removed soon in favor of the `refresh_token` flow on the [token](post_token) endpoint.
-// #[utoipa::path(
-//     post,
-//     path = "/oidc/token/refresh",
-//     tag = "deprecated",
-//     request_body = RefreshTokenRequest,
-//     responses(
-//         (status = 200, description = "Ok", body = TokenSet),
-//         (status = 401, description = "Unauthorized", body = ErrorResponse),
-//         (status = 404, description = "NotFound", body = ErrorResponse),
-//     ),
-// )]
-// #[post("/oidc/token/refresh")]
-// pub async fn post_refresh_token(
-//     req_data: actix_web_validator::Json<RefreshTokenRequest>,
-//     data: web::Data<AppState>,
-// ) -> Result<HttpResponse, ErrorResponse> {
-//     oidc::validate_refresh_token(None, &req_data.refresh_token, &data)
-//         .await
-//         .map(|token_set| HttpResponse::Ok().json(token_set))
-// }
-
 /// DEPRECATED
 ///
 /// This is an older endpoint for validating tokens manually. This is not being used anymore an will
@@ -1129,7 +1105,7 @@ pub async fn get_well_known(data: web::Data<AppState>) -> Result<HttpResponse, E
         .insert_header((CONTENT_TYPE, APPLICATION_JSON))
         .insert_header((
             header::ACCESS_CONTROL_ALLOW_ORIGIN,
-            HeaderValue::from_str("*").unwrap(),
+            HeaderValue::from_static("*"),
         ))
         .body(wk))
 }
diff --git a/src/common/src/constants.rs b/src/common/src/constants.rs
@@ -78,7 +78,6 @@ pub const IDX_MFA_LOGIN_REQ: &str = "mfa_login_req_";
 pub const IDX_PASSWORD_RULES: &str = "password_rules_";
 pub const IDX_ROLES: &str = "roles_";
 pub const IDX_SCOPES: &str = "scopes_";
-pub const IDX_SESSION: &str = "session_";
 pub const IDX_SESSIONS: &str = "sessions";
 pub const IDX_USERS: &str = "users_";
 pub const IDX_USER_COUNT: &str = "users_count_total";

diff --git a/src/models/src/entity/sessions.rs b/src/models/src/entity/sessions.rs
@@ -2,15 +2,14 @@ use crate::api_cookie::ApiCookie;
 use crate::database::{Cache, DB};
 use crate::entity::continuation_token::ContinuationToken;
 use crate::entity::users::User;
-use actix_web::cookie::{time, Cookie, SameSite};
+use actix_web::cookie::{time, SameSite};
 use actix_web::http::header::{HeaderName, HeaderValue};
 use actix_web::{cookie, web, HttpRequest};
 use chrono::Utc;
 use hiqlite::{params, Param};
 use rauthy_api_types::generic::SearchParamsIdx;
 use rauthy_common::constants::{
-    CACHE_TTL_SESSION, COOKIE_SESSION, COOKIE_SESSION_FED_CM, CSRF_HEADER, IDX_SESSION,
-    SESSION_LIFETIME_FED_CM,
+    CACHE_TTL_SESSION, COOKIE_SESSION, COOKIE_SESSION_FED_CM, CSRF_HEADER, SESSION_LIFETIME_FED_CM,
 };
 use rauthy_common::is_hiqlite;
 use rauthy_common::utils::get_rand;
@@ -100,7 +99,7 @@ impl SessionState {
 
 // CRUD
 impl Session {
-    pub async fn delete(&self) -> Result<(), ErrorResponse> {
+    pub async fn delete(self) -> Result<(), ErrorResponse> {
         if is_hiqlite() {
             DB::client()
                 .execute("DELETE FROM sessions WHERE id = $1", params!(&self.id))
@@ -111,9 +110,7 @@ impl Session {
                 .await?;
         }
 
-        DB::client()
-            .delete(Cache::Session, Session::cache_idx(&self.id))
-            .await?;
+        DB::client().delete(Cache::Session, self.id).await?;
 
         Ok(())
     }
@@ -149,20 +146,17 @@ impl Session {
 
         let client = DB::client();
         for id in sids {
-            client
-                .delete(Cache::Session, Session::cache_idx(&id))
-                .await?;
+            client.delete(Cache::Session, id).await?;
         }
 
         Ok(())
     }
 
     // Returns a session by id
     pub async fn find(id: String) -> Result<Self, ErrorResponse> {
-        let idx = Session::cache_idx(&id);
         let client = DB::client();
 
-        if let Some(slf) = client.get(Cache::Session, &idx).await? {
+        if let Some(slf) = client.get(Cache::Session, id.clone()).await? {
             return Ok(slf);
         }
 
@@ -184,7 +178,7 @@ impl Session {
         };
 
         client
-            .put(Cache::Session, idx, &slf, CACHE_TTL_SESSION)
+            .put(Cache::Session, slf.id.clone(), &slf, CACHE_TTL_SESSION)
             .await?;
 
         Ok(slf)
@@ -485,12 +479,7 @@ remote_ip = $10"#,
         }
 
         DB::client()
-            .put(
-                Cache::Session,
-                Session::cache_idx(&self.id),
-                self,
-                CACHE_TTL_SESSION,
-            )
+            .put(Cache::Session, self.id.clone(), self, CACHE_TTL_SESSION)
             .await?;
 
         Ok(())
@@ -593,11 +582,6 @@ impl Session {
         }
     }
 
-    #[inline(always)]
-    fn cache_idx(id: &String) -> String {
-        format!("{}{}", IDX_SESSION, id)
-    }
-
     /// exp_in will be the time in seconds when the session will expire
     pub fn try_new(
         user: &User,
@@ -692,22 +676,31 @@ impl Session {
         )
     }
 
-    pub async fn invalidate(&mut self) -> Result<Cookie, ErrorResponse> {
-        let idx = Session::cache_idx(&self.id);
-
-        self.exp = OffsetDateTime::now_utc().unix_timestamp();
-        self.state = SessionState::LoggedOut.as_str().to_string();
+    pub async fn invalidate(self) -> Result<(), ErrorResponse> {
+        let now = Utc::now().timestamp() - 1;
+        let state = SessionState::LoggedOut.as_str().to_string();
 
-        sqlx::query("update sessions set exp = $1, state = $2 where id = $3")
-            .bind(self.exp)
-            .bind(self.state.as_str())
-            .bind(&self.id)
+        if is_hiqlite() {
+            DB::client()
+                .execute(
+                    "UPDATE sessions SET exp = $1, state = $2 WHERE id = $3",
+                    params!(now, state, self.id.clone()),
+                )
+                .await?;
+        } else {
+            sqlx::query!(
+                "UPDATE sessions SET exp = $1, state = $2 WHERE id = $3",
+                now,
+                state,
+                &self.id
+            )
             .execute(DB::conn())
             .await?;
+        }
 
-        DB::client().delete(Cache::Session, idx).await?;
+        DB::client().delete(Cache::Session, self.id).await?;
 
-        Ok(ApiCookie::build(COOKIE_SESSION, &self.id, 0))
+        Ok(())
     }
 
     #[inline(always)]