implicit authz on token list and delete by using current actor ID

Author: david-crespo

nexus/db-queries/src/db/datastore/device_auth.rs

@@ -19,6 +19,7 @@ use nexus_db_schema::schema::device_access_token;
 use omicron_common::api::external::CreateResult;
 use omicron_common::api::external::DataPageParams;
 use omicron_common::api::external::Error;
+use omicron_common::api::external::InternalContext;
 use omicron_common::api::external::ListResultVec;
 use omicron_common::api::external::LookupResult;
 use omicron_common::api::external::LookupType;
@@ -181,19 +182,25 @@ impl DataStore {
             })
     }
 
-    pub async fn device_access_tokens_list(
+    // Similar to session hard delete and silo group list, we do not do a
+    // typical authz check, instead effectively encoding the policy here that
+    // any user is allowed to list and delete their own tokens. When we add the
+    // ability for silo admins to list and delete tokens from any user, we will
+    // have to model these permissions properly in the polar policy.
+
+    pub async fn current_user_token_list(
         &self,
         opctx: &OpContext,
-        authz_user: &authz::SiloUser,
         pagparams: &DataPageParams<'_, Uuid>,
     ) -> ListResultVec<DeviceAccessToken> {
-        // TODO: this authz check can't be right can it? or at least, we
-        // should probably handle this explicitly at the policy level
-        opctx.authorize(authz::Action::ListChildren, authz_user).await?;
+        let &actor = opctx
+            .authn
+            .actor_required()
+            .internal_context("listing current user's tokens")?;
 
         use nexus_db_schema::schema::device_access_token::dsl;
         paginated(dsl::device_access_token, dsl::id, &pagparams)
-            .filter(dsl::silo_user_id.eq(authz_user.id()))
+            .filter(dsl::silo_user_id.eq(actor.actor_id()))
             // we don't have time_deleted on tokens. unfortunately this is not
             // indexed well. maybe it can be!
             .filter(
@@ -207,19 +214,20 @@ impl DataStore {
             .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))
     }
 
-    pub async fn device_access_token_delete(
+    pub async fn current_user_token_delete(
         &self,
         opctx: &OpContext,
-        authz_user: &authz::SiloUser,
         token_id: Uuid,
     ) -> Result<(), Error> {
-        // TODO: surely this is the wrong permission
-        opctx.authorize(authz::Action::Modify, authz_user).await?;
+        let &actor = opctx
+            .authn
+            .actor_required()
+            .internal_context("deleting current user's token")?;
 
         use nexus_db_schema::schema::device_access_token::dsl;
         let num_deleted = diesel::delete(dsl::device_access_token)
+            .filter(dsl::silo_user_id.eq(actor.actor_id()))
             .filter(dsl::id.eq(token_id))
-            .filter(dsl::silo_user_id.eq(authz_user.id()))
             .execute_async(&*self.pool_connection_authorized(opctx).await?)
             .await
             .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?;

nexus/src/app/device_auth.rs

@@ -56,7 +56,7 @@ use anyhow::anyhow;
 use nexus_types::external_api::params::DeviceAccessTokenRequest;
 use nexus_types::external_api::views;
 use omicron_common::api::external::{
-    CreateResult, DataPageParams, Error, InternalContext, ListResultVec,
+    CreateResult, DataPageParams, Error, ListResultVec,
 };
 
 use chrono::{Duration, Utc};
@@ -297,34 +297,14 @@ impl super::Nexus {
         opctx: &OpContext,
         pagparams: &DataPageParams<'_, Uuid>,
     ) -> ListResultVec<DeviceAccessToken> {
-        let &actor = opctx
-            .authn
-            .actor_required()
-            .internal_context("loading current user to list tokens")?;
-        let (.., authz_user) = LookupPath::new(opctx, self.datastore())
-            .silo_user_id(actor.actor_id())
-            .lookup_for(authz::Action::ListChildren)
-            .await?;
-        self.db_datastore
-            .device_access_tokens_list(opctx, &authz_user, pagparams)
-            .await
+        self.db_datastore.current_user_token_list(opctx, pagparams).await
     }
 
     pub(crate) async fn current_user_token_delete(
         &self,
         opctx: &OpContext,
         token_id: Uuid,
     ) -> Result<(), Error> {
-        let &actor = opctx
-            .authn
-            .actor_required()
-            .internal_context("loading current user to delete token")?;
-        let (.., authz_user) = LookupPath::new(opctx, self.datastore())
-            .silo_user_id(actor.actor_id())
-            .lookup_for(authz::Action::Modify)
-            .await?;
-        self.db_datastore
-            .device_access_token_delete(opctx, &authz_user, token_id)
-            .await
+        self.db_datastore.current_user_token_delete(opctx, token_id).await
     }
 }

nexus/tests/integration_tests/device_auth.rs

@@ -12,7 +12,7 @@ use nexus_db_queries::db::fixed_data::silo::DEFAULT_SILO;
 use nexus_db_queries::db::identity::{Asset, Resource};
 use nexus_test_utils::http_testing::TestResponse;
 use nexus_test_utils::resource_helpers::{
-    object_get, object_put, object_put_error,
+    object_delete_error, object_get, object_put, object_put_error,
 };
 use nexus_test_utils::{
     http_testing::{AuthnMode, NexusRequest, RequestBuilder},
@@ -201,17 +201,9 @@ async fn test_device_auth_flow(cptestctx: &ControlPlaneTestContext) {
 
     let token_id = tokens_unpriv_after[0].id;
 
-    // Test that privileged user cannot delete unpriv's token through this
-    // endpoint, though it will probably be able to do it via a different one
+    // Priv user cannot delete unpriv's token through this endpoint by ID
     let token_url = format!("/v1/me/device-tokens/{}", token_id);
-    NexusRequest::new(
-        RequestBuilder::new(testctx, Method::DELETE, &token_url)
-            .expect_status(Some(StatusCode::NOT_FOUND)),
-    )
-    .authn_as(AuthnMode::PrivilegedUser)
-    .execute()
-    .await
-    .expect("privileged user should get a 404 when trying to delete another user's token");
+    object_delete_error(testctx, &token_url, StatusCode::NOT_FOUND).await;
 
     // Test deleting the token as the owner
     NexusRequest::object_delete(testctx, &token_url)

nexus/tests/output/uncovered-authz-endpoints.txt

@@ -1,6 +1,6 @@
 API endpoints with no coverage in authz tests:
 probe_delete                             (delete "/experimental/v1/probes/{probe}")
-current_user_token_delete                (delete "/v1/me/device-tokens/{token_id}")
+current_user_device_token_delete         (delete "/v1/me/device-tokens/{token_id}")
 probe_list                               (get    "/experimental/v1/probes")
 probe_view                               (get    "/experimental/v1/probes/{probe}")
 support_bundle_download                  (get    "/experimental/v1/system/support-bundles/{bundle_id}/download")