From d4dd8c13e3470783441fcd1c1279593e0ac9b05e Mon Sep 17 00:00:00 2001
From: Pauline <4224001+paulineribeyre@users.noreply.github.com>
Date: Thu, 12 Jan 2023 16:36:50 -0600
Subject: [PATCH 1/9] migration: client name not unique

---
 .pre-commit-config.yaml                       |   2 +-
 .secrets.baseline                             |  44 +++++-
 fence/models.py                               |   2 +-
 .../a04a70296688_non_unique_client_name.py    |  36 +++++
 tests/migrations/test_a04a70296688.py         | 148 ++++++++++++++++++
 tests/migrations/test_ea7e1b843f82.py         |   1 -
 6 files changed, 225 insertions(+), 8 deletions(-)
 create mode 100644 migrations/versions/a04a70296688_non_unique_client_name.py
 create mode 100644 tests/migrations/test_a04a70296688.py

diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 554d3c29c..6220df12b 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -4,7 +4,7 @@ repos:
     hooks:
     -   id: black
 -   repo: git@github.com:Yelp/detect-secrets
-    rev: v1.2.0
+    rev: v1.4.0
     hooks:
     -   id: detect-secrets
         args: ['--baseline', '.secrets.baseline']
diff --git a/.secrets.baseline b/.secrets.baseline
index 84b1a9a1a..fe56de184 100644
--- a/.secrets.baseline
+++ b/.secrets.baseline
@@ -1,5 +1,5 @@
 {
-  "version": "1.2.0",
+  "version": "1.4.0",
   "plugins_used": [
     {
       "name": "ArtifactoryDetector"
@@ -115,6 +115,15 @@
     }
   ],
   "results": {
+    ".github/workflows/buildpipeline.yaml": [
+      {
+        "type": "Secret Keyword",
+        "filename": ".github/workflows/buildpipeline.yaml",
+        "hashed_secret": "3e26d6750975d678acb8fa35a0f69237881576b0",
+        "is_verified": false,
+        "line_number": 17
+      }
+    ],
     "deployment/scripts/postgresql/postgresql_init.sql": [
       {
         "type": "Secret Keyword",
@@ -192,6 +201,15 @@
         "line_number": 112
       }
     ],
+    "migrations/versions/a04a70296688_non_unique_client_name.py": [
+      {
+        "type": "Hex High Entropy String",
+        "filename": "migrations/versions/a04a70296688_non_unique_client_name.py",
+        "hashed_secret": "bb2372fb50034d559d2920e7229fb5879cf1be72",
+        "is_verified": false,
+        "line_number": 13
+      }
+    ],
     "migrations/versions/e4c7b0ab68d3_create_tables.py": [
       {
         "type": "Hex High Entropy String",
@@ -269,7 +287,14 @@
         "filename": "tests/data/test_indexed_file.py",
         "hashed_secret": "a62f2225bf70bfaccbc7f1ef2a397836717377de",
         "is_verified": false,
-        "line_number": 410
+        "line_number": 411
+      },
+      {
+        "type": "Secret Keyword",
+        "filename": "tests/data/test_indexed_file.py",
+        "hashed_secret": "c258a8d1264cc59de81f8b1975ac06732b1cf182",
+        "is_verified": false,
+        "line_number": 432
       }
     ],
     "tests/keys/2018-05-01T21:29:02Z/jwt_private_key.pem": [
@@ -290,20 +315,29 @@
         "line_number": 48
       }
     ],
+    "tests/migrations/test_a04a70296688.py": [
+      {
+        "type": "Hex High Entropy String",
+        "filename": "tests/migrations/test_a04a70296688.py",
+        "hashed_secret": "bb2372fb50034d559d2920e7229fb5879cf1be72",
+        "is_verified": false,
+        "line_number": 27
+      }
+    ],
     "tests/migrations/test_ea7e1b843f82.py": [
       {
         "type": "Hex High Entropy String",
         "filename": "tests/migrations/test_ea7e1b843f82.py",
         "hashed_secret": "adb1fcd33b07abf9b6a064745759accea5cb341f",
         "is_verified": false,
-        "line_number": 27
+        "line_number": 26
       },
       {
         "type": "Hex High Entropy String",
         "filename": "tests/migrations/test_ea7e1b843f82.py",
         "hashed_secret": "bb2372fb50034d559d2920e7229fb5879cf1be72",
         "is_verified": false,
-        "line_number": 44
+        "line_number": 43
       }
     ],
     "tests/ras/test_ras.py": [
@@ -334,5 +368,5 @@
       }
     ]
   },
-  "generated_at": "2022-11-10T22:37:01Z"
+  "generated_at": "2023-01-12T22:35:50Z"
 }
diff --git a/fence/models.py b/fence/models.py
index f199569e1..9122e874c 100644
--- a/fence/models.py
+++ b/fence/models.py
@@ -193,7 +193,7 @@ class Client(Base, OAuth2ClientMixin):
     client_secret = Column(String(60), unique=True, index=True, nullable=True)
 
     # human readable name
-    name = Column(String(40), unique=True, nullable=False)
+    name = Column(String(40), nullable=False)
 
     # human readable description, not required
     description = Column(String(400))
