From da8715975ad0c002c414bdde1db5495da71a519e Mon Sep 17 00:00:00 2001
From: Christopher Moussa <moussa1@llnl.gov>
Date: Mon, 11 Nov 2024 11:35:58 -0800
Subject: [PATCH] add-user: make --username and --bank required

Problem: The --username and --bank arguments for add-user should be
required arguments, but they are currently treated as optional
arguments. This means that if someone calls add-user without passing
--username or --bank, the corresponding add_user() function will try
to run the function with None values. Requirements should be added for
both of these arguments at the top level so that the call fails if
either one of these is not present.

Add required=True to both the --username and --bank arguments in
add-user. Specify which arguments are being passed where in
flux-account-service.py. Add two tests to t1007-flux-account-users.t to
make sure errors are raised when either one of these is missing from the
add-user call.
---
 src/cmd/flux-account-service.py | 20 ++++++++++----------
 src/cmd/flux-account.py         |  2 ++
 t/t1007-flux-account-users.t    | 10 ++++++++++
 3 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/src/cmd/flux-account-service.py b/src/cmd/flux-account-service.py
index 01eff28e..d636eed2 100755
--- a/src/cmd/flux-account-service.py
+++ b/src/cmd/flux-account-service.py
@@ -171,16 +171,16 @@ def view_user(self, handle, watcher, msg, arg):
     def add_user(self, handle, watcher, msg, arg):
         try:
             val = u.add_user(
-                self.conn,
-                msg.payload["username"],
-                msg.payload["bank"],
-                msg.payload["userid"],
-                msg.payload["shares"],
-                msg.payload["max_running_jobs"],
-                msg.payload["max_active_jobs"],
-                msg.payload["max_nodes"],
-                msg.payload["queues"],
-                msg.payload["projects"],
+                conn=self.conn,
+                username=msg.payload["username"],
+                bank=msg.payload["bank"],
+                uid=msg.payload["userid"],
+                shares=msg.payload["shares"],
+                max_running_jobs=msg.payload["max_running_jobs"],
+                max_active_jobs=msg.payload["max_active_jobs"],
+                max_nodes=msg.payload["max_nodes"],
+                queues=msg.payload["queues"],
+                projects=msg.payload["projects"],
             )
 
             payload = {"add_user": val}
diff --git a/src/cmd/flux-account.py b/src/cmd/flux-account.py
index 7bcd3342..4d5284e8 100755
--- a/src/cmd/flux-account.py
+++ b/src/cmd/flux-account.py
@@ -66,6 +66,7 @@ def add_add_user_arg(subparsers):
         "--username",
         help="username",
         metavar="USERNAME",
+        required=True,
     )
     subparser_add_user.add_argument(
         "--userid",
@@ -77,6 +78,7 @@ def add_add_user_arg(subparsers):
         "--bank",
         help="bank to charge jobs against",
         metavar="BANK",
+        required=True,
     )
     subparser_add_user.add_argument(
         "--shares",
diff --git a/t/t1007-flux-account-users.t b/t/t1007-flux-account-users.t
index 23e7dbf2..9db1332a 100755
--- a/t/t1007-flux-account-users.t
+++ b/t/t1007-flux-account-users.t
@@ -50,6 +50,16 @@ test_expect_success 'add some queues to the DB' '
 	flux account add-queue special --priority=99999
 '
 
+test_expect_success 'call add-user without specifying a username' '
+	test_must_fail flux account add-user --bank=A > error.out 2>&1 &&
+	grep "add-user: error: the following arguments are required: --username" error.out
+'
+
+test_expect_success 'call add-user without specifying a bank' '
+	test_must_fail flux account add-user --username=user5011 > error.out 2>&1 &&
+	grep "add-user: error: the following arguments are required: --bank" error.out
+'
+
 test_expect_success 'trying to add an association that already exists should raise an IntegrityError' '
 	test_must_fail flux account add-user --username=user5011 --userid=5011 --bank=A > already_exists.out 2>&1 &&
 	grep "association user5011,A already active in association_table" already_exists.out