From 4296285a6cb2a60514fc401ea84f8f7953a855db Mon Sep 17 00:00:00 2001 From: Peter C Date: Thu, 27 Jun 2024 16:43:25 -0400 Subject: [PATCH] Bump ruff and fix type equalvalence checking issues (#93) --- .github/workflows/lint.yml | 1 - api/models/access_request.py | 2 +- api/operations/constraints/check_for_reason.py | 4 ++-- api/operations/constraints/check_for_self_add.py | 4 ++-- api/operations/create_access_request.py | 4 ++-- api/operations/create_app.py | 6 +++--- api/operations/create_group.py | 4 ++-- api/operations/delete_group.py | 4 ++-- api/operations/modify_group_type.py | 13 ++++++------- api/operations/modify_group_users.py | 6 +++--- api/syncer.py | 8 ++++---- api/views/resources/group.py | 6 +++--- api/views/resources/webhook.py | 2 +- requirements.txt | 2 +- tox.ini | 2 +- 15 files changed, 33 insertions(+), 35 deletions(-) diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 03b17cc..a619a3b 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -23,7 +23,6 @@ jobs: python-version: ["3.12"] steps: - uses: actions/checkout@v4 - - uses: chartboost/ruff-action@v1 - name: Set up Python ${{ matrix.python-version }} uses: actions/setup-python@v5 with: diff --git a/api/models/access_request.py b/api/models/access_request.py index 24696ac..94b04ea 100644 --- a/api/models/access_request.py +++ b/api/models/access_request.py @@ -15,7 +15,7 @@ def get_all_possible_request_approvers(access_request: AccessRequest) -> Set[Okt app_managers = [] - if type(access_request.requested_group) == AppGroup: + if type(access_request.requested_group) is AppGroup: app_managers = get_app_managers(access_request.requested_group.app_id) return set(group_owners + access_app_owners + app_managers) diff --git a/api/operations/constraints/check_for_reason.py b/api/operations/constraints/check_for_reason.py index 1a0647b..fab0ead 100644 --- a/api/operations/constraints/check_for_reason.py +++ b/api/operations/constraints/check_for_reason.py @@ -64,7 +64,7 @@ def execute_for_group(self) -> Tuple[bool, str]: # If the group is a role group check to see if a reason is required for adding members or owners # to the associated groups - if type(self.group) == RoleGroup and self.group.is_managed: + if type(self.group) is RoleGroup and self.group.is_managed: member_groups = [rm.active_group for rm in self.group.active_role_associated_group_member_mappings] for member_group in member_groups: require_member_reason = coalesce_constraints( @@ -92,7 +92,7 @@ def execute_for_group(self) -> Tuple[bool, str]: return True, "" def execute_for_role(self) -> Tuple[bool, str]: - if type(self.group) != RoleGroup: + if type(self.group) is not RoleGroup: return True, "" if self.invalid_reason(self.reason): diff --git a/api/operations/constraints/check_for_self_add.py b/api/operations/constraints/check_for_self_add.py index b9a05fb..4c112fd 100644 --- a/api/operations/constraints/check_for_self_add.py +++ b/api/operations/constraints/check_for_self_add.py @@ -79,7 +79,7 @@ def execute_for_group(self) -> Tuple[bool, str]: # If the group is a role group check to see if a reason is required for adding members or owners # to the associated groups - if type(self.group) == RoleGroup and self.group.is_managed: + if type(self.group) is RoleGroup and self.group.is_managed: member_groups = [rm.active_group for rm in self.group.active_role_associated_group_member_mappings] for member_group in member_groups: disallow_self_add_membership = coalesce_constraints( @@ -112,7 +112,7 @@ def execute_for_role(self) -> Tuple[bool, str]: if self.current_user is None or AuthorizationHelpers.is_access_admin(self.current_user.id): return True, "" - if type(self.group) != RoleGroup: + if type(self.group) is not RoleGroup: return True, "" # Check to see if the current user is a member of the role, diff --git a/api/operations/create_access_request.py b/api/operations/create_access_request.py index 28f48ac..5ea6dae 100644 --- a/api/operations/create_access_request.py +++ b/api/operations/create_access_request.py @@ -82,9 +82,9 @@ def execute(self) -> Optional[AccessRequest]: # If there are no approvers, try to get the app managers # or if the only approver is the requester, try to get the app managers if ( - (len(approvers) == 0 and type(self.requested_group) == AppGroup) + (len(approvers) == 0 and type(self.requested_group) is AppGroup) or (len(approvers) == 1 and approvers[0].id == self.requester.id) - and type(self.requested_group) == AppGroup + and type(self.requested_group) is AppGroup ): approvers = get_app_managers(self.requested_group.app_id) diff --git a/api/operations/create_app.py b/api/operations/create_app.py index a2ef03c..54ffd52 100644 --- a/api/operations/create_app.py +++ b/api/operations/create_app.py @@ -129,7 +129,7 @@ def execute(self) -> App: owner_app_group = CreateGroup(group=owner_app_group, current_user_id=self.current_user_id).execute() else: group_id = existing_owner_group.id - if type(existing_owner_group) != AppGroup: + if type(existing_owner_group) is not AppGroup: ModifyGroupType( group=existing_owner_group, group_changes=AppGroup(app_id=app_id, is_owner=True), @@ -168,7 +168,7 @@ def execute(self) -> App: ) existing_app_group_ids_to_update = [] for existing_app_group in other_existing_app_groups: - if type(existing_app_group) == AppGroup: + if type(existing_app_group) is AppGroup: existing_app_group.app_id = app_id existing_app_group.is_owner = False else: @@ -198,7 +198,7 @@ def execute(self) -> App: CreateGroup(group=app_group, current_user_id=self.current_user_id).execute() else: group_id = existing_group.id - if type(existing_owner_group) != AppGroup: + if type(existing_owner_group) is not AppGroup: ModifyGroupType( group=existing_owner_group, group_changes=AppGroup(app_id=app_id, is_owner=False), diff --git a/api/operations/create_group.py b/api/operations/create_group.py index 25379d5..c1af9fc 100644 --- a/api/operations/create_group.py +++ b/api/operations/create_group.py @@ -45,7 +45,7 @@ def execute(self, *, _group: Optional[T] = None) -> T: # Make sure the app exists if we're creating an app group if ( - type(self.group) == AppGroup + type(self.group) is AppGroup and App.query.filter(App.id == self.group.app_id).filter(App.deleted_at.is_(None)).first() is None ): raise ValueError("App for AppGroup does not exist") @@ -58,7 +58,7 @@ def execute(self, *, _group: Optional[T] = None) -> T: db.session.commit() # If this is an app group, add any app tags - if type(self.group) == AppGroup: + if type(self.group) is AppGroup: app_tag_maps = ( AppTagMap.query.filter(AppTagMap.app_id == self.group.app_id) .filter( diff --git a/api/operations/delete_group.py b/api/operations/delete_group.py index 7bed8bf..c20d8d0 100644 --- a/api/operations/delete_group.py +++ b/api/operations/delete_group.py @@ -48,7 +48,7 @@ async def _execute(self) -> None: okta_tasks = [] # Prevent deletion of the Access owner group - if type(self.group) == AppGroup and self.group.is_owner: + if type(self.group) is AppGroup and self.group.is_owner: app = App.query.filter(App.id == self.group.app_id).filter(App.deleted_at.is_(None)).first() if app is not None and app.name == App.ACCESS_APP_RESERVED_NAME: raise ValueError("Access application owner group cannot be deleted") @@ -110,7 +110,7 @@ async def _execute(self) -> None: {RoleGroupMap.ended_at: db.func.now()}, synchronize_session="fetch" ) - if type(self.group) == RoleGroup: + if type(self.group) is RoleGroup: # End all group memberships via the role grant OktaUserGroupMember.query.filter( db.or_( diff --git a/api/operations/modify_group_type.py b/api/operations/modify_group_type.py index 5e5e505..c119942 100644 --- a/api/operations/modify_group_type.py +++ b/api/operations/modify_group_type.py @@ -40,13 +40,12 @@ def __init__(self, *, group: OktaGroup | str, group_changes: OktaGroup, current_ def execute(self) -> OktaGroup: # Update group type if it's being modified - # Ignore Ruff because we don't want to include subclasses with an isinstance comparison - if type(self.group) != type(self.group_changes): # noqa: E721 + if type(self.group) is not type(self.group_changes): group_id = self.group.id old_group_type = self.group.type # Clean-up the old child table row - if type(self.group) == RoleGroup: + if type(self.group) is RoleGroup: # End all group attachments to this role and all group memberships via the role grant active_role_associated_groups = RoleGroupMap.query.filter( db.or_( @@ -63,7 +62,7 @@ def execute(self) -> OktaGroup: db.session.commit() db.session.execute(delete(RoleGroup.__table__).where(RoleGroup.__table__.c.id == group_id)) - elif type(self.group) == AppGroup: + elif type(self.group) is AppGroup: # Bail if this is the owner group for the app # which cannot have its type changed if self.group.is_owner: @@ -96,7 +95,7 @@ def execute(self) -> OktaGroup: self.group = OktaGroup.query.filter(OktaGroup.deleted_at.is_(None)).filter(OktaGroup.id == group_id).first() # Create new child table row - if type(self.group_changes) == RoleGroup: + if type(self.group_changes) is RoleGroup: # Convert any group memberships and ownerships via a role to direct group memberships and ownerships active_group_users_from_role = ( OktaUserGroupMember.query.filter( @@ -153,7 +152,7 @@ def execute(self) -> OktaGroup: ).execute() db.session.execute(insert(RoleGroup.__table__).values(id=group_id)) - elif type(self.group_changes) == AppGroup: + elif type(self.group_changes) is AppGroup: db.session.execute( insert(AppGroup.__table__).values( id=group_id, @@ -170,7 +169,7 @@ def execute(self) -> OktaGroup: db.session.expunge_all() # Add all app tags to this new app group, after we've updated the group type - if type(self.group_changes) == AppGroup: + if type(self.group_changes) is AppGroup: app_tag_maps = ( AppTagMap.query.options(joinedload(AppTagMap.active_tag)) .filter( diff --git a/api/operations/modify_group_users.py b/api/operations/modify_group_users.py index a37415d..04dbd41 100644 --- a/api/operations/modify_group_users.py +++ b/api/operations/modify_group_users.py @@ -206,7 +206,7 @@ async def _execute(self) -> OktaGroup: ) # For role groups, members to be removed should also be removed from all role associated groups - if type(self.group) == RoleGroup: + if type(self.group) is RoleGroup: role_associated_groups_mappings = ( RoleGroupMap.query.filter( db.or_( @@ -274,7 +274,7 @@ async def _execute(self) -> OktaGroup: async_tasks.append(asyncio.create_task(okta.async_remove_owner_from_group(self.group.id, owner_id))) # For role groups, members to be removed should also be removed from all Access-managed role associated groups - if type(self.group) == RoleGroup and self.sync_to_okta: + if type(self.group) is RoleGroup and self.sync_to_okta: role_associated_groups_mappings = ( RoleGroupMap.query.options(joinedload(RoleGroupMap.active_group)) .join(RoleGroupMap.active_group) @@ -388,7 +388,7 @@ async def _execute(self) -> OktaGroup: db.session.add(ownership_to_add) # For role groups, new members should also be added to all Access-managed role associated groups - if type(self.group) == RoleGroup: + if type(self.group) is RoleGroup: role_associated_groups_mappings = ( RoleGroupMap.query.options(joinedload(RoleGroupMap.active_group)) .join(RoleGroupMap.active_group) diff --git a/api/syncer.py b/api/syncer.py index 011a9ae..4d33138 100644 --- a/api/syncer.py +++ b/api/syncer.py @@ -526,7 +526,7 @@ def expiring_access_notifications_owner() -> None: owners = get_group_managers(group.id) if len(owners) == 0: - owners += get_app_managers(group.app_id) if type(group) == AppGroup else [] + owners += get_app_managers(group.app_id) if type(group) is AppGroup else [] if len(owners) == 0: owners = access_owners @@ -563,7 +563,7 @@ def expiring_access_notifications_owner() -> None: owners = get_group_managers(group.id) if len(owners) == 0: - owners += get_app_managers(group.app_id) if type(group) == AppGroup else [] + owners += get_app_managers(group.app_id) if type(group) is AppGroup else [] if len(owners) == 0: owners = access_owners @@ -632,7 +632,7 @@ def expiring_access_notifications_owner() -> None: owners = get_group_managers(group.id) if len(owners) == 0: - owners += get_app_managers(group.app_id) if type(group) == AppGroup else [] + owners += get_app_managers(group.app_id) if type(group) is AppGroup else [] if len(owners) == 0: owners = access_owners @@ -666,7 +666,7 @@ def expiring_access_notifications_owner() -> None: owners = get_group_managers(group.id) if len(owners) == 0: - owners += get_app_managers(group.app_id) if type(group) == AppGroup else [] + owners += get_app_managers(group.app_id) if type(group) is AppGroup else [] if len(owners) == 0: owners = access_owners diff --git a/api/views/resources/group.py b/api/views/resources/group.py index 29f947b..6d32c06 100644 --- a/api/views/resources/group.py +++ b/api/views/resources/group.py @@ -126,7 +126,7 @@ def put(self, group: OktaGroup) -> ResponseReturnValue: ) # Do not allow non-tag modifications of app owner groups (including Access app owner group) - if type(group) == AppGroup and group.is_owner: + if type(group) is AppGroup and group.is_owner: if len(json_data.get("tags_to_add", [])) > 0 or len(json_data.get("tags_to_remove", [])) > 0: ModifyGroupTags( group=group, @@ -160,7 +160,7 @@ def put(self, group: OktaGroup) -> ResponseReturnValue: abort(400, "Group already exists with the same name") # Update group type if it's being modified - if type(group) != type(group_changes): + if type(group) is not type(group_changes): # Only access admins should be able to change group types if not AuthorizationHelpers.is_access_admin(): abort( @@ -221,7 +221,7 @@ def delete(self, group: OktaGroup) -> ResponseReturnValue: "Groups not managed by Access cannot be modified", ) # Do not allow deletion of app owner groups (including the Access app owner group) - if type(group) == AppGroup and group.is_owner: + if type(group) is AppGroup and group.is_owner: abort( 400, "Application owner groups cannot be deleted without first deleting the application", diff --git a/api/views/resources/webhook.py b/api/views/resources/webhook.py index 5798984..cc985ec 100644 --- a/api/views/resources/webhook.py +++ b/api/views/resources/webhook.py @@ -100,7 +100,7 @@ def post(self) -> ResponseReturnValue: ).execute() # If the user has access to this group via a role, remove them from the role - if type(group) != RoleGroup and event.get("eventType") == "group.user_membership.remove": + if type(group) is not RoleGroup and event.get("eventType") == "group.user_membership.remove": active_role_user_group_memberships = ( OktaUserGroupMember.query.options( joinedload(OktaUserGroupMember.active_role_group_mapping).joinedload( diff --git a/requirements.txt b/requirements.txt index af38f5a..5d43cc5 100644 --- a/requirements.txt +++ b/requirements.txt @@ -43,7 +43,7 @@ pluggy==1.5.0 # Test - TODO: Move test packages to separate file tox==4.15.1 # Lint -ruff==0.4.8 +ruff==0.5.0 # Typing mypy==1.10.0 types-google-cloud-ndb==2.3.0.20240311 diff --git a/tox.ini b/tox.ini index 1e940f0..92e3deb 100644 --- a/tox.ini +++ b/tox.ini @@ -40,7 +40,7 @@ setenv = [testenv:ruff] commands = - ruff check --diff . + ruff check . ruff format --check --diff . [testenv:mypy]