diff --git a/migrations/versions/a04a70296688_non_unique_client_name.py b/migrations/versions/a04a70296688_non_unique_client_name.py
new file mode 100644
index 000000000..2a1bb1185
--- /dev/null
+++ b/migrations/versions/a04a70296688_non_unique_client_name.py
@@ -0,0 +1,36 @@
+"""Non-unique client name
+
+Revision ID: a04a70296688
+Revises: ea7e1b843f82
+Create Date: 2022-12-23 09:36:28.425744
+
+"""
+from alembic import op
+
+
+# revision identifiers, used by Alembic.
+revision = "a04a70296688"
+down_revision = "ea7e1b843f82"
+branch_labels = None
+depends_on = None
+
+
+def upgrade():
+    # the `name` does not have to be unique anymore
+    op.drop_constraint("client_name_key", "client")
+
+
+def downgrade():
+    # remove duplicate rows (rows with the same `name`):
+    # for each client `name`, only keep the row with the latest expiration
+    op.execute(
+        """DELETE FROM client WHERE client_id IN (
+            SELECT client_id FROM (
+                SELECT client_id, ROW_NUMBER() OVER(PARTITION BY name ORDER BY expires_at DESC)
+                AS row_num FROM client
+            ) dup where dup.row_num > 1
+        )"""
+    )
+
+    # the `name` must be unique
+    op.create_unique_constraint("client_name_key", "client", ["name"])
diff --git a/tests/migrations/test_a04a70296688.py b/tests/migrations/test_a04a70296688.py
new file mode 100644
index 000000000..ab3560420
--- /dev/null
+++ b/tests/migrations/test_a04a70296688.py
@@ -0,0 +1,148 @@
+"""
+"Non-unique client name" migration
+"""
+
+from alembic.config import main as alembic_main
+import pytest
+from sqlalchemy.exc import IntegrityError
+
+from fence.models import Client
+from fence.utils import random_str
+
+
+@pytest.fixture(scope="function", autouse=True)
+def post_test_clean_up(app):
+    yield
+
+    # clean up the client table
+    with app.db.session as db_session:
+        db_session.query(Client).delete()
+
+    # go back to the latest state of the DB
+    alembic_main(["--raiseerr", "upgrade", "head"])
+
+
+def test_upgrade(app):
+    # state before migration
+    alembic_main(["--raiseerr", "downgrade", "ea7e1b843f82"])
+
+    client_name = "non_unique_client_name"
+
+    # before the migration, it should not be possible to create 2 clients
+    # with the same name
+    with app.db.session as db_session:
+        db_session.add(
+            Client(
+                name=client_name,
+                client_id="client_id1",
+                grant_types="client_credentials",
+            )
+        )
+        db_session.add(
+            Client(
+                name=client_name,
+                client_id="client_id2",
+                grant_types="client_credentials",
+            )
+        )
+        with pytest.raises(IntegrityError):
+            db_session.commit()
+        db_session.rollback()
+
+    # run the upgrade migration
+    alembic_main(["--raiseerr", "upgrade", "a04a70296688"])
+
+    # now it should be possible
+    with app.db.session as db_session:
+        db_session.add(
+            Client(
+                name=client_name,
+                client_id="client_id1",
+                grant_types="client_credentials",
+            )
+        )
+        db_session.add(
+            Client(
+                name=client_name,
+                client_id="client_id2",
+                grant_types="client_credentials",
+            )
+        )
+        db_session.commit()
+        query_result = db_session.query(Client).all()
+
+    # make sure the client was created
+    assert len(query_result) == 2, query_result
+    assert query_result[0].name == client_name
+    assert query_result[1].name == client_name
+
+
+@pytest.mark.parametrize("expirations", [[1, 100], [0, 0], [0, 100]])
+def test_downgrade(app, expirations):
+    """
+    Test the downgrade with the following expiration values:
+    - 1 and 100: we keep the row with the highest expiration (100)
+    - 0 and 0: both rows have no expiration: we keep any of the 2
+    - 0 and 100: we keep the row that has an expiration (100)
+    """
+    # state after migration
+    alembic_main(["--raiseerr", "downgrade", "a04a70296688"])
+
+    client_name = "non_unique_client_name"
+
+    # it should be possible to create 2 clients with the same name
+    with app.db.session as db_session:
+        db_session.add(
+            Client(
+                name=client_name,
+                client_id="client_id1",
+                grant_types="client_credentials",
+                expires_in=expirations[0],
+            )
+        )
+        db_session.add(
+            Client(
+                name=client_name,
+                client_id="client_id2",
+                grant_types="client_credentials",
+                expires_in=expirations[1],
+            )
+        )
+        db_session.commit()
+        query_result = db_session.query(Client).all()
+
+    assert len(query_result) == 2, query_result
+    assert query_result[0].name == client_name
+    expires_at1 = query_result[0].expires_at
+    assert query_result[1].name == client_name
+    expires_at2 = query_result[1].expires_at
+
+    # run the downgrade migration
+    alembic_main(["--raiseerr", "downgrade", "ea7e1b843f82"])
+
+    # the duplicate row with the lowest expiration should have been deleted
+    with app.db.session as db_session:
+        query_result = db_session.query(Client).all()
+    assert len(query_result) == 1, query_result
+    assert query_result[0].name == client_name
+    assert query_result[0].expires_at == max(expires_at1, expires_at2)
+
+    # now it should not be possible anymore to create 2 clients with the same name
+    with app.db.session as db_session:
+        db_session.add(
+            Client(
+                name=client_name,
+                client_id="client_id1",
+                grant_types="client_credentials",
+            )
+        )
+        db_session.add(
+            Client(
+                name=client_name,
+                client_id="client_id2",
+                grant_types="client_credentials",
+            )
+        )
+        with pytest.raises(IntegrityError):
+            db_session.commit()
+        db_session.rollback()
diff --git a/tests/migrations/test_ea7e1b843f82.py b/tests/migrations/test_ea7e1b843f82.py
index 8898f4caf..57c9e129d 100644
--- a/tests/migrations/test_ea7e1b843f82.py
+++ b/tests/migrations/test_ea7e1b843f82.py
@@ -7,7 +7,6 @@
 from sqlalchemy.exc import IntegrityError
 
 from fence.models import Client
-from fence.utils import random_str
 
 
 @pytest.fixture(scope="function", autouse=True)

