Skip to content

Commit f3bd37a

Browse files
david-crespodavid-crespo
committed
eliza feedback
1 parent 6f16cfe commit f3bd37a

File tree

12 files changed

+82
-83
lines changed

12 files changed

+82
-83
lines changed

‎nexus/auth/src/authn/external/session_cookie.rs‎

Lines changed: 12 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -13,15 +13,15 @@ use chrono::{DateTime, Duration, Utc};
1313
use dropshot::HttpError;
1414
use http::HeaderValue;
1515
use nexus_types::authn::cookies::parse_cookies;
16-
use omicron_uuid_kinds::{ConsoleSessionKind, TypedUuid};
16+
use omicron_uuid_kinds::ConsoleSessionUuid;
1717
use slog::debug;
1818
use uuid::Uuid;
1919

2020
// many parts of the implementation will reference this OWASP guide
2121
// https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html
2222

2323
pub trait Session {
24-
fn id(&self) -> TypedUuid<ConsoleSessionKind>;
24+
fn id(&self) -> ConsoleSessionUuid;
2525
fn silo_user_id(&self) -> Uuid;
2626
fn silo_id(&self) -> Uuid;
2727
fn time_last_used(&self) -> DateTime<Utc>;
@@ -41,14 +41,11 @@ pub trait SessionStore {
4141
/// Extend session by updating time_last_used to now
4242
async fn session_update_last_used(
4343
&self,
44-
id: TypedUuid<ConsoleSessionKind>,
44+
id: ConsoleSessionUuid,
4545
) -> Option<Self::SessionModel>;
4646

4747
/// Mark session expired
48-
async fn session_expire(
49-
&self,
50-
id: TypedUuid<ConsoleSessionKind>,
51-
) -> Option<()>;
48+
async fn session_expire(&self, id: ConsoleSessionUuid) -> Option<()>;
5249

5350
/// Maximum time session can remain idle before expiring
5451
fn session_idle_timeout(&self) -> Duration;
@@ -204,8 +201,7 @@ mod test {
204201
use async_trait::async_trait;
205202
use chrono::{DateTime, Duration, Utc};
206203
use http;
207-
use omicron_uuid_kinds::ConsoleSessionKind;
208-
use omicron_uuid_kinds::TypedUuid;
204+
use omicron_uuid_kinds::ConsoleSessionUuid;
209205
use slog;
210206
use std::sync::Mutex;
211207
use uuid::Uuid;
@@ -218,7 +214,7 @@ mod test {
218214

219215
#[derive(Clone)]
220216
struct FakeSession {
221-
id: TypedUuid<ConsoleSessionKind>,
217+
id: ConsoleSessionUuid,
222218
token: String,
223219
silo_user_id: Uuid,
224220
silo_id: Uuid,
@@ -227,7 +223,7 @@ mod test {
227223
}
228224

229225
impl Session for FakeSession {
230-
fn id(&self) -> TypedUuid<ConsoleSessionKind> {
226+
fn id(&self) -> ConsoleSessionUuid {
231227
self.id
232228
}
233229
fn silo_user_id(&self) -> Uuid {
@@ -262,7 +258,7 @@ mod test {
262258

263259
async fn session_update_last_used(
264260
&self,
265-
id: TypedUuid<ConsoleSessionKind>,
261+
id: ConsoleSessionUuid,
266262
) -> Option<Self::SessionModel> {
267263
let mut sessions = self.sessions.lock().unwrap();
268264
if let Some(pos) = sessions.iter().position(|s| s.id == id) {
@@ -277,10 +273,7 @@ mod test {
277273
}
278274
}
279275

280-
async fn session_expire(
281-
&self,
282-
id: TypedUuid<ConsoleSessionKind>,
283-
) -> Option<()> {
276+
async fn session_expire(&self, id: ConsoleSessionUuid) -> Option<()> {
284277
let mut sessions = self.sessions.lock().unwrap();
285278
sessions.retain(|s| s.id != id);
286279
Some(())
@@ -336,7 +329,7 @@ mod test {
336329
async fn test_expired_cookie_idle() {
337330
let context = TestServerContext {
338331
sessions: Mutex::new(vec![FakeSession {
339-
id: TypedUuid::new_v4(),
332+
id: ConsoleSessionUuid::new_v4(),
340333
token: "abc".to_string(),
341334
silo_user_id: Uuid::new_v4(),
342335
silo_id: Uuid::new_v4(),
@@ -362,7 +355,7 @@ mod test {
362355
async fn test_expired_cookie_absolute() {
363356
let context = TestServerContext {
364357
sessions: Mutex::new(vec![FakeSession {
365-
id: TypedUuid::new_v4(),
358+
id: ConsoleSessionUuid::new_v4(),
366359
token: "abc".to_string(),
367360
silo_user_id: Uuid::new_v4(),
368361
silo_id: Uuid::new_v4(),
@@ -389,7 +382,7 @@ mod test {
389382
let time_last_used = Utc::now() - Duration::seconds(5);
390383
let context = TestServerContext {
391384
sessions: Mutex::new(vec![FakeSession {
392-
id: TypedUuid::new_v4(),
385+
id: ConsoleSessionUuid::new_v4(),
393386
token: "abc".to_string(),
394387
silo_user_id: Uuid::new_v4(),
395388
silo_id: Uuid::new_v4(),

‎nexus/auth/src/context.rs‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,7 @@ use crate::authz::AuthorizedResource;
1111
use crate::storage::Storage;
1212
use chrono::{DateTime, Utc};
1313
use omicron_common::api::external::Error;
14-
use omicron_uuid_kinds::ConsoleSessionKind;
15-
use omicron_uuid_kinds::TypedUuid;
14+
use omicron_uuid_kinds::ConsoleSessionUuid;
1615
use slog::debug;
1716
use slog::o;
1817
use slog::trace;
@@ -354,7 +353,7 @@ impl OpContext {
354353
}
355354

356355
impl Session for ConsoleSessionWithSiloId {
357-
fn id(&self) -> TypedUuid<ConsoleSessionKind> {
356+
fn id(&self) -> ConsoleSessionUuid {
358357
self.console_session.id()
359358
}
360359

‎nexus/db-lookup/src/lookup.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ use omicron_common::api::external::{LookupResult, LookupType, ResourceType};
2929
use omicron_uuid_kinds::AccessTokenKind;
3030
use omicron_uuid_kinds::AlertReceiverUuid;
3131
use omicron_uuid_kinds::AlertUuid;
32-
use omicron_uuid_kinds::ConsoleSessionKind;
32+
use omicron_uuid_kinds::ConsoleSessionUuid;
3333
use omicron_uuid_kinds::PhysicalDiskUuid;
3434
use omicron_uuid_kinds::SupportBundleUuid;
3535
use omicron_uuid_kinds::TufArtifactKind;
@@ -204,7 +204,7 @@ impl<'a> LookupPath<'a> {
204204
/// Select a resource of type ConsoleSession, identified by its `id`
205205
pub fn console_session_id(
206206
self,
207-
id: TypedUuid<ConsoleSessionKind>,
207+
id: ConsoleSessionUuid,
208208
) -> ConsoleSession<'a> {
209209
ConsoleSession::PrimaryKey(Root { lookup_root: self }, id)
210210
}

‎nexus/db-model/src/console_session.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
use chrono::{DateTime, Utc};
66
use nexus_db_schema::schema::console_session;
77
use omicron_uuid_kinds::ConsoleSessionKind;
8-
use omicron_uuid_kinds::TypedUuid;
8+
use omicron_uuid_kinds::ConsoleSessionUuid;
99
use uuid::Uuid;
1010

1111
use crate::typed_uuid::DbTypedUuid;
@@ -26,15 +26,15 @@ impl ConsoleSession {
2626
pub fn new(token: String, silo_user_id: Uuid) -> Self {
2727
let now = Utc::now();
2828
Self {
29-
id: TypedUuid::new_v4().into(),
29+
id: ConsoleSessionUuid::new_v4().into(),
3030
token,
3131
silo_user_id,
3232
time_last_used: now,
3333
time_created: now,
3434
}
3535
}
3636

37-
pub fn id(&self) -> TypedUuid<ConsoleSessionKind> {
37+
pub fn id(&self) -> ConsoleSessionUuid {
3838
self.id.0
3939
}
4040
}

‎nexus/db-queries/src/db/datastore/console_session.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,12 @@ use omicron_common::api::external::UpdateResult;
2525
use omicron_uuid_kinds::GenericUuid;
2626

2727
impl DataStore {
28+
/// Look up session by token. The token is a kind of password, so simply
29+
/// having the token _is_ in a sense the primary authz check here.
30+
///
31+
/// We need to define this lookup function manually because `token` is not
32+
/// the primary key on the session (sessions have IDs), so we can't use the
33+
/// automatically-generated lookup methods we use for IDs or names.
2834
pub async fn session_lookup_by_token(
2935
&self,
3036
opctx: &OpContext,

‎nexus/db-queries/src/db/datastore/mod.rs‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -651,9 +651,6 @@ mod test {
651651
renewed.console_session.time_last_used > session.time_last_used
652652
);
653653

654-
// TODO: check the opctx on these changes, make sure we're using the
655-
// right thing between opctx or authn_opctx
656-
657654
// time_last_used change persists in DB
658655
let (.., fetched) = datastore
659656
.session_lookup_by_token(&opctx, token.clone())

‎nexus/src/app/session.rs‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,8 @@ use omicron_common::api::external::Error;
1818
use omicron_common::api::external::LookupResult;
1919
use omicron_common::api::external::LookupType;
2020
use omicron_common::api::external::UpdateResult;
21-
use omicron_uuid_kinds::ConsoleSessionKind;
21+
use omicron_uuid_kinds::ConsoleSessionUuid;
2222
use omicron_uuid_kinds::GenericUuid;
23-
use omicron_uuid_kinds::TypedUuid;
2423
use rand::{RngCore, SeedableRng, rngs::StdRng};
2524
use uuid::Uuid;
2625

@@ -71,7 +70,7 @@ impl super::Nexus {
7170
pub(crate) async fn session_update_last_used(
7271
&self,
7372
opctx: &OpContext,
74-
id: TypedUuid<ConsoleSessionKind>,
73+
id: ConsoleSessionUuid,
7574
) -> UpdateResult<authn::ConsoleSessionWithSiloId> {
7675
let authz_session = authz::ConsoleSession::new(
7776
authz::FLEET,
@@ -84,7 +83,7 @@ impl super::Nexus {
8483
pub(crate) async fn session_hard_delete(
8584
&self,
8685
opctx: &OpContext,
87-
id: TypedUuid<ConsoleSessionKind>,
86+
id: ConsoleSessionUuid,
8887
) -> DeleteResult {
8988
let authz_session = authz::ConsoleSession::new(
9089
authz::FLEET,

‎nexus/src/context.rs‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,8 @@ use nexus_db_queries::authn::external::session_cookie::SessionStore;
1919
use nexus_db_queries::context::{OpContext, OpKind};
2020
use nexus_db_queries::{authn, authz, db};
2121
use omicron_common::address::{AZ_PREFIX, Ipv6Subnet};
22-
use omicron_uuid_kinds::ConsoleSessionKind;
22+
use omicron_uuid_kinds::ConsoleSessionUuid;
2323
use omicron_uuid_kinds::GenericUuid;
24-
use omicron_uuid_kinds::TypedUuid;
2524
use oximeter::types::ProducerRegistry;
2625
use oximeter_instruments::http::{HttpService, LatencyTracker};
2726
use slog::Logger;
@@ -472,16 +471,13 @@ impl SessionStore for ServerContext {
472471

473472
async fn session_update_last_used(
474473
&self,
475-
id: TypedUuid<ConsoleSessionKind>,
474+
id: ConsoleSessionUuid,
476475
) -> Option<Self::SessionModel> {
477476
let opctx = self.nexus.opctx_external_authn();
478477
self.nexus.session_update_last_used(&opctx, id).await.ok()
479478
}
480479

481-
async fn session_expire(
482-
&self,
483-
id: TypedUuid<ConsoleSessionKind>,
484-
) -> Option<()> {
480+
async fn session_expire(&self, id: ConsoleSessionUuid) -> Option<()> {
485481
let opctx = self.nexus.opctx_external_authn();
486482
self.nexus.session_hard_delete(opctx, id).await.ok()
487483
}

‎nexus/tests/integration_tests/authn_http.rs‎

Lines changed: 9 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,7 @@ use nexus_db_queries::authn::external::spoof;
2727
use nexus_db_queries::authn::external::spoof::HttpAuthnSpoof;
2828
use nexus_db_queries::authn::external::spoof::SPOOF_SCHEME_NAME;
2929
use nexus_types::silo::DEFAULT_SILO_ID;
30-
use omicron_uuid_kinds::ConsoleSessionKind;
31-
use omicron_uuid_kinds::TypedUuid;
30+
use omicron_uuid_kinds::ConsoleSessionUuid;
3231
use std::sync::Mutex;
3332
use uuid::Uuid;
3433

@@ -106,23 +105,23 @@ async fn test_authn_session_cookie() {
106105
Box<dyn HttpAuthnScheme<WhoamiServerState> + 'static>,
107106
> = vec![Box::new(session_cookie::HttpAuthnSessionCookie)];
108107
let valid_session = FakeSession {
109-
id: TypedUuid::new_v4(),
108+
id: ConsoleSessionUuid::new_v4(),
110109
token: "valid".to_string(),
111110
silo_user_id: Uuid::new_v4(),
112111
silo_id: Uuid::new_v4(),
113112
time_last_used: Utc::now() - Duration::seconds(5),
114113
time_created: Utc::now() - Duration::seconds(5),
115114
};
116115
let idle_expired_session = FakeSession {
117-
id: TypedUuid::new_v4(),
116+
id: ConsoleSessionUuid::new_v4(),
118117
token: "idle_expired".to_string(),
119118
silo_user_id: Uuid::new_v4(),
120119
silo_id: Uuid::new_v4(),
121120
time_last_used: Utc::now() - Duration::hours(2),
122121
time_created: Utc::now() - Duration::hours(3),
123122
};
124123
let abs_expired_session = FakeSession {
125-
id: TypedUuid::new_v4(),
124+
id: ConsoleSessionUuid::new_v4(),
126125
token: "abs_expired".to_string(),
127126
silo_user_id: Uuid::new_v4(),
128127
silo_id: Uuid::new_v4(),
@@ -347,7 +346,7 @@ impl SiloUserSilo for WhoamiServerState {
347346

348347
#[derive(Clone)]
349348
struct FakeSession {
350-
id: TypedUuid<ConsoleSessionKind>,
349+
id: ConsoleSessionUuid,
351350
token: String,
352351
silo_user_id: Uuid,
353352
silo_id: Uuid,
@@ -356,7 +355,7 @@ struct FakeSession {
356355
}
357356

358357
impl session_cookie::Session for FakeSession {
359-
fn id(&self) -> TypedUuid<ConsoleSessionKind> {
358+
fn id(&self) -> ConsoleSessionUuid {
360359
self.id
361360
}
362361
fn silo_user_id(&self) -> Uuid {
@@ -378,17 +377,12 @@ impl session_cookie::SessionStore for WhoamiServerState {
378377
type SessionModel = FakeSession;
379378

380379
async fn session_fetch(&self, token: String) -> Option<Self::SessionModel> {
381-
self.sessions
382-
.lock()
383-
.unwrap()
384-
.iter()
385-
.find(|s| s.token == token)
386-
.map(|s| s.clone())
380+
self.sessions.lock().unwrap().iter().find(|s| s.token == token).cloned()
387381
}
388382

389383
async fn session_update_last_used(
390384
&self,
391-
id: TypedUuid<ConsoleSessionKind>,
385+
id: ConsoleSessionUuid,
392386
) -> Option<Self::SessionModel> {
393387
let mut sessions = self.sessions.lock().unwrap();
394388
if let Some(pos) = sessions.iter().position(|s| s.id == id) {
@@ -403,10 +397,7 @@ impl session_cookie::SessionStore for WhoamiServerState {
403397
}
404398
}
405399

406-
async fn session_expire(
407-
&self,
408-
id: TypedUuid<ConsoleSessionKind>,
409-
) -> Option<()> {
400+
async fn session_expire(&self, id: ConsoleSessionUuid) -> Option<()> {
410401
let mut sessions = self.sessions.lock().unwrap();
411402
sessions.retain(|s| s.id != id);
412403
Some(())

0 commit comments

Comments
 (0)