Add admin configuration #316
Conversation
ionut-arm
added
enhancement
| @@ -24,7 +24,10 @@ use zeroize::Zeroize; | |||
|
|
|||
| /// String wrapper for app names | |||
| #[derive(Debug, Clone, Eq, PartialEq, Hash)] | |||
| pub struct ApplicationName(String); | |||
| pub struct ApplicationName { | |||
There was a problem hiding this comment.
Wasn't sure if I should just change this to Application
There was a problem hiding this comment.
I think for this PR this is fine keeping the name to not trigger too many changes
src/authenticators/mod.rs
Outdated
| } | ||
|
|
||
| impl Admin { | ||
| fn check_app_name(&self, app_name: &mut ApplicationName) { |
There was a problem hiding this comment.
Similarly - wasn't sure what name to give to this, but it felt better to keep the place where the actual check is done and set_admin is called contained, so it's easier to verify
|
Also, wasn't sure if I should add the checks in the backend or if you'd rather do it when implementing the operations that actually need them |
There was a problem hiding this comment.
Thanks looks good! I agree with the general algorithm, just had some questions about method naming to maybe understand it better.
Also, wasn't sure if I should add the checks in the backend or if you'd rather do it when implementing the operations that actually need them
I guess we can do it when adding support for the first admin operations.
| # WARNING: Admins have special privileges and access to operations that are not permitted for normal | ||
| # users of the service. Only enable this feature with some list of admins if you are confident | ||
| # about the need for those permissions. | ||
| # Read more here: https://parallaxsecond.github.io/parsec-book/parsec_client/operations/index.html#core-operations |
| } | ||
|
|
||
| /// Verify if the app is an admin | ||
| fn admin_check(&self, app_name: &mut ApplicationName) { |
There was a problem hiding this comment.
I would prefer renaming this method to understand better that it modifies app_name, what about set_admin_status? And similarly for the one on Admin
| let conn_metadata = None; | ||
| let status = authenticator | ||
| .authenticate(&RequestAuth::new(Vec::new()), conn_metadata) | ||
| .expect_err("Empty auth should have failed"); | ||
|
|
||
| assert_eq!(status, ResponseStatus::AuthenticationError); | ||
| } | ||
|
|
||
| #[test] | ||
| fn admin_check() { |
| @@ -24,7 +24,10 @@ use zeroize::Zeroize; | |||
|
|
|||
| /// String wrapper for app names | |||
| #[derive(Debug, Clone, Eq, PartialEq, Hash)] | |||
| pub struct ApplicationName(String); | |||
| pub struct ApplicationName { | |||
There was a problem hiding this comment.
I think for this PR this is fine keeping the name to not trigger too many changes
src/authenticators/mod.rs
Outdated
| } | ||
|
|
||
| /// Flag current application as being an admin | ||
| fn set_admin(&mut self) { |
There was a problem hiding this comment.
What about this method taking a boolean to set the field? I feel like set_admin(true) is nice to understand
There was a problem hiding this comment.
I think I'll change my approach here and just keep ApplicationName immutable, though that means I'll have to check whether a string is an admin name before calling the constructor.
| AuthenticatorConfig::Direct { admins } => authenticators.push(( | ||
| AuthType::Direct, | ||
| Box::from(DirectAuthenticator::new( | ||
| admins.as_ref().cloned().unwrap_or_default(), |
There was a problem hiding this comment.
Will the Default yield to an empty vector?
There was a problem hiding this comment.
Yes, that's the default for Vec
ionut-arm
force-pushed
the
admin
branch
2 times, most recently
from
b4b40d9 to
c42fdae
Compare
There was a problem hiding this comment.
It looks better like that indeed!
Just one nitpick but feel free to ignore:
Ok(ApplicationName::new(
app_name.clone(),
self.admins.is_admin(&app_name),
))could that be changed to:
let is_admin = self.admins.is_admin(&app_name);
Ok(ApplicationName::new(
app_name,
is_admin,
))in order to avoid the clone?
|
Good point - was aware of the "tradeoff", but didn't think of performance, though if you do a lot of operations, it might matter |
This commit adds support for authenticators to identify admin applications. The admin app names can be configured from the config.toml file and lead to the ApplicationName having a flag set indicating whether the app is an admin or not. Signed-off-by: Ionut Mihalcea <ionut.mihalcea@arm.com>
This commit adds support for authenticators to identify admin
applications. The admin app names can be configured from the config.toml
file and lead to the ApplicationName having a flag set indicating
whether the app is an admin or not.
Fixes #308