From d3fae2907eba2634de412ad3e432dc57193924a7 Mon Sep 17 00:00:00 2001
From: Pauline <4224001+paulineribeyre@users.noreply.github.com>
Date: Thu, 12 Jan 2023 16:49:27 -0600
Subject: [PATCH 2/9] test client name uniqueness at creation

---
 .secrets.baseline                    |  6 +++---
 fence/utils.py                       |  1 +
 tests/scripting/test_fence-create.py | 26 ++++++++++++++++++++++++++
 3 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/.secrets.baseline b/.secrets.baseline
index fe56de184..2919df9e8 100644
--- a/.secrets.baseline
+++ b/.secrets.baseline
@@ -198,7 +198,7 @@
         "filename": "fence/utils.py",
         "hashed_secret": "8318df9ecda039deac9868adf1944a29a95c7114",
         "is_verified": false,
-        "line_number": 112
+        "line_number": 113
       }
     ],
     "migrations/versions/a04a70296688_non_unique_client_name.py": [
@@ -355,7 +355,7 @@
         "filename": "tests/scripting/test_fence-create.py",
         "hashed_secret": "e5e9fa1ba31ecd1ae84f75caaa474f3a663f05f4",
         "is_verified": false,
-        "line_number": 264
+        "line_number": 290
       }
     ],
     "tests/test-fence-config.yaml": [
@@ -368,5 +368,5 @@
       }
     ]
   },
-  "generated_at": "2023-01-12T22:35:50Z"
+  "generated_at": "2023-01-12T22:49:18Z"
 }
