Improve error messages for missing roles.

src/daemon/auth/authorizer.rs

@@ -65,7 +65,7 @@ impl From<openid_connect::AuthProvider> for AuthProvider {
 }
 
 impl AuthProvider {
-    /// Authenticate a user from information included in an HTTP request.
+    /// Authenticates a user from information included in an HTTP request.
     ///
     /// Returns `Ok(None)` to indicate that no authentication information
     /// was present in the request and the request should thus be treated

src/daemon/auth/providers/admin_token.rs

@@ -1,3 +1,5 @@
+//! Auth provider using a pre-defined token.
+
 use std::sync::Arc;
 use crate::commons::KrillResult;
 use crate::commons::api::Token;
@@ -7,34 +9,51 @@ use crate::daemon::auth::{AuthInfo, LoggedInUser, Role};
 use crate::daemon::config::Config;
 use crate::daemon::http::{HttpResponse, HyperRequest};
 
-// This is NOT an actual relative path to redirect to. Instead it is the path
-// string of an entry in the Vue router routes table to "route" to (in the
-// Lagosta single page application). See the routes array in router.js of the
-// Lagosta source code. Ideally we could instead return a route name and then
-// Lagosta could change this path without requiring that we update to match.
+
+//------------ Constants -----------------------------------------------------
+
+/// The path defined in Krill UI for the login view.
 const LAGOSTA_LOGIN_ROUTE_PATH: &str = "/login";
 
+
+//------------ AuthProvider --------------------------------------------------
+
+/// The admin token auth provider.
+///
+/// This auth provider takes a single token from the configuration and
+/// only allows requests that carry this token as a bearer token.
+///
+/// Currently, the this provider is hard-coded to translate this token into
+/// a user named “admin” having the admin special role which allows
+/// everything everywhere all at once.
 pub struct AuthProvider {
+    /// The configured token to compare with.
     required_token: Token,
+
+    /// The user name of the actor if authentication succeeds.
     user_id: Arc<str>,
+
+    /// The role to use if authentication succeeds.
     role: Arc<Role>,
 }
 
 impl AuthProvider {
+    /// Creates a new admin token auth provider from the given config.
     pub fn new(config: Arc<Config>) -> Self {
         AuthProvider {
             required_token: config.admin_token.clone(),
-            // XXX Get from config.
-            user_id: "admin".into(),
+            user_id: "admin-token".into(),
             role: Role::admin().into(),
         }
     }
-}
-
-impl AuthProvider {
+
+    /// Authenticates a user from information included in an HTTP request.
+    ///
+    /// If there request has a bearer token, returns `Ok(Some(_))` if it
+    /// matches the configured token or `Err(_)` otherwise. If there is no
+    /// bearer token, returns `Ok(None)`.
     pub fn authenticate(
-        &self,
-        request: &HyperRequest,
+        &self, request: &HyperRequest,
     ) -> Result<Option<AuthInfo>, ApiAuthError> {
         if log_enabled!(log::Level::Trace) {
             trace!("Attempting to authenticate the request..");
@@ -59,11 +78,13 @@ impl AuthProvider {
         res
     }
 
+    /// Returns an HTTP text response with the login URL.
     pub fn get_login_url(&self) -> KrillResult<HttpResponse> {
         // Direct Lagosta to show the user the Lagosta API token login form
         Ok(HttpResponse::text_no_cache(LAGOSTA_LOGIN_ROUTE_PATH.into()))
     }
 
+    /// Establishes a client session from credentials in an HTTP request.
     pub fn login(&self, request: &HyperRequest) -> KrillResult<LoggedInUser> {
         match self.authenticate(request)? {
             Some(_actor) => Ok(LoggedInUser {
@@ -76,6 +97,7 @@ impl AuthProvider {
         }
     }
 
+    /// Returns an HTTP text response with the logout URL.
     pub fn logout(
         &self,
         request: &HyperRequest,

src/daemon/auth/providers/config_file.rs

@@ -1,3 +1,5 @@
+//! Auth provider using user information from the configuration.
+
 use std::collections::HashMap;
 use std::sync::Arc;
 use base64::engine::general_purpose::STANDARD as BASE64_ENGINE;
@@ -20,19 +22,35 @@ use crate::daemon::http::{HttpResponse, HyperRequest};
 /// The location of the login page in Krill UI.
 const UI_LOGIN_ROUTE_PATH: &str = "/login?withId=true";
 
+/// A password hash used to prolong operation when a user doesn’t exist.
+const FAKE_PASSWORD_HASH: &str = "66616B652070617373776F72642068617368";
+
+/// A salt value used to prolong operation when a user doesn’t exist.
+const FAKE_SALT: &str = "66616B652073616C74";
+
 
 //------------ AuthProvider --------------------------------------------------
 
+/// The config file auth provider.
+///
+/// This auth provider uses user and role information provided via the Krill
+/// config and authenticates requests using HTTP Basic Authorization headers.
 pub struct AuthProvider {
+    /// The user directory.
     users: HashMap<String, UserDetails>,
+
+    /// The role directory.
     roles: Arc<RoleMap>,
+
+    /// The session key for encrypting client session information.
     session_key: crypt::CryptState,
+
+    /// The client session cache.
     session_cache: SessionCache,
-    fake_password_hash: String,
-    fake_salt: String,
 }
 
 impl AuthProvider {
+    /// Creates an auth provider from the given config.
     pub fn new(
         config: &Config,
     ) -> KrillResult<Self> {
@@ -47,8 +65,6 @@ impl AuthProvider {
             roles,
             session_key,
             session_cache: SessionCache::new(),
-            fake_password_hash: hex::encode("fake password hash"),
-            fake_salt: hex::encode("fake salt"),
         })
     }
 
@@ -77,6 +93,14 @@ impl AuthProvider {
     ) -> Result<AuthInfo, ApiAuthError> {
         self.roles.get(&session.secrets.role).map(|role| {
             AuthInfo::user(session.user_id.clone(), role)
+        }).ok_or_else(|| {
+            ApiAuthError::ApiAuthPermanentError(
+                format!(
+                    "user '{}' with undefined role '{}' \
+                    not caught by config check",
+                    session.user_id, session.secrets.role
+                )
+            )
         })
     }
 }
@@ -139,12 +163,9 @@ impl AuthProvider {
         let (user_password_hash, user_salt) =
             match self.users.get(&auth.username) {
                 Some(user) => {
-                    (user.password_hash.to_string(), user.salt.clone())
+                    (user.password_hash.as_ref(), user.salt.as_ref())
                 }
-                None => (
-                    self.fake_password_hash.clone(),
-                    self.fake_salt.clone(),
-                ),
+                None => (FAKE_PASSWORD_HASH, FAKE_SALT),
             };
 
         let username = auth.username.trim().nfkc().collect::<String>();
@@ -206,7 +227,15 @@ impl AuthProvider {
         };
 
         // Check that the user is allowed to log in.
-        let role = self.roles.get(&user.role)?;
+        let role = self.roles.get(&user.role).ok_or_else(|| {
+            ApiAuthError::ApiAuthPermanentError(
+                format!(
+                    "user '{}' with undefined role '{}' \
+                    not caught by config check",
+                    username, user.role,
+                )
+            )
+        })?;
 
         if !role.is_allowed(Permission::Login, None) {
             let reason = format!(

src/daemon/auth/providers/openid_connect/provider.rs

@@ -1106,7 +1106,15 @@ impl AuthProvider {
     ) -> KrillResult<AuthInfo> {
         Ok(AuthInfo::user(
             session.user_id.clone(),
-            self.config.auth_roles.get(&session.secrets.role)?
+            self.config.auth_roles.get(&session.secrets.role).ok_or_else(|| {
+                ApiAuthError::ApiAuthPermanentError(
+                    format!(
+                        "user '{}' with undefined role '{}' \
+                        not caught during login",
+                        session.user_id, session.secrets.role,
+                    )
+                )
+            })?
         ))
     }
 }
@@ -1657,7 +1665,17 @@ impl AuthProvider {
                 let id = claims.extract_id()?;
                 let role_name = claims.extract_role()?;
 
-                let role = self.config.auth_roles.get(&role_name)?;
+                let role = self.config.auth_roles.get(
+                    &role_name
+                ).ok_or_else(|| {
+                    let reason = format!(
+                        "Login denied for user '{}': \
+                         user is assigned undefined role '{}'.",
+                         id, role_name
+                    );
+                    warn!("{}", reason);
+                    Error::ApiInsufficientRights(reason)
+                })?;
 
                 // Step 4 1/2: Check that the user is allowed to log in.
                 if !role.is_allowed(Permission::Login, None) {

src/daemon/auth/roles.rs

@@ -2,17 +2,23 @@ use std::collections::HashMap;
 use std::sync::Arc;
 use rpki::ca::idexchange::MyHandle;
 use serde::Deserialize;
-use crate::commons::error::ApiAuthError;
 use super::{Permission, PermissionSet};
 
 
 //------------ Role ----------------------------------------------------------
 
-/// The role of actor has.
+/// A set of access permissions for resources.
 ///
-/// Permissions aren’t assigned to actors directly but rather to roles to
-/// which actors are assigned in turn.
+/// Roles provide an intermediary for assigning access permissions to users
+/// by managing [permission sets][PermissionSet]. Separete sets can be
+/// provided for specific resources, all other resources, and requests that
+/// do not operate on resources.
+///
+/// Currently, roles are given names and are defined in
+/// [Config::auth_roles][crate::daemon::config::Config::auth_roles] and
+/// referenced by authorization providers through those names.
 #[derive(Clone, Debug, Deserialize, Eq, PartialEq)]
+#[serde(from = "RoleConf")]
 pub struct Role {
     /// Permissions for requests without specific resources.
     none: PermissionSet,
@@ -28,26 +34,42 @@ pub struct Role {
 }
 
 impl Role {
+    /// Creates the special admin role.
+    ///
+    /// This role allows all access to everything.
     pub fn admin() -> Self {
         Self::simple(PermissionSet::ANY)
     }
 
+    /// Creates the default read-write role.
+    ///
+    /// This role uses `PermissionSet::READWRITE` for everything.
     pub fn readwrite() -> Self {
         Self::simple(PermissionSet::READWRITE)
     }
 
+    /// Creates the default read-only role.
+    ///
+    /// This role uses `PermissionSet::READONLY` for everything.
     pub fn readonly() -> Self {
         Self::simple(PermissionSet::READONLY)
     }
 
+    /// Creates the special testbed role.
+    ///
+    /// This role uses `PermissionSet::TESTBED` for everything.
     pub fn testbed() -> Self {
         Self::simple(PermissionSet::TESTBED)
     }
 
+    /// Creates the anonymous special role.
+    ///
+    /// This role allows nothing.
     pub fn anonymous() -> Self {
         Self::simple(PermissionSet::NONE)
     }
 
+    /// Creates a role that uses the provided permission set for all access.
     pub fn simple(permissions: PermissionSet) -> Self {
         Self {
             none: permissions,
@@ -56,6 +78,10 @@ impl Role {
         }
     }
 
+    /// Creates a role that uses the provided set for the given resources.
+    ///
+    /// The role will allow access with the set to non-resource requests and
+    /// all resources provided. Access to all other resources will be denied.
     pub fn with_resources(
         permissions: PermissionSet,
         resources: impl IntoIterator<Item = MyHandle>
@@ -69,6 +95,12 @@ impl Role {
         }
     }
 
+    /// Creates a comples role.
+    ///
+    /// The permission set `none` will be used for non-resource requests.
+    /// The `resources` hash map contains special permission sets for the
+    /// provided resources. The `any` set will be used for all resources
+    /// not mentioned in the hash map.
     pub fn complex(
         none: PermissionSet,
         any: PermissionSet,
@@ -77,6 +109,13 @@ impl Role {
         Self { none, any, resources }
     }
 
+    /// Returns whether access is allowed.
+    ///
+    /// The method whether the role allows access with the provided
+    /// `permission` to the provided `resource`. If the resource is `None`,
+    /// access for non-resource requests is checked.
+    ///
+    /// Returns `true` if access is allowed or `false` if not.
     pub fn is_allowed(
         &self,
         permission: Permission,
@@ -114,38 +153,45 @@ impl From<RoleConf> for Role {
 /// [`Role`] supports. This is on purpose to keep the config format simple.
 #[derive(Clone, Debug, Deserialize)]
 struct RoleConf {
+    /// The permission set to use.
     permissions: PermissionSet,
 
+    /// An optional list of resources to limit access to.
+    ///
+    /// If this is `None`, access to all resources will be allowed.
     cas: Option<Vec<MyHandle>>,
 }
 
 
 //------------ RoleMap -------------------------------------------------------
 
+/// A mapping storing roles under a name.
+///
+/// Roles are stored behind an arc to users to keep a keep of the role around.
 #[derive(Clone, Debug, Default, Deserialize)]
 pub struct RoleMap(HashMap<String, Arc<Role>>);
 
 impl RoleMap {
+    /// Creates a new, empty role map.
     pub fn new() -> Self {
         Self::default()
     }
 
+    /// Adds the given role.
     pub fn add(
         &mut self, name: impl Into<String>, role: impl Into<Arc<Role>>
     ) {
         self.0.insert(name.into(), role.into());
     }
 
+    /// Returns whether the map contains a role by the given name.
     pub fn contains(&self, name: &str) -> bool {
         self.0.contains_key(name)
     }
 
-    pub fn get(&self, name: &str) -> Result<Arc<Role>, ApiAuthError> {
-        self.0.get(name).cloned().ok_or_else(|| {
-            ApiAuthError::ApiAuthPermanentError(
-                "user with undefined role not caught by config check".into()
-            )
-        })
+    /// Returns the role of the given name if present.
+    pub fn get(&self, name: &str) -> Option<Arc<Role>> {
+        self.0.get(name).cloned()
     }
 }