diff --git a/fence/auth.py b/fence/auth.py index e7c1a1fb8..3861951f8 100644 --- a/fence/auth.py +++ b/fence/auth.py @@ -101,9 +101,8 @@ def set_flask_session_values(user): user = query_for_user(session=current_app.scoped_session(), username=username) if user: - if user.active is False: - # Abort login if user.active is False (user.active is None or True are both - # considered active in this case): + if user.active == False: + # Abort login if user.active == False: raise Unauthorized( "User is known but not authorized/activated in the system" ) diff --git a/fence/blueprints/admin.py b/fence/blueprints/admin.py index ed64a9272..a34b385ca 100644 --- a/fence/blueprints/admin.py +++ b/fence/blueprints/admin.py @@ -78,7 +78,7 @@ def create_user(): Returns a json object """ - username = request.get_json().get("name", None) + username = request.get_json().get("username", None) role = request.get_json().get("role", None) email = request.get_json().get("email", None) display_name = request.get_json().get("display_name", None) @@ -110,11 +110,13 @@ def update_user(username): Returns a json object """ - name = request.get_json().get("name", None) + new_username = request.get_json().get("username", None) role = request.get_json().get("role", None) email = request.get_json().get("email", None) return jsonify( - admin.update_user(current_app.scoped_session(), username, role, email, name) + admin.update_user( + current_app.scoped_session(), username, role, email, new_username + ) ) @@ -133,6 +135,20 @@ def delete_user(username): return response +@blueprint.route("/users//soft", methods=["DELETE"]) +@blueprint.route("/user//soft", methods=["DELETE"]) +@admin_login_required +@debug_log +def soft_delete_user(username): + """ + Soft-remove the user by marking it as active=False. + + Returns json object + """ + response = jsonify(admin.soft_delete_user(current_app.scoped_session(), username)) + return response + + @blueprint.route("/users//groups", methods=["GET"]) @blueprint.route("/user//groups", methods=["GET"]) @admin_login_required diff --git a/fence/resources/admin/admin_users.py b/fence/resources/admin/admin_users.py index 956da6cd7..37b586476 100644 --- a/fence/resources/admin/admin_users.py +++ b/fence/resources/admin/admin_users.py @@ -28,6 +28,7 @@ "update_user", "add_user_to_projects", "delete_user", + "soft_delete_user", "add_user_to_groups", "connect_user_to_group", "remove_user_from_groups", @@ -77,7 +78,7 @@ def get_all_users(current_session): users_names = [] for user in users: new_user = {} - new_user["name"] = user.username + new_user["username"] = user.username if user.is_admin: new_user["role"] = "admin" else: @@ -123,7 +124,7 @@ def create_user( except NotFound: logger.debug(f"User not found for: {username}. Checking again ignoring case...") user_list = [ - user["name"].upper() for user in get_all_users(current_session)["users"] + user["username"].upper() for user in get_all_users(current_session)["users"] ] if username.upper() in user_list: logger.debug(f"User already exists for: {username}") @@ -167,7 +168,7 @@ def create_user( def update_user(current_session, username, role, email, new_name): usr = us.get_user(current_session, username) user_list = [ - user["name"].upper() for user in get_all_users(current_session)["users"] + user["username"].upper() for user in get_all_users(current_session)["users"] ] if ( new_name @@ -359,6 +360,17 @@ def raise_unavailable(gpg_email): logger.info("Done with Google deletions.") +def soft_delete_user(current_session, username): + """ + Soft-remove the user by marking it as active=False. + """ + logger.debug(f"Soft-delete user '{username}'") + usr = us.get_user(current_session, username) + usr.active = False + current_session.commit() + return us.get_user_info(current_session, usr.username) + + def delete_user(current_session, username): """ Remove a user from both the userdatamodel diff --git a/fence/resources/user/__init__.py b/fence/resources/user/__init__.py index 3543f875f..152e75057 100644 --- a/fence/resources/user/__init__.py +++ b/fence/resources/user/__init__.py @@ -91,6 +91,7 @@ def get_user_info(current_session, username): "phone_number": user.phone_number, "email": user.email, "is_admin": user.is_admin, + "active": user.active, "role": role, "project_access": dict(user.project_access), "certificates_uploaded": [], diff --git a/migrations/versions/3a5712474808_make_user_active_field_non_nullable.py b/migrations/versions/3a5712474808_make_user_active_field_non_nullable.py new file mode 100644 index 000000000..cc9aebb51 --- /dev/null +++ b/migrations/versions/3a5712474808_make_user_active_field_non_nullable.py @@ -0,0 +1,36 @@ +"""Make User.active field non nullable + +Revision ID: 3a5712474808 +Revises: 9b3a5a7145d7 +Create Date: 2024-11-08 22:00:41.161934 + +""" +from alembic import op +import sqlalchemy as sa +from userdatamodel.models import User +from sqlalchemy.orm import Session + + +# revision identifiers, used by Alembic. +revision = "3a5712474808" # pragma: allowlist secret +down_revision = "9b3a5a7145d7" # pragma: allowlist secret +branch_labels = None +depends_on = None + + +def upgrade(): + conn = op.get_bind() + session = Session(bind=conn) + session.query(User) + active_users_count = session.query(User).filter(User.active.is_(True)).count() + if active_users_count > 0: + # if we have at least one user where "active" is explicitly set to "True", then we'll assume NULL is to mean "False": + op.execute('UPDATE "User" SET active = False WHERE active IS NULL') + else: + # else, we assume NULL means "True" + op.execute('UPDATE "User" SET active = True WHERE active IS NULL') + op.alter_column("User", "active", nullable=False, server_default="True") + + +def downgrade(): + op.alter_column("User", "active", nullable=True, server_default=None) diff --git a/openapis/swagger.yaml b/openapis/swagger.yaml index 17ea7eb17..7416bddc6 100644 --- a/openapis/swagger.yaml +++ b/openapis/swagger.yaml @@ -387,6 +387,73 @@ paths: '*/*': schema: $ref: '#/components/schemas/UserInfo' + '/admin/user/{username}': + get: + tags: + - 'admin/user' + summary: Return info about a given user + description: Admin method to retrieve info about any given user + parameters: + - name: username + required: true + in: path + description: Username of user to find + schema: + type: string + security: + - OAuth2: + - user + operationId: getUserInfo + responses: + '200': + description: successful operation + content: + '*/*': + schema: + $ref: '#/components/schemas/UserInfo' + '404': + description: Couldn't find user + '/admin/user/{username}/soft': + delete: + tags: + - 'admin/user' + summary: Soft-delete user + description: Admin method to soft-delete a user, setting the user to inactive + security: + - OAuth2: + - admin + operationId: softDeleteUser + parameters: + - name: username + required: true + in: path + description: Username of user to deactivate + schema: + type: string + responses: + '204': + description: successful deletion + '404': + description: Couldn't find user + /admin/user: + post: + tags: + - admin/user + summary: Add a new user + description: Admin method to add a new user + security: + - OAuth2: + - admin + operationId: createUser + responses: + '200': + description: New user + content: + '*/*': + schema: + $ref: '#/components/schemas/UserInfo' + requestBody: + $ref: '#/components/requestBodies/NewUser' /logout: get: tags: @@ -1683,6 +1750,13 @@ components: $ref: '#/components/schemas/APIKeyScopes' description: API key scopes required: false + NewUser: + content: + application/json: + schema: + $ref: '#/components/schemas/NewUser' + description: New User + required: true securitySchemes: OAuth2: type: oauth2 @@ -1693,6 +1767,46 @@ components: scopes: user: generic user access schemas: + NewUser: + type: object + required: + - username + - role + - email + properties: + username: + type: string + description: 'This value is deprecated in favor of name.' + role: + type: string + description: 'Set to "admin" if the user should be given admin rights. Any other value is not parsed or used, and results in user being a normal/regular user.' + email: + type: string + description: 'The email of the end-user' + display_name: + type: string + description: 'The display name of the end-user.' + phone_number: + type: string + description: 'The phone number of the end-user.' + idp_name: + type: string + description: | + Name of the preferred Identity Provider used to autheticate the user. Given instances of Fence + may or may not have all of these available (the set of IDPs available is a configuration). + * *google* - Google/GSuite + * *ras* - NIH's Researcher Auth Service (RAS) + * *itrust* - NIH Login / iTrust / eRA Commons + * *fence* - Upstream Fence (the idp used depends on the specific configuration, consult the Gen3 Operators) + * *orcid* - ORCHID + * *microsoft* - Microsoft + * *elixir* - Elixir + * *synapse* - Synapse + * *cognito* - AWS Cognito + * More may be added in the future... + tags: + type: object + description: User's tags RequestUploadBlank: type: object required: @@ -2335,6 +2449,9 @@ components: is_admin: type: boolean description: 'Boolean value stating if end-user is an admin or not' + active: + type: boolean + description: 'Boolean value stating if user is active or not' role: type: string description: '' diff --git a/poetry.lock b/poetry.lock index f28981dca..37f05f237 100644 --- a/poetry.lock +++ b/poetry.lock @@ -2424,17 +2424,18 @@ zstd = ["zstandard (>=0.18.0)"] [[package]] name = "userdatamodel" -version = "2.4.3" -description = "" +version = "3.0.1" +description = "PDC data model for user management" optional = false -python-versions = "*" +python-versions = "<4.0,>=3.9" files = [ - {file = "userdatamodel-2.4.3.tar.gz", hash = "sha256:760e41bc7f29bc16b3a9964956b52a3386db971a64dc3c873d362e135adda5b9"}, + {file = "userdatamodel-3.0.1-py3-none-any.whl", hash = "sha256:28e34acf1998e81b32b8e35ac61d950cc48c88ef3545ac45939be0116f20d3d5"}, + {file = "userdatamodel-3.0.1.tar.gz", hash = "sha256:775fa5d11a32c44d11322945a817ae265b7787c90ea05fefcd1742839cca111a"}, ] [package.dependencies] -cdislogging = "*" -sqlalchemy = ">=1.3.3" +cdislogging = ">=1,<2" +sqlalchemy = ">=1.3.0" [[package]] name = "werkzeug" @@ -2582,4 +2583,4 @@ type = ["pytest-mypy"] [metadata] lock-version = "2.0" python-versions = ">=3.9,<4.0.0" -content-hash = "655105408255f1d68d57e3517f8d961b68d1389fe93aed82d1c1daed7042c444" +content-hash = "cfbc8d55dad030a712faa8c311773f99bf60401c614cf93f91e7880b6df568ba" diff --git a/pyproject.toml b/pyproject.toml index 38ed3e908..bb5e567b0 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -47,7 +47,7 @@ pyyaml = "^6.0.1" requests = ">=2.18.0" retry = "^0.9.2" sqlalchemy = "^1.3.3" -userdatamodel = ">=2.4.3" +userdatamodel = ">=3.0.1" werkzeug = ">=3.0.0" cachelib = "^0.2.0" azure-storage-blob = "^12.6.0" diff --git a/tests/admin/test_admin_users.py b/tests/admin/test_admin_users.py index 23543478e..2bc50d6d6 100644 --- a/tests/admin/test_admin_users.py +++ b/tests/admin/test_admin_users.py @@ -17,7 +17,7 @@ def test_get_user(db_session, awg_users): assert "test_group_1" in info["groups"] assert "test_group_2" in info["groups"] assert info["message"] == "" - assert info["email"] == None + assert info["email"] is None assert info["certificates_uploaded"] == [] assert info["resources_granted"] == [] assert info["project_access"]["phs_project_1"] == ["read"] @@ -34,6 +34,7 @@ def test_create_user(db_session, oauth_client): assert user.phone_number is None assert user.identity_provider is None assert len(user.tags) == 0 + assert user.active == True def test_create_user_with_all_fields_set(db_session, oauth_client): @@ -98,6 +99,34 @@ def test_delete_user(db_session, awg_users, cloud_manager): assert user_groups == [] +def test_soft_delete_user(db_session, awg_users): + """ + Tests adm.soft_delete_user() by querying an existing User, + asserting it is not inactive, and then checking it became inactive + after it was soft-deleted. + """ + username = "awg_user" + user = db_session.query(User).filter(User.username == username).first() + assert user != None + assert user.username == username + assert user.active == True + adm.soft_delete_user(db_session, username) + user = db_session.query(User).filter(User.username == username).first() + assert user != None + assert user.username == username + # soft-deleted user should have "active" explicitly set to False now: + assert user.active == False + + +def test_soft_delete_user_not_found(db_session, awg_users): + """ + Check that adm.soft_delete_user() fails with NotFound + when called for a username that is not found in db. + """ + with pytest.raises(NotFound, match="user non_existing_user not found"): + adm.soft_delete_user(db_session, "non_existing_user") + + def test_update_user_without_conflict(db_session, awg_users, oauth_client): user = db_session.query(User).filter(User.username == "awg_user").first() assert user != None @@ -133,7 +162,7 @@ def test_create_already_existing_user(db_session, awg_users): def test_get_all_users(db_session, awg_users): user_list = adm.get_all_users(db_session) - user_name_list = [item["name"] for item in user_list["users"]] + user_name_list = [item["username"] for item in user_list["users"]] assert "awg_user" in user_name_list assert "awg_user_2" in user_name_list diff --git a/tests/admin/test_admin_users_endpoints.py b/tests/admin/test_admin_users_endpoints.py index 8416fb9d0..a598c7b65 100644 --- a/tests/admin/test_admin_users_endpoints.py +++ b/tests/admin/test_admin_users_endpoints.py @@ -249,7 +249,7 @@ def test_get_user( assert r.status_code == 200 # should at least have the users added from above (may have more from other tests) assert len(r.json["users"]) >= 4 - usernames = [user["name"] for user in r.json["users"]] + usernames = [user["username"] for user in r.json["users"]] assert "test_a" in usernames assert "test_b" in usernames assert "test_amazing_user_with_an_fancy_but_extremely_long_name" in usernames @@ -274,7 +274,11 @@ def test_post_user(client, admin_user, encoded_admin_jwt, db_session): "Content-Type": "application/json", }, data=json.dumps( - {"name": "new_test_user", "role": "user", "email": "new_test_user@fake.com"} + { + "username": "new_test_user", + "role": "user", + "email": "new_test_user@fake.com", + } ), ) assert r.status_code == 200 @@ -284,10 +288,12 @@ def test_post_user(client, admin_user, encoded_admin_jwt, db_session): assert r.json["email"] == "new_test_user@fake.com" assert r.json["project_access"] == {} assert r.json["groups"] == [] + assert r.json["active"] == True new_test_user = db_session.query(User).filter_by(username="new_test_user").one() assert new_test_user.username == "new_test_user" assert new_test_user.is_admin == False assert new_test_user.email == "new_test_user@fake.com" + assert new_test_user.active == True def test_post_user_no_fields_defined(client, admin_user, encoded_admin_jwt, db_session): @@ -341,7 +347,7 @@ def test_post_user_only_username_defined( "Authorization": "Bearer " + encoded_admin_jwt, "Content-Type": "application/json", }, - data=json.dumps({"name": "new_test_user"}), + data=json.dumps({"username": "new_test_user"}), ) assert r.status_code == 200 assert r.json["username"] == "new_test_user" @@ -366,7 +372,7 @@ def test_post_user_already_exists( "Authorization": "Bearer " + encoded_admin_jwt, "Content-Type": "application/json", }, - data=json.dumps({"name": "test_a"}), + data=json.dumps({"username": "test_a"}), ) assert r.status_code == 400 @@ -392,7 +398,7 @@ def test_put_user_username( }, data=json.dumps( { - "name": "test_a_updated", + "username": "test_a_updated", "role": "admin", "email": "test_a_updated@fake.com", } @@ -422,7 +428,7 @@ def test_put_user_username_nonexistent( "Authorization": "Bearer " + encoded_admin_jwt, "Content-Type": "application/json", }, - data=json.dumps({"name": "test_nonexistent_updated"}), + data=json.dumps({"username": "test_nonexistent_updated"}), ) assert r.status_code == 404 assert ( @@ -442,7 +448,7 @@ def test_put_user_username_already_exists( "Authorization": "Bearer " + encoded_admin_jwt, "Content-Type": "application/json", }, - data=json.dumps({"name": "test_b"}), + data=json.dumps({"username": "test_b"}), ) assert r.status_code == 400 assert db_session.query(User).filter_by(username="test_a").one() @@ -455,7 +461,7 @@ def test_put_user_username_try_delete_username( """PUT /users/: [update_user] try to delete username""" """ This probably shouldn't be allowed. Conveniently, the code flow ends up - the same as though the user had not tried to update 'name' at all, + the same as though the user had not tried to update 'username' at all, since they pass in None. Right now, this just returns a 200 without updating anything or sending any message to the user. So the test has been written to ensure this behavior, but maybe it should be noted that @@ -467,7 +473,7 @@ def test_put_user_username_try_delete_username( "Authorization": "Bearer " + encoded_admin_jwt, "Content-Type": "application/json", }, - data=json.dumps({"name": None}), + data=json.dumps({"username": None}), ) assert r.status_code == 200 user = db_session.query(User).filter_by(username="test_a").one() @@ -685,6 +691,52 @@ def assert_google_proxy_group_data_deleted(db_session): assert len(gbag) == 1 +def test_soft_delete_user_username( + client, + encoded_admin_jwt, + db_session, + load_non_google_user_data, +): + """ + Test soft-delete user endpoint by checking that the result is an + deactivated user. + """ + username = "test_user_d" + user = db_session.query(User).filter_by(username=username).one() + assert user.username == username + assert user.active == True + # now soft-delete and assert "active" changed to False: + r = client.delete( + f"/admin/users/{username}/soft", + headers={"Authorization": "Bearer " + encoded_admin_jwt}, + ) + assert r.status_code == 200 + assert r.json["username"] == username + assert r.json["active"] == False + user = db_session.query(User).filter_by(username=username).one() + assert user.username == username + assert user.active == False + + +def test_soft_delete_user_user_not_found( + client, + encoded_admin_jwt, + db_session, +): + """ + Test soft-delete user endpoint returns error when user is not found. + """ + username = "non_existing_user" + user = db_session.query(User).filter_by(username=username).first() + assert user is None + # now call soft-delete and assert it fails: + r = client.delete( + f"/admin/users/{username}/soft", + headers={"Authorization": "Bearer " + encoded_admin_jwt}, + ) + assert r.status_code == 404 + + def test_delete_user_username( app, client,