diff --git a/fence/utils.py b/fence/utils.py
index 28fb995e9..27cd52bc3 100644
--- a/fence/utils.py
+++ b/fence/utils.py
@@ -86,6 +86,7 @@ def create_client(
             if arborist is not None:
                 arborist.delete_client(client_id)
             raise Exception("client {} already exists".format(name))
+
         client = Client(
             client_id=client_id,
             client_secret=hashed_secret,
diff --git a/tests/scripting/test_fence-create.py b/tests/scripting/test_fence-create.py
index 1121383d8..107fbe844 100644
--- a/tests/scripting/test_fence-create.py
+++ b/tests/scripting/test_fence-create.py
@@ -253,6 +253,32 @@ def to_test():
         )
 
 
+def test_create_client_duplicate_name(db_session):
+    """
+    Test that we can't create a new client with the same name as an existing client.
+    """
+    client_name = "non_unique_client_name"
+
+    create_client_action(
+        config["DB"],
+        client=client_name,
+        username="exampleuser",
+        urls=["https://localhost"],
+        grant_types=["authorization_code"],
+    )
+    saved_client = db_session.query(Client).filter_by(name=client_name).first()
+    assert saved_client.name == client_name
+
+    with pytest.raises(Exception, match=f"client {client_name} already exists"):
+        create_client_action(
+            config["DB"],
+            client=client_name,
+            username="exampleuser",
+            urls=["https://localhost"],
+            grant_types=["authorization_code"],
+        )
+
+
 def test_client_delete(app, db_session, cloud_manager, test_user_a):
     """
     Test that the client delete function correctly cleans up the client's

From 5614cb7484d5604bd7977bc9ba38f84367ea5472 Mon Sep 17 00:00:00 2001
From: Pauline <4224001+paulineribeyre@users.noreply.github.com>
Date: Thu, 12 Jan 2023 16:49:51 -0600
Subject: [PATCH 3/9] black

---
 fence/config.py                      | 8 ++++++--
 fence/resources/openid/idp_oauth2.py | 2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/fence/config.py b/fence/config.py
index ca149991d..9551a8418 100644
--- a/fence/config.py
+++ b/fence/config.py
@@ -51,10 +51,14 @@ def post_process(self):
 
         # allow setting DB connection string via env var
         if os.environ.get("DB"):
-            logger.info("Found environment variable 'DB': overriding 'DB' field from config file")
+            logger.info(
+                "Found environment variable 'DB': overriding 'DB' field from config file"
+            )
             self["DB"] = os.environ["DB"]
         else:
-            logger.info("Environment variable 'DB' empty or not set: using 'DB' field from config file")
+            logger.info(
+                "Environment variable 'DB' empty or not set: using 'DB' field from config file"
+            )
 
         if "ROOT_URL" not in self._configs and "BASE_URL" in self._configs:
             url = urllib.parse.urlparse(self._configs["BASE_URL"])
diff --git a/fence/resources/openid/idp_oauth2.py b/fence/resources/openid/idp_oauth2.py
index 28201ffcd..2eb77d9e7 100644
--- a/fence/resources/openid/idp_oauth2.py
+++ b/fence/resources/openid/idp_oauth2.py
@@ -187,7 +187,7 @@ def get_access_token(self, user, token_endpoint, db_session=None):
         expires = None
 
         # get refresh_token and expiration from db
-        for row in sorted(user.upstream_refresh_tokens, key=lambda row:row.expires):
+        for row in sorted(user.upstream_refresh_tokens, key=lambda row: row.expires):
             refresh_token = row.refresh_token
             expires = row.expires
 

From dedcee40a3c7cb14b8d9b87021e150e8f9440955 Mon Sep 17 00:00:00 2001
From: Pauline <4224001+paulineribeyre@users.noreply.github.com>
Date: Wed, 18 Jan 2023 14:07:01 -0600
Subject: [PATCH 4/9] fix unit test

---
 .secrets.baseline                    |  4 +--
 tests/scripting/test_fence-create.py | 51 ++++++++++++++++------------
 2 files changed, 32 insertions(+), 23 deletions(-)

diff --git a/.secrets.baseline b/.secrets.baseline
index 2919df9e8..d1d55a9a9 100644
--- a/.secrets.baseline
+++ b/.secrets.baseline
@@ -355,7 +355,7 @@
         "filename": "tests/scripting/test_fence-create.py",
         "hashed_secret": "e5e9fa1ba31ecd1ae84f75caaa474f3a663f05f4",
         "is_verified": false,
-        "line_number": 290
+        "line_number": 299
       }
     ],
     "tests/test-fence-config.yaml": [
@@ -368,5 +368,5 @@
       }
     ]
   },
-  "generated_at": "2023-01-12T22:49:18Z"
+  "generated_at": "2023-01-18T20:11:19Z"
 }
diff --git a/tests/scripting/test_fence-create.py b/tests/scripting/test_fence-create.py
index 107fbe844..35fd9a33d 100644
--- a/tests/scripting/test_fence-create.py
+++ b/tests/scripting/test_fence-create.py
@@ -62,6 +62,19 @@ def mock_arborist(mock_arborist_requests):
     mock_arborist_requests()
 
 
+def delete_client_if_exists(db, client_name, username=None):
+    driver = SQLAlchemyDriver(db)
+    with driver.session as session:
+        client = session.query(Client).filter_by(name=client_name).first()
+        if client is not None:
+            session.delete(client)
+        if username:
+            user = session.query(User).filter_by(username=username).first()
+            if user is not None:
+                session.delete(user)
+        session.commit()
+
+
 def create_client_action_wrapper(
     to_test,
     db=None,
@@ -87,15 +100,7 @@ def create_client_action_wrapper(
         **kwargs,
     )
     to_test()
-    driver = SQLAlchemyDriver(db)
-    with driver.session as session:
-        client = session.query(Client).filter_by(name=client_name).first()
-        user = session.query(User).filter_by(username=username).first()
-        if client is not None:
-            session.delete(client)
-        if user is not None:
-            session.delete(user)
-        session.commit()
+    delete_client_if_exists(db, client_name, username)
 
 
 def test_create_client_inits_default_allowed_scopes(db_session):
@@ -258,18 +263,8 @@ def test_create_client_duplicate_name(db_session):
     Test that we can't create a new client with the same name as an existing client.
     """
     client_name = "non_unique_client_name"
-
-    create_client_action(
-        config["DB"],
-        client=client_name,
-        username="exampleuser",
-        urls=["https://localhost"],
-        grant_types=["authorization_code"],
-    )
-    saved_client = db_session.query(Client).filter_by(name=client_name).first()
-    assert saved_client.name == client_name
-
-    with pytest.raises(Exception, match=f"client {client_name} already exists"):
+    try:
+        # successfully create a client
         create_client_action(
             config["DB"],
             client=client_name,
@@ -277,6 +272,20 @@ def test_create_client_duplicate_name(db_session):
             urls=["https://localhost"],
             grant_types=["authorization_code"],
         )
+        saved_client = db_session.query(Client).filter_by(name=client_name).first()
+        assert saved_client.name == client_name
+
+        # we should fail to create a 2nd client with the same name
+        with pytest.raises(Exception, match=f"client {client_name} already exists"):
+            create_client_action(
+                config["DB"],
+                client=client_name,
+                username="exampleuser",
+                urls=["https://localhost"],
+                grant_types=["authorization_code"],
+            )
+    finally:
+        delete_client_if_exists(config["DB"], client_name)
 
 
 def test_client_delete(app, db_session, cloud_manager, test_user_a):

From 94807c590a7bdde56648d31d4ac1a11b2144cc9f Mon Sep 17 00:00:00 2001
From: Pauline <4224001+paulineribeyre@users.noreply.github.com>
Date: Wed, 18 Jan 2023 14:47:00 -0600
Subject: [PATCH 5/9] add client-rotate command

---
 .secrets.baseline                    |  6 ++--
 bin/fence_create.py                  | 11 +++++++
 fence/scripting/fence_create.py      | 37 ++++++++++++++++++++++-
 fence/utils.py                       | 21 ++++++++-----
 tests/scripting/test_fence-create.py | 45 ++++++++++++++++++++++++++++
 5 files changed, 108 insertions(+), 12 deletions(-)

diff --git a/.secrets.baseline b/.secrets.baseline
index d1d55a9a9..b8aaf0899 100644
--- a/.secrets.baseline
+++ b/.secrets.baseline
@@ -198,7 +198,7 @@
         "filename": "fence/utils.py",
         "hashed_secret": "8318df9ecda039deac9868adf1944a29a95c7114",
         "is_verified": false,
-        "line_number": 113
+        "line_number": 118
       }
     ],
     "migrations/versions/a04a70296688_non_unique_client_name.py": [
@@ -355,7 +355,7 @@
         "filename": "tests/scripting/test_fence-create.py",
         "hashed_secret": "e5e9fa1ba31ecd1ae84f75caaa474f3a663f05f4",
         "is_verified": false,
-        "line_number": 299
+        "line_number": 300
       }
     ],
     "tests/test-fence-config.yaml": [
@@ -368,5 +368,5 @@
       }
     ]
   },
-  "generated_at": "2023-01-18T20:11:19Z"
+  "generated_at": "2023-01-18T20:46:53Z"
 }
diff --git a/bin/fence_create.py b/bin/fence_create.py
index cec5b2337..7adaedd82 100755
--- a/bin/fence_create.py
+++ b/bin/fence_create.py
@@ -17,6 +17,7 @@
     create_sample_data,
     delete_client_action,
     delete_expired_clients_action,
+    rotate_client_action,
     delete_users,
     delete_expired_google_access,
     cleanup_expired_ga4gh_information,
@@ -163,6 +164,14 @@ def parse_arguments():
         default=7,
     )
 
+    client_rotate = subparsers.add_parser("client-rotate")
+    client_rotate.add_argument("--client", required=True)
+    client_rotate.add_argument(
+        "--expires-in",
+        help="days until the new client credentials expire",
+        required=False,
+    )
+
     user_delete = subparsers.add_parser("user-delete")
     user_delete.add_argument("--users", required=True, nargs="+")
 
