Skip to content

Commit

Permalink
Prevent unrestrained unblocking of workers
Browse files Browse the repository at this point in the history
This change prevents non-approver users from unblocking their workers
when they are not blocked by themselves.
  • Loading branch information
MinetaS committed Jul 3, 2024
1 parent 61ec81b commit 7c16021
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 17 deletions.
4 changes: 2 additions & 2 deletions server/fishtest/rundb.py
Original file line number Diff line number Diff line change
Expand Up @@ -1033,10 +1033,10 @@ def sync_request_task(self, worker_info):
my_name = worker_name(worker_info, short=True)
host_url = worker_info.get("host_url", "<host_url>")
w = self.workerdb.get_worker(my_name)
if w["blocked"]:
if w["blocked_by"]:
# updates last_updated
self.workerdb.update_worker(
my_name, blocked=w["blocked"], message=w["message"]
my_name, blocked_by=w["blocked_by"], message=w["message"]
)
error = self.blocked_worker_message(my_name, w["message"], host_url)
return {"task_waiting": False, "error": error}
Expand Down
2 changes: 1 addition & 1 deletion server/fishtest/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ def size_is_length(x):
worker_schema = {
"_id?": ObjectId,
"worker_name": short_worker_name,
"blocked": bool,
"blocked_by": str,
"message": worker_message,
"last_updated": datetime_utc,
}
Expand Down
40 changes: 30 additions & 10 deletions server/fishtest/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ def login(request):
# limited.


def worker_email(worker_name, blocker_name, message, host_url, blocked):
def worker_email(worker_name, blocker_name, message, host_url, blocked_by):
owner_name = worker_name.split("-")[0]
body = f"""\
Dear {owner_name},
Expand Down Expand Up @@ -204,7 +204,7 @@ def workers(request):
blocker_name,
w["message"],
request.host_url,
w["blocked"],
w["blocked_by"],
)
w["subject"] = f"Issue(s) with worker {w['worker_name']}"

Expand Down Expand Up @@ -247,26 +247,46 @@ def workers(request):
)
message = message[:max_chars]
message = normalize_lf(message)
was_blocked = request.workerdb.get_worker(worker_name)["blocked"]
request.rundb.workerdb.update_worker(
worker_name, blocked=blocked, message=message
)
if blocked != was_blocked:
blocked_by = request.workerdb.get_worker(worker_name)["blocked_by"]
new_worker_info = {
"worker_name": worker_name,
"blocked_by": blocked_by,
"message": message,
}
if blocked and not blocked_by:
new_worker_info["blocked_by"] = blocker_name
request.session.flash(
f"Worker {worker_name} {'blocked' if blocked else 'unblocked'}!",
f"Worker {worker_name} blocked!",
)
request.actiondb.block_worker(
username=blocker_name,
worker=worker_name,
message="blocked" if blocked else "unblocked",
message="blocked",
)
elif not blocked and blocked_by:
# Prevent users from abusing unblocking workers.
if not is_approver and blocked_by != blocker_name:
request.session.flash(
f"Contact approvers to unblock worker {worker_name}!",
)
else:
new_worker_info["blocked_by"] = None
request.session.flash(
f"Worker {worker_name} unblocked!",
)
request.actiondb.block_worker(
username=blocker_name,
worker=worker_name,
message="unblocked",
)
request.workerdb.update_worker(**new_worker_info)
return HTTPFound(location=request.route_url("workers", worker_name="show"))

w = request.rundb.workerdb.get_worker(worker_name)
return {
"show_admin": True,
"worker_name": worker_name,
"blocked": w["blocked"],
"blocked": w["blocked_by"] is not None,
"message": w["message"],
"show_email": is_approver,
"last_updated": w["last_updated"],
Expand Down
8 changes: 4 additions & 4 deletions server/fishtest/workerdb.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,23 @@ def get_worker(
if r is None:
return {
"worker_name": worker_name,
"blocked": False,
"blocked_by": None,
"message": "",
"last_updated": None,
}
else:
return r

def update_worker(self, worker_name, blocked=None, message=None):
def update_worker(self, worker_name, blocked_by=None, message=None):
r = {
"worker_name": worker_name,
"blocked": blocked,
"blocked_by": blocked_by,
"message": message,
"last_updated": datetime.now(timezone.utc),
}
validate(worker_schema, r, "worker") # may throw exception
self.workers.replace_one({"worker_name": worker_name}, r, upsert=True)

def get_blocked_workers(self):
q = {"blocked": True}
q = {"blocked": {"$ne": None}}
return list(self.workers.find(q))

0 comments on commit 7c16021

Please sign in to comment.