correct and more robust update privilege check

This commit is contained in:
Scott Lamb 2022-12-26 00:38:48 -05:00
parent 163eaa4cf9
commit 88d7165c3e

View File

@ -128,8 +128,8 @@ impl Service {
let user = db let user = db
.get_user_by_id_mut(id) .get_user_by_id_mut(id)
.ok_or_else(|| format_err_t!(NotFound, "can't find requested user"))?; .ok_or_else(|| format_err_t!(NotFound, "can't find requested user"))?;
if r.update.as_ref().map(|u| u.password).is_some() if r.update.as_ref().and_then(|u| u.password).is_some()
&& r.precondition.as_ref().map(|p| p.password).is_none() && r.precondition.as_ref().and_then(|p| p.password).is_none()
&& !caller.permissions.admin_users && !caller.permissions.admin_users
{ {
bail_t!( bail_t!(
@ -137,12 +137,6 @@ impl Service {
"to change password, must supply previous password or have admin_users permission" "to change password, must supply previous password or have admin_users permission"
); );
} }
if r.update.as_ref().map(|u| &u.permissions).is_some() && !caller.permissions.admin_users {
bail_t!(
Unauthenticated,
"to change permissions, must have admin_users permission"
);
}
match (r.csrf, caller.user.and_then(|u| u.session)) { match (r.csrf, caller.user.and_then(|u| u.session)) {
(None, Some(_)) => bail_t!(Unauthenticated, "csrf must be supplied"), (None, Some(_)) => bail_t!(Unauthenticated, "csrf must be supplied"),
(Some(csrf), Some(session)) if !csrf_matches(csrf, session.csrf) => { (Some(csrf), Some(session)) if !csrf_matches(csrf, session.csrf) => {
@ -180,9 +174,8 @@ impl Service {
} }
if let Some(mut update) = r.update { if let Some(mut update) = r.update {
let mut change = user.change(); let mut change = user.change();
if let Some(n) = update.username.take() {
change.username = n.to_string(); // First, set up updates which non-admins are allowed to perform on themselves.
}
if let Some(preferences) = update.preferences.take() { if let Some(preferences) = update.preferences.take() {
change.config.preferences = preferences; change.config.preferences = preferences;
} }
@ -191,6 +184,14 @@ impl Service {
Some(None) => change.clear_password(), Some(None) => change.clear_password(),
Some(Some(p)) => change.set_password(p.to_owned()), Some(Some(p)) => change.set_password(p.to_owned()),
} }
// Requires admin_users if there's anything else.
if update != Default::default() && !caller.permissions.admin_users {
bail_t!(Unauthenticated, "must have admin_users permission");
}
if let Some(n) = update.username.take() {
change.username = n.to_string();
}
if let Some(permissions) = update.permissions.take() { if let Some(permissions) = update.permissions.take() {
change.permissions = (&permissions).into(); change.permissions = (&permissions).into();
} }
@ -199,6 +200,8 @@ impl Service {
if update != Default::default() { if update != Default::default() {
bail_t!(Unimplemented, "updates not supported: {:#?}", &update); bail_t!(Unimplemented, "updates not supported: {:#?}", &update);
} }
// Then apply all together.
db.apply_user_change(change)?; db.apply_user_change(change)?;
} }
Ok(plain_response(StatusCode::NO_CONTENT, &b""[..])) Ok(plain_response(StatusCode::NO_CONTENT, &b""[..]))