@@ -477,6 +486,8 @@ def main():
         list_client_action(DB)
     elif args.action == "client-delete-expired":
         delete_expired_clients_action(DB, args.slack_webhook, args.warning_days)
+    elif args.action == "client-rotate":
+        rotate_client_action(DB, args.client, args.expires_in)
     elif args.action == "user-delete":
         delete_users(DB, args.users)
     elif args.action == "expired-service-account-delete":
diff --git a/fence/scripting/fence_create.py b/fence/scripting/fence_create.py
index 3e4556ac7..63c2d6a0c 100644
--- a/fence/scripting/fence_create.py
+++ b/fence/scripting/fence_create.py
@@ -57,7 +57,7 @@
 from fence.scripting.google_monitor import email_users_without_access, validation_check
 from fence.config import config
 from fence.sync.sync_users import UserSyncer
-from fence.utils import create_client, get_valid_expiration
+from fence.utils import create_client, get_valid_expiration, generate_client_credentials
 
 from gen3authz.client.arborist.client import ArboristClient
 
@@ -276,6 +276,41 @@ def split_uris(uris):
             logger.info(nothing_to_do_msg)
 
 
+def rotate_client_action(DB, client_name, expires_in=None):
+    driver = SQLAlchemyDriver(DB)
+    with driver.session as s:
+        client = s.query(Client).filter(Client.name == client_name).first()
+        if not client:
+            raise Exception("client {} does not exist".format(client_name))
+
+        # create a new row in the DB for the same client, with a new ID, secret and expiration
+        client_id, client_secret, hashed_secret = generate_client_credentials(
+            client.is_confidential
+        )
+        client = Client(
+            client_id=client_id,
+            client_secret=hashed_secret,
+            expires_in=expires_in,
+            # the rest is identical to the client being rotated
+            user=client.user,
+            redirect_uris=client.redirect_uris,
+            _allowed_scopes=client._allowed_scopes,
+            description=client.description,
+            name=client.name,
+            auto_approve=client.auto_approve,
+            grant_types=client.grant_types,
+            is_confidential=client.is_confidential,
+            token_endpoint_auth_method=client.token_endpoint_auth_method,
+        )
+        s.add(client)
+        s.commit()
+
+    res = (client_id, client_secret)
+    print(
+        f"\nSave these credentials! Fence will not save the unhashed client secret.\nclient id, client secret:\n{res}"
+    )
+
+
 def _remove_client_service_accounts(db_session, client):
     client_service_accounts = (
         db_session.query(GoogleServiceAccount)
diff --git a/fence/utils.py b/fence/utils.py
index 27cd52bc3..ef39dda68 100644
--- a/fence/utils.py
+++ b/fence/utils.py
@@ -34,6 +34,18 @@ def json_res(data):
     return flask.Response(json.dumps(data), mimetype="application/json")
 
 
+def generate_client_credentials(confidential):
+    client_id = random_str(40)
+    client_secret = None
+    hashed_secret = None
+    if confidential:
+        client_secret = random_str(55)
+        hashed_secret = bcrypt.hashpw(
+            client_secret.encode("utf-8"), bcrypt.gensalt()
+        ).decode("utf-8")
+    return client_id, client_secret, hashed_secret
+
+
 def create_client(
     DB,
     username=None,
@@ -49,17 +61,10 @@ def create_client(
     allowed_scopes=None,
     expires_in=None,
 ):
-    client_id = random_str(40)
+    client_id, client_secret, hashed_secret = generate_client_credentials(confidential)
     if arborist is not None:
         arborist.create_client(client_id, policies)
     driver = SQLAlchemyDriver(DB)
-    client_secret = None
-    hashed_secret = None
-    if confidential:
-        client_secret = random_str(55)
-        hashed_secret = bcrypt.hashpw(
-            client_secret.encode("utf-8"), bcrypt.gensalt()
-        ).decode("utf-8")
     auth_method = "client_secret_basic" if confidential else "none"
 
     allowed_scopes = allowed_scopes or config["CLIENT_ALLOWED_SCOPES"]
diff --git a/tests/scripting/test_fence-create.py b/tests/scripting/test_fence-create.py
index 35fd9a33d..66e5f1edb 100644
--- a/tests/scripting/test_fence-create.py
+++ b/tests/scripting/test_fence-create.py
@@ -38,6 +38,7 @@
     create_client_action,
     delete_client_action,
     delete_expired_clients_action,
+    rotate_client_action,
     delete_expired_service_accounts,
     delete_expired_google_access,
     link_external_bucket,
@@ -459,6 +460,50 @@ def test_client_delete_expired(app, db_session, cloud_manager, post_to_slack):
     assert len(client_sa_after) == 0
 
 
+def test_client_rotate(db_session):
+    """
+    Create a client, rotate it and check that the 2 rows in the DB are identical except
+    for the client ID, secret and expiration.
+    """
+    client_name = "client_abc"
+
+    create_client_action(
+        config["DB"],
+        client=client_name,
+        username="exampleuser",
+        urls=["https://localhost"],
+        grant_types=["authorization_code"],
+        expires_in=30,
+    )
+    clients = db_session.query(Client).filter_by(name=client_name).all()
+    assert len(clients) == 1
+    assert clients[0].name == client_name
+
+    rotate_client_action(config["DB"], client_name, 20)
+
+    clients = db_session.query(Client).filter_by(name=client_name).all()
+    assert len(clients) == 2
+
+    assert clients[0].name == client_name
+    assert clients[1].name == client_name
+    for attr in [
+        "user",
+        "redirect_uris",
+        "_allowed_scopes",
+        "description",
+        "auto_approve",
+        "grant_types",
+        "is_confidential",
+        "token_endpoint_auth_method",
+    ]:
+        assert getattr(clients[0], attr) == getattr(
+            clients[1], attr
+        ), f"attribute '{attr}' differs"
+    assert clients[0].client_id != clients[1].client_id
+    assert clients[0].client_secret != clients[1].client_secret
+    assert clients[0].expires_at != clients[1].expires_at
+
+
 def test_delete_users(app, db_session, example_usernames):
     """
     Test the basic functionality of ``delete_users``.

From 03000a945cb311fe3a2099973556a3d5d3618b1e Mon Sep 17 00:00:00 2001
From: Pauline <4224001+paulineribeyre@users.noreply.github.com>
Date: Wed, 18 Jan 2023 14:57:54 -0600
Subject: [PATCH 6/9] update readme

---
 README.md | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/README.md b/README.md
index fb4da28d3..59ffa0f31 100644
--- a/README.md
+++ b/README.md
@@ -552,6 +552,14 @@ Add `--append` argument to add new callback urls or allowed scopes to existing c
 fence-create client-modify --client CLIENT_NAME --urls http://localhost/api/v0/new/oauth2/authorize --append (--expires-in 30)
 ```
 
+#### Rotate client credentials
+
+Use the `client-rotate` command to receive a new set of credentials for a client. The old credentials are NOT deactivated and must be deleted or expired separately. This allows for a rotation without downtime.
+
+```bash
+fence-create client-rotate --client CLIENT_NAME (--expires-in 30)
+```
+
 #### Delete OAuth Client
 
 ```bash

From dda4a8b5b8820a810bdf75eb6ddad4b8be22d633 Mon Sep 17 00:00:00 2001
From: Pauline <4224001+paulineribeyre@users.noreply.github.com>
Date: Fri, 20 Jan 2023 11:35:23 -0600
Subject: [PATCH 7/9] fix migration

---
 .../a04a70296688_non_unique_client_name.py      | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/migrations/versions/a04a70296688_non_unique_client_name.py b/migrations/versions/a04a70296688_non_unique_client_name.py
index 2a1bb1185..c13c1c6cc 100644
--- a/migrations/versions/a04a70296688_non_unique_client_name.py
+++ b/migrations/versions/a04a70296688_non_unique_client_name.py
@@ -16,8 +16,19 @@
 
 
 def upgrade():
-    # the `name` does not have to be unique anymore
-    op.drop_constraint("client_name_key", "client")
+    # the `name` does not have to be unique anymore: remove the constraint
+    connection = op.get_bind()
+    results = connection.execute(
+        "SELECT conname FROM pg_constraint WHERE conrelid = 'client'::regclass and contype = 'u'"
+    )
+    name_constraints = [e[0] for e in results if "name" in e[0]]
+    if len(name_constraints) != 1:
+        raise Exception(
+            f"Found multiple 'unique client name' constraints: {name_constraints}"
+        )
+
+    print(f"Droppping 'unique client name' constraint: '{name_constraints[0]}'")
+    op.drop_constraint(name_constraints[0], "client")
 
 
 def downgrade():
@@ -33,4 +44,4 @@ def downgrade():
     )
 
     # the `name` must be unique
-    op.create_unique_constraint("client_name_key", "client", ["name"])
+    op.create_unique_constraint("unique_client_name", "client", ["name"])

From 14c84fec4fa12409fcc80f8c5f310f7f85edc20f Mon Sep 17 00:00:00 2001
From: Pauline <4224001+paulineribeyre@users.noreply.github.com>
Date: Thu, 26 Jan 2023 13:34:11 -0600
Subject: [PATCH 8/9] add docstrings

---
 .secrets.baseline                                   |  4 ++--
 deployment/uwsgi/uwsgi.ini                          |  1 -
 fence/scripting/fence_create.py                     | 13 +++++++++++++
 fence/utils.py                                      | 11 +++++++++++
 .../versions/a04a70296688_non_unique_client_name.py |  5 ++++-
 5 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/.secrets.baseline b/.secrets.baseline
index 0a16e9733..90cde413f 100644
--- a/.secrets.baseline
+++ b/.secrets.baseline
@@ -198,7 +198,7 @@
         "filename": "fence/utils.py",
         "hashed_secret": "8318df9ecda039deac9868adf1944a29a95c7114",
         "is_verified": false,
-        "line_number": 118
+        "line_number": 129
       }
     ],
     "migrations/versions/a04a70296688_non_unique_client_name.py": [
@@ -368,5 +368,5 @@
       }
     ]
   },
-  "generated_at": "2023-01-23T20:39:16Z"
+  "generated_at": "2023-01-26T19:34:06Z"
 }
diff --git a/deployment/uwsgi/uwsgi.ini b/deployment/uwsgi/uwsgi.ini
index a2f7741c4..0ebedb7ce 100644
--- a/deployment/uwsgi/uwsgi.ini
+++ b/deployment/uwsgi/uwsgi.ini
@@ -22,7 +22,6 @@ plugins = python3
 vacuum = true
 pythonpath = /var/www/fence/
 pythonpath = /fence/
-pythonpath = /usr/local/lib/python3.6/site-packages/
 # poetry installs git dependencies at /usr/local/src
 pythonpath = /usr/local/src/*
 
diff --git a/fence/scripting/fence_create.py b/fence/scripting/fence_create.py
index 63c2d6a0c..767feebee 100644
--- a/fence/scripting/fence_create.py
+++ b/fence/scripting/fence_create.py
@@ -277,6 +277,19 @@ def split_uris(uris):
 
 
 def rotate_client_action(DB, client_name, expires_in=None):
+    """
+    Rorate a client's credentials (client ID and secret). The old credentials are
+    NOT deactivated and must be deleted or expired separately. This allows for a
+    rotation without downtime.
+
+    Args:
+        DB (str): database connection string
+        client_name (str): name of the client to rotate credentials for
+        expires_in (optional): number of days until this client expires (by default, no expiration)
+
+    Returns:
+        This functions does not return anything, but it prints the new set of credentials.
+    """
     driver = SQLAlchemyDriver(DB)
     with driver.session as s:
         client = s.query(Client).filter(Client.name == client_name).first()
diff --git a/fence/utils.py b/fence/utils.py
index ef39dda68..1f5f3225a 100644
--- a/fence/utils.py
+++ b/fence/utils.py
@@ -35,6 +35,17 @@ def json_res(data):
 
 
 def generate_client_credentials(confidential):
+    """
+    Generate a new client ID. If the client is confidential, also generate a new client secret.
+    The unhashed secret should be returned to the user and the hashed secret should be stored
+    in the database for later use.
+
+    Args:
+        confidential (bool): true if the client is confidential, false if it is public
+
+    Returns:
+        tuple: (client ID, unhashed client secret or None, hashed client secret or None)
+    """
     client_id = random_str(40)
     client_secret = None
     hashed_secret = None
diff --git a/migrations/versions/a04a70296688_non_unique_client_name.py b/migrations/versions/a04a70296688_non_unique_client_name.py
index c13c1c6cc..83b74fae1 100644
--- a/migrations/versions/a04a70296688_non_unique_client_name.py
+++ b/migrations/versions/a04a70296688_non_unique_client_name.py
@@ -16,17 +16,20 @@
 
 
 def upgrade():
-    # the `name` does not have to be unique anymore: remove the constraint
+    # get all "unique" constraints on "client" table
     connection = op.get_bind()
     results = connection.execute(
         "SELECT conname FROM pg_constraint WHERE conrelid = 'client'::regclass and contype = 'u'"
     )
+
+    # filter out the constraints that are not for the "name" column; only 1 constraint should be left
     name_constraints = [e[0] for e in results if "name" in e[0]]
     if len(name_constraints) != 1:
         raise Exception(
             f"Found multiple 'unique client name' constraints: {name_constraints}"
         )
 
+    # the `name` does not have to be unique anymore: remove the constraint
     print(f"Droppping 'unique client name' constraint: '{name_constraints[0]}'")
     op.drop_constraint(name_constraints[0], "client")
 

From 3fcde12c4023b7b8d31249389b0b0d71f63718c2 Mon Sep 17 00:00:00 2001
From: Pauline <4224001+paulineribeyre@users.noreply.github.com>
Date: Thu, 26 Jan 2023 13:58:55 -0600
Subject: [PATCH 9/9] update other client commands

---
 .secrets.baseline                    |   4 +-
 fence/scripting/fence_create.py      |  73 +++++++++---------
 tests/scripting/test_fence-create.py | 110 ++++++++++++++++++++-------
 3 files changed, 122 insertions(+), 65 deletions(-)

diff --git a/.secrets.baseline b/.secrets.baseline
index 90cde413f..642b6e493 100644
--- a/.secrets.baseline
+++ b/.secrets.baseline
@@ -355,7 +355,7 @@
         "filename": "tests/scripting/test_fence-create.py",
         "hashed_secret": "e5e9fa1ba31ecd1ae84f75caaa474f3a663f05f4",
         "is_verified": false,
-        "line_number": 300
+        "line_number": 301
       }
     ],
     "tests/test-fence-config.yaml": [
@@ -368,5 +368,5 @@
       }
     ]
   },
-  "generated_at": "2023-01-26T19:34:06Z"
+  "generated_at": "2023-01-26T20:09:03Z"
 }
diff --git a/fence/scripting/fence_create.py b/fence/scripting/fence_create.py
index 767feebee..dd1765a28 100644
--- a/fence/scripting/fence_create.py
+++ b/fence/scripting/fence_create.py
@@ -92,43 +92,44 @@ def modify_client_action(
     driver = SQLAlchemyDriver(DB)
     with driver.session as s:
         client_name = client
-        client = s.query(Client).filter(Client.name == client_name).first()
-        if not client:
+        clients = s.query(Client).filter(Client.name == client_name).all()
+        if not clients:
             raise Exception("client {} does not exist".format(client_name))
-        if urls:
-            if append:
-                client.redirect_uris += urls
-                logger.info("Adding {} to urls".format(urls))
-            else:
-                client.redirect_uris = urls
-                logger.info("Changing urls to {}".format(urls))
-        if delete_urls:
-            client.redirect_uris = []
-            logger.info("Deleting urls")
-        if set_auto_approve:
-            client.auto_approve = True
-            logger.info("Auto approve set to True")
-        if unset_auto_approve:
-            client.auto_approve = False
-            logger.info("Auto approve set to False")
-        if name:
-            client.name = name
-            logger.info("Updating name to {}".format(name))
-        if description:
-            client.description = description
-            logger.info("Updating description to {}".format(description))
-        if allowed_scopes:
-            if append:
-                new_scopes = client._allowed_scopes.split() + allowed_scopes
-                client._allowed_scopes = " ".join(new_scopes)
-                logger.info("Adding {} to allowed_scopes".format(allowed_scopes))
-            else:
-                client._allowed_scopes = " ".join(allowed_scopes)
-                logger.info("Updating allowed_scopes to {}".format(allowed_scopes))
-        if expires_in:
-            client.expires_at = get_client_expires_at(
-                expires_in=expires_in, grant_types=client.grant_type
-            )
+        for client in clients:
+            if urls:
+                if append:
+                    client.redirect_uris += urls
+                    logger.info("Adding {} to urls".format(urls))
+                else:
+                    client.redirect_uris = urls
+                    logger.info("Changing urls to {}".format(urls))
+            if delete_urls:
+                client.redirect_uris = []
+                logger.info("Deleting urls")
+            if set_auto_approve:
+                client.auto_approve = True
+                logger.info("Auto approve set to True")
+            if unset_auto_approve:
+                client.auto_approve = False
+                logger.info("Auto approve set to False")
+            if name:
+                client.name = name
+                logger.info("Updating name to {}".format(name))
+            if description:
+                client.description = description
+                logger.info("Updating description to {}".format(description))
+            if allowed_scopes:
+                if append:
+                    new_scopes = client._allowed_scopes.split() + allowed_scopes
+                    client._allowed_scopes = " ".join(new_scopes)
+                    logger.info("Adding {} to allowed_scopes".format(allowed_scopes))
+                else:
+                    client._allowed_scopes = " ".join(allowed_scopes)
+                    logger.info("Updating allowed_scopes to {}".format(allowed_scopes))
+            if expires_in:
+                client.expires_at = get_client_expires_at(
+                    expires_in=expires_in, grant_types=client.grant_type
+                )
         s.commit()
     if arborist is not None and policies:
         arborist.update_client(client.client_id, policies)
diff --git a/tests/scripting/test_fence-create.py b/tests/scripting/test_fence-create.py
index 66e5f1edb..4bed8f1da 100644
--- a/tests/scripting/test_fence-create.py
+++ b/tests/scripting/test_fence-create.py
@@ -66,9 +66,10 @@ def mock_arborist(mock_arborist_requests):
 def delete_client_if_exists(db, client_name, username=None):
     driver = SQLAlchemyDriver(db)
     with driver.session as session:
-        client = session.query(Client).filter_by(name=client_name).first()
-        if client is not None:
-            session.delete(client)
+        clients = session.query(Client).filter_by(name=client_name).all()
+        if clients:
+            for client in clients:
+                session.delete(client)
         if username:
             user = session.query(User).filter_by(username=username).first()
             if user is not None:
@@ -467,41 +468,96 @@ def test_client_rotate(db_session):
     """
     client_name = "client_abc"
 
+    try:
+        create_client_action(
+            config["DB"],
+            client=client_name,
+            username="exampleuser",
+            urls=["https://localhost"],
+            grant_types=["authorization_code"],
+            expires_in=30,
+        )
+        clients = db_session.query(Client).filter_by(name=client_name).all()
+        assert len(clients) == 1
+        assert clients[0].name == client_name
+
+        rotate_client_action(config["DB"], client_name, 20)
+
+        clients = db_session.query(Client).filter_by(name=client_name).all()
+        assert len(clients) == 2
+
+        assert clients[0].name == client_name
+        assert clients[1].name == client_name
+        for attr in [
+            "user",
+            "redirect_uris",
+            "_allowed_scopes",
+            "description",
+            "auto_approve",
+            "grant_types",
+            "is_confidential",
+            "token_endpoint_auth_method",
+        ]:
+            assert getattr(clients[0], attr) == getattr(
+                clients[1], attr
+            ), f"attribute '{attr}' differs"
+        assert clients[0].client_id != clients[1].client_id
+        assert clients[0].client_secret != clients[1].client_secret
+        assert clients[0].expires_at != clients[1].expires_at
+    finally:
+        delete_client_if_exists(config["DB"], client_name)
+
+
+def test_client_rotate_and_actions(db_session, capsys):
+    """
+    Check that listing, modifying or deleting a client (after rotating it) affects
+    all of this client's rows in the DB.
+    """
+    client_name = "client_abc"
+
+    # create a client and rotate the credentials twice
+    url1 = "https://localhost"
     create_client_action(
         config["DB"],
         client=client_name,
         username="exampleuser",
-        urls=["https://localhost"],
+        urls=[url1],
         grant_types=["authorization_code"],
         expires_in=30,
     )
+    rotate_client_action(config["DB"], client_name, 20)
+    rotate_client_action(config["DB"], client_name, 10)
+
+    # this should result in 3 rows for this client in the DB
     clients = db_session.query(Client).filter_by(name=client_name).all()
-    assert len(clients) == 1
-    assert clients[0].name == client_name
+    assert len(clients) == 3
+    for i in range(3):
+        assert clients[i].name == client_name
 
-    rotate_client_action(config["DB"], client_name, 20)
+    # check that `list_client_action` lists all the rows
+    capsys.readouterr()  # clear the buffer
+    list_client_action(db_session)
+    captured_logs = str(capsys.readouterr())
+    assert captured_logs.count("'name': 'client_abc'") == 3
+    for i in range(3):
+        assert captured_logs.count(f"'client_id': '{clients[i].client_id}'") == 1
+
+    # check that `modify_client_action` updates all the rows
+    description = "new description"
+    url2 = "new url"
+    modify_client_action(
+        db_session, client_name, description=description, urls=[url2], append=True
+    )
+    clients = db_session.query(Client).filter_by(name=client_name).all()
+    assert len(clients) == 3
+    for i in range(3):
+        assert clients[i].description == description
+        assert clients[i].redirect_uri == f"{url1}\n{url2}"
 
+    # check that `delete_client_action` deletes all the rows
+    delete_client_action(config["DB"], client_name)
     clients = db_session.query(Client).filter_by(name=client_name).all()
-    assert len(clients) == 2
-
-    assert clients[0].name == client_name
-    assert clients[1].name == client_name
-    for attr in [
-        "user",
-        "redirect_uris",
-        "_allowed_scopes",
-        "description",
-        "auto_approve",
-        "grant_types",
-        "is_confidential",
-        "token_endpoint_auth_method",
-    ]:
-        assert getattr(clients[0], attr) == getattr(
-            clients[1], attr
-        ), f"attribute '{attr}' differs"
-    assert clients[0].client_id != clients[1].client_id
-    assert clients[0].client_secret != clients[1].client_secret
-    assert clients[0].expires_at != clients[1].expires_at
+    assert len(clients) == 0
 
 
 def test_delete_users(app, db_session, example_usernames):