Added documentation and comments.

Author: adsnaider

src/controllers/user/session.rs

@@ -14,8 +14,10 @@ use crate::util::token::NewSecureToken;
 use crate::util::token::SecureToken;
 use crate::Env;
 
+/// Name of the cookie used for session-based authentication.
 pub const SESSION_COOKIE_NAME: &str = "crates_auth";
 
+/// Creates a session cookie with the given token.
 pub fn session_cookie(token: &NewSecureToken, secure: bool) -> Cookie<'static> {
     Cookie::build(SESSION_COOKIE_NAME, token.plaintext().to_string())
         .http_only(true)
@@ -112,24 +114,24 @@ pub fn authorize(req: &mut dyn RequestExt) -> EndpointResult {
     let ghuser = req.app().github.current_user(token)?;
     let user = save_user_to_database(&ghuser, token.secret(), &req.app().emails, &*req.db_conn()?)?;
 
+    // Setup a persistent session for the newly logged in user.
     let user_agent = req
         .headers()
         .get(header::USER_AGENT)
         .and_then(|value| value.to_str().ok())
         .map(|value| value.to_string())
         .unwrap_or_default();
 
-    // Setup a session for the newly logged in user.
     let token = SecureToken::generate(crate::util::token::SecureTokenKind::Session);
-    let session = PersistentSession::create(user.id, &token, req.remote_addr().ip(), &user_agent);
-    session.insert(&*req.db_conn()?)?;
+    PersistentSession::create(user.id, &token, req.remote_addr().ip(), &user_agent)
+        .insert(&*req.db_conn()?)?;
 
-    // Setup session cookie.
+    // Setup persistent session cookie.
     let secure = req.app().config.env() == Env::Production;
     req.cookies_mut().add(session_cookie(&token, secure));
 
-    // Log in by setting a cookie and the middleware authentication
-    // TODO(adsnaider): Remove.
+    // TODO(adsnaider): Remove as part of https://github.com/rust-lang/crates.io/issues/2630.
+    // Log in by setting a cookie and the middleware authentication.
     req.session_mut()
         .insert("user_id".to_string(), user.id.to_string());
 
@@ -168,8 +170,10 @@ fn save_user_to_database(
 
 /// Handles the `DELETE /api/private/session` route.
 pub fn logout(req: &mut dyn RequestExt) -> EndpointResult {
-    // TODO(adsnaider): Remove this.
+    // TODO(adsnaider): Remove as part of https://github.com/rust-lang/crates.io/issues/2630.
     req.session_mut().remove(&"user_id".to_string());
+
+    // Remove persistent session from database.
     if let Some(session_token) = req
         .cookies()
         .get(SESSION_COOKIE_NAME)
@@ -178,12 +182,8 @@ pub fn logout(req: &mut dyn RequestExt) -> EndpointResult {
         req.cookies_mut().remove(Cookie::named(SESSION_COOKIE_NAME));
 
         if let Ok(conn) = req.db_conn() {
-            match PersistentSession::revoke_from_token(&conn, &session_token) {
-                Ok(0) => {}
-                Ok(1) => {}
-                Ok(_count) => {}
-                Err(_e) => {}
-            }
+            // TODO(adsnaider): Maybe log errors somehow?
+            let _ = PersistentSession::revoke_from_token(&conn, &session_token);
         }
     }
     Ok(req.json(&true))

src/controllers/util.rs

@@ -68,7 +68,8 @@ fn verify_origin(req: &dyn RequestExt) -> AppResult<()> {
 fn authenticate_user(req: &dyn RequestExt) -> AppResult<AuthenticatedUser> {
     let conn = req.db_conn()?;
 
-    // TODO(adsnaider): Remove this.
+    // TODO(adsnaider): Remove as part of https://github.com/rust-lang/crates.io/issues/2630.
+    // Log in with the session id.
     let session = req.session();
     let user_id_from_session = session.get("user_id").and_then(|s| s.parse::<i32>().ok());
 
@@ -82,6 +83,7 @@ fn authenticate_user(req: &dyn RequestExt) -> AppResult<AuthenticatedUser> {
         });
     }
 
+    // Log in with persistent session token.
     if let Some(session_token) = req
         .cookies()
         .get(SESSION_COOKIE_NAME)

src/models/persistent_session.rs

@@ -8,19 +8,34 @@ use crate::schema::persistent_sessions;
 use crate::util::token::SecureToken;
 use crate::util::token::SecureTokenKind;
 
+/// A persistent session model (as is stored in the database).
+///
+/// The sessions table works by maintaining a `hashed_token`. In order for a user to securely
+/// demonstrate authenticity, the user provides us with the token (stored as part of a cookie). We
+/// hash the token and search in the database for matches. If we find one and the token hasn't
+/// been revoked, then we update the session with the latest values and authorize the user.
 #[derive(Clone, Debug, PartialEq, Eq, Identifiable, Queryable)]
 #[table_name = "persistent_sessions"]
 pub struct PersistentSession {
+    /// The id of this session.
     pub id: i32,
+    /// The user id associated with this session.
     pub user_id: i32,
+    /// The token (hashed) that identifies the session.
     pub hashed_token: SecureToken,
+    /// Datetime the session was created.
     pub created_at: NaiveDateTime,
+    /// Datetime the session was last used.
     pub last_used_at: NaiveDateTime,
+    /// Whether the session is revoked.
     pub revoked: bool,
+    /// Last IP address that used the session.
     pub last_ip_address: IpNetwork,
+    /// Last user agent that used the session.
     pub last_user_agent: String,
 }
 
+/// Session-related errors.
 #[derive(Error, Debug, PartialEq)]
 pub enum SessionError {
     #[error("token prefix doesn't match session tokens")]
@@ -30,6 +45,7 @@ pub enum SessionError {
 }
 
 impl PersistentSession {
+    /// Creates a `NewPersistentSession` that can be inserted into the database.
     pub fn create<'a, 'b>(
         user_id: i32,
         token: &'a SecureToken,
@@ -44,6 +60,13 @@ impl PersistentSession {
         }
     }
 
+    /// Finds an unrevoked session that matches `token` from the database and returns it.
+    ///
+    /// # Returns
+    ///
+    /// * `Ok(Some(...))` if a session matches the `token`.
+    /// * `Ok(None)` if no session matches the `token`.
+    /// * `Err(...)` for other errors such as invalid tokens or diesel errors.
     pub fn find_from_token_and_update(
         conn: &PgConnection,
         token: &str,
@@ -56,6 +79,8 @@ impl PersistentSession {
             .filter(persistent_sessions::revoked.eq(false))
             .filter(persistent_sessions::hashed_token.eq(hashed_token));
 
+        // TODO: Do we want to check if the user agent or IP address don't match? What about the
+        // created_at/last_user_at times, do we want to expire the tokens?
         conn.transaction(|| {
             diesel::update(sessions.clone())
                 .set((
@@ -70,6 +95,12 @@ impl PersistentSession {
         .map_err(SessionError::DieselError)
     }
 
+    /// Revokes the `token` in the database.
+    ///
+    /// # Returns
+    ///
+    /// The number of sessions that were revoked or an error if the `token` isn't valid or there
+    /// was an issue with the database connection.
     pub fn revoke_from_token(conn: &PgConnection, token: &str) -> Result<usize, SessionError> {
         let hashed_token = SecureToken::parse(SecureTokenKind::Session, token)
             .ok_or(SessionError::InvalidSessionToken)?;
@@ -84,6 +115,7 @@ impl PersistentSession {
     }
 }
 
+/// A new, insertable persistent session.
 #[derive(Clone, Debug, PartialEq, Eq, Insertable)]
 #[table_name = "persistent_sessions"]
 pub struct NewPersistentSession<'a, 'b> {
@@ -94,6 +126,7 @@ pub struct NewPersistentSession<'a, 'b> {
 }
 
 impl NewPersistentSession<'_, '_> {
+    /// Inserts the session into the database.
     pub fn insert(self, conn: &PgConnection) -> Result<PersistentSession, diesel::result::Error> {
         diesel::insert_into(persistent_sessions::table)
             .values(self)