ref(server): Skip auth checks when not configured#519
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #519 +/- ##
=======================================
Coverage 87.40% 87.40%
=======================================
Files 86 86
Lines 13893 13887 -6
=======================================
- Hits 12143 12138 -5
+ Misses 1750 1749 -1
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| if !enforce { | ||
| objectstore_log::warn!(?permission, ?usecase, ?code, ?msg, "Auth failure"); | ||
|
|
||
| if warn { |
There was a problem hiding this comment.
This moves the responsibility to decide on the log level to the caller, which actually carries the necessary info. It's a more appropriate place than on the error.
| objectstore_log::warn!(?permission, ?usecase, ?code, ?msg, "Auth failure"); | ||
|
|
||
| if warn { | ||
| objectstore_log::warn!(code, reason=%self, "Auth failure"); |
There was a problem hiding this comment.
Permission and usecase were only passed at a single place where they are available. Thus, we're now configuring them on the scope, removing two special-case parameters from this function's signature. The values are still available in the Sentry error.
Note that they won't be printed to stderr anymore, which we have not needed.
| true => Ok(auth_result?), | ||
| false => Ok(()), | ||
| pub fn new(service: StorageService, context: Option<AuthContext>, enforce: bool) -> Self { | ||
| Self { |
There was a problem hiding this comment.
enforce && context.is_none() is no longer a valid state, but also the contract has changed:
- the extractor still has the responsibility for validating this
context: Nonenow is the primary argument andenforceonly controls what happens when a context is provided.
The new doc comment above elaborates on this. Note that this is just the contract for AuthAwareService, not the public config!
| /// Returns `Ok(())` if authorized, or an error indicating the reason. | ||
| pub fn check_permission(&self, perm: Permission, context: &ObjectContext) -> ApiResult<()> { | ||
| self.assert_authorized(perm, context) | ||
| if let Some(auth) = &self.context |
There was a problem hiding this comment.
This was inlined. Apparently, the assert_authorized method was intended to become public, but instead a plain wrapper created.
| if !state.config.auth.is_active() { | ||
| return Ok(AuthAwareService::new(state.service.clone(), None, enforce)); | ||
| } |
There was a problem hiding this comment.
This is the main difference: If auth is inactive, we skip everything.
Everything below is equivalent to before and only refactored for readability / brevity.
| /// configured. Without enforcement, authorization checks are still performed and reported but | ||
| /// failures will not result in `403 Unauthorized` | ||
| pub fn is_active(&self) -> bool { | ||
| self.enforce || !self.keys.is_empty() |
There was a problem hiding this comment.
This is because enforce defaults to true: We must activate auth also when there are no keys provided, so that the default config triggers auth failures.
2 participants
Auth checks previously ran on every request regardless of server-side configuration, producing noisy warnings in environments without auth (e.g. local development). Auth is now skipped entirely when it is not configured — meaning no keys are present and enforcement is off (
AuthZ::is_active()).When auth is active,
enforcecontrols whether failures reject the request (debug log) or just warn.AuthAwareService::newis now infallible since the "enforcement enabled but no context" error state is no longer reachable.