Update README for performing DB migrations, add new indexes, update migration params

README.md

@@ -12,7 +12,7 @@ The access service exists to help answer the following questions for each person
 
 - All Users
   - What do I have access to?
-  - What does a teammate have access to that I don’t?
+  - What does a teammate have access to that I don't?
   - What groups and roles are available?
   - Can I get access?
 - Team Leads
@@ -341,6 +341,47 @@ flask init <YOUR_OKTA_USER_EMAIL>
 
 Visit [http://localhost:3000/](http://localhost:3000/) to view your running version of Access!
 
+### Database Migrations
+
+Access uses [Alembic](https://alembic.sqlalchemy.org/) for database schema migrations. When you make changes to the database models, you'll need to create and apply a migration.
+
+#### Creating a New Migration
+
+To create a new migration after making changes to your models:
+
+1. Make your changes to the SQLAlchemy models in the `api/models` directory
+2. Generate a new migration using:
+   ```
+   flask db migrate -m "Description of your changes"
+   ```
+   This will create a new migration file in the `migrations/versions` directory with a name like `xxxxxxxxxxxx_description_of_your_changes.py`
+
+   For changes that don't involve schema modifications (like adding indexes), you may need to manually create the migration:
+   ```
+   flask db revision -m "Description of your changes"
+   ```
+   Then edit the generated migration file to include your changes, for example:
+   ```python
+   def upgrade():
+       op.create_index('ix_table_column', 'table', ['column'])
+
+   def downgrade():
+       op.drop_index('ix_table_column', 'table')
+   ```
+
+3. Review the generated migration file in `migrations/versions` to ensure it correctly captures your changes
+4. Apply the migration using:
+   ```
+   flask db upgrade
+   ```
+
+If you need to rollback a migration, you can use:
+```
+flask db downgrade
+```
+
+For more information about Alembic migrations, see the [Alembic documentation](https://alembic.sqlalchemy.org/).
+
 ### Kubernetes Deployment and CronJobs
 
 As Access is a web application packaged with Docker, it can easily be deployed to a Kubernetes cluster. We've included example Kubernetes yaml objects you can use to deploy Access in the [examples/kubernetes](https://github.com/discord/access/tree/main/examples/kubernetes) directory.

api/models/core_models.py

@@ -40,6 +40,16 @@ class OktaUserGroupMember(db.Model):
 
     created_reason: Mapped[str] = mapped_column(db.Unicode(1024), nullable=False, default="", server_default="")
 
+    __table_args__ = (
+        db.Index("idx_okta_user_group_member_user_id", "user_id"),
+        db.Index("idx_okta_user_group_member_group_id", "group_id"),
+        db.Index("idx_okta_user_group_member_role_group_map_id", "role_group_map_id"),
+        db.Index("idx_okta_user_group_member_created_actor_id", "created_actor_id"),
+        db.Index("idx_okta_user_group_member_ended_actor_id", "ended_actor_id"),
+        db.Index("idx_okta_user_group_member_user_group", "user_id", "group_id"),
+        db.Index("idx_okta_user_group_member_group_ended", "group_id", "ended_at"),
+    )
+
     # See more details on specifying alternative join conditions for relationships at
     # https://docs.sqlalchemy.org/en/14/orm/join_conditions.html#specifying-alternate-join-conditions
     group: Mapped["OktaGroup"] = db.relationship(
@@ -136,6 +146,7 @@ class OktaUser(db.Model):
             postgresql_where=db.text("deleted_at IS NULL"),
             sqlite_where=db.text("deleted_at IS NULL"),
         ),
+        db.Index("idx_okta_user_manager_id", "manager_id"),
     )
 
     # A JSON field for storing the user profile, including extra user attribute data from Okta
@@ -455,6 +466,13 @@ class RoleGroupMap(db.Model):
 
     created_reason: Mapped[str] = mapped_column(db.Unicode(1024), nullable=False, default="", server_default="")
 
+    __table_args__ = (
+        db.Index("idx_role_group_map_role_id", "role_id"),
+        db.Index("idx_role_group_map_group_id", "group_id"),
+        db.Index("idx_role_group_map_created_actor_id", "created_actor_id"),
+        db.Index("idx_role_group_map_ended_actor_id", "ended_actor_id"),
+    )
+
     # See more details on specifying alternative join conditions for relationships at
     # https://docs.sqlalchemy.org/en/14/orm/join_conditions.html#specifying-alternate-join-conditions
     role_group: Mapped["RoleGroup"] = db.relationship(
@@ -589,6 +607,8 @@ class AppGroup(OktaGroup):
     # group to administer and manage membership of the app owner group
     is_owner: Mapped[bool] = mapped_column(db.Boolean, nullable=False, server_default=expression.false(), default=False)
 
+    __table_args__ = (db.Index("idx_app_group_app_id", "app_id"),)
+
     # SQLAlchemy doesn't seem to support loading
     # group.active_role_associated_group_[member|owner]_mappings.active_group when a group_id or user_id is specified
     # in GET /api/audit/users so we have to enable "select" lazy loading.
@@ -714,6 +734,14 @@ class AccessRequest(db.Model):
         db.ForeignKey("okta_user_group_member.id"),
     )
 
+    __table_args__ = (
+        db.Index("idx_access_request_requester_user_id", "requester_user_id"),
+        db.Index("idx_access_request_requested_group_id", "requested_group_id"),
+        db.Index("idx_access_request_resolver_user_id", "resolver_user_id"),
+        db.Index("idx_access_request_approved_membership_id", "approved_membership_id"),
+        db.Index("idx_access_request_status_resolved", "status", "resolved_at"),
+    )
+
     requester: Mapped[OktaUser] = db.relationship(
         "OktaUser",
         back_populates="all_access_requests",
@@ -801,6 +829,14 @@ class RoleRequest(db.Model):
         db.ForeignKey("role_group_map.id"),
     )
 
+    __table_args__ = (
+        db.Index("idx_role_request_requester_user_id", "requester_user_id"),
+        db.Index("idx_role_request_requester_role_id", "requester_role_id"),
+        db.Index("idx_role_request_requested_group_id", "requested_group_id"),
+        db.Index("idx_role_request_resolver_user_id", "resolver_user_id"),
+        db.Index("idx_role_request_approved_membership_id", "approved_membership_id"),
+    )
+
     requester: Mapped[OktaUser] = db.relationship(
         "OktaUser",
         primaryjoin="OktaUser.id == RoleRequest.requester_user_id",

migrations/env.py

@@ -3,9 +3,8 @@
 import logging
 from logging.config import fileConfig
 
-from flask import current_app
-
 from alembic import context
+from flask import current_app
 
 # this is the Alembic Config object, which provides
 # access to the values within the .ini file in use.
@@ -30,6 +29,27 @@
 # ... etc.
 
 
+def include_object(object, name, type_, reflected, compare_to):
+    """
+    Control which database objects are included in auto-generated migrations.
+    """
+    # Always include indexes to ensure they're properly tracked
+    if type_ == "index":
+        return True
+
+    # Exclude Flask-Migrate's own version table
+    if type_ == "table" and name == "alembic_version":
+        return False
+
+    # Exclude any PostgreSQL system tables/schemas
+    if hasattr(object, "schema"):
+        if object.schema in ["information_schema", "pg_catalog", "pg_toast"]:
+            return False
+
+    # Include everything else (tables, columns, etc.)
+    return True
+
+
 def run_migrations_offline():
     """Run migrations in 'offline' mode.
 
@@ -69,6 +89,18 @@ def process_revision_directives(context, revision, directives):
 
     connectable = current_app.extensions["migrate"].db.get_engine()
 
+    configure_args = current_app.extensions["migrate"].configure_args.copy()
+
+    configure_args.update(
+        {
+            "compare_type": True,  # Detect column type changes
+            "compare_server_default": True,  # Detect default value changes
+            "include_schemas": True,  # Include schema information
+            "include_object": include_object,  # Use our custom filter
+            "render_as_batch": True,  # For SQLite compatibility
+        }
+    )
+
     with connectable.connect() as connection:
         context.configure(
             connection=connection,

migrations/versions/0a113a8bc0df_create_new_indexes.py

@@ -0,0 +1,95 @@
+"""create new indexes
+
+Revision ID: 0a113a8bc0df
+Revises: 6d2a03b326f9
+Create Date: 2025-06-13 10:28:06.674777
+
+"""
+
+from alembic import op
+
+# revision identifiers, used by Alembic.
+revision = "0a113a8bc0df"
+down_revision = "6d2a03b326f9"
+branch_labels = None
+depends_on = None
+
+
+def upgrade():
+    # ### commands auto generated by Alembic - please adjust! ###
+    with op.batch_alter_table("access_request", schema=None) as batch_op:
+        batch_op.create_index("idx_access_request_approved_membership_id", ["approved_membership_id"], unique=False)
+        batch_op.create_index("idx_access_request_requested_group_id", ["requested_group_id"], unique=False)
+        batch_op.create_index("idx_access_request_requester_user_id", ["requester_user_id"], unique=False)
+        batch_op.create_index("idx_access_request_resolver_user_id", ["resolver_user_id"], unique=False)
+        batch_op.create_index("idx_access_request_status_resolved", ["status", "resolved_at"], unique=False)
+
+    with op.batch_alter_table("app_group", schema=None) as batch_op:
+        batch_op.create_index("idx_app_group_app_id", ["app_id"], unique=False)
+
+    with op.batch_alter_table("okta_user", schema=None) as batch_op:
+        batch_op.create_index("idx_okta_user_manager_id", ["manager_id"], unique=False)
+
+    with op.batch_alter_table("okta_user_group_member", schema=None) as batch_op:
+        batch_op.create_index("idx_okta_user_group_member_created_actor_id", ["created_actor_id"], unique=False)
+        batch_op.create_index("idx_okta_user_group_member_ended_actor_id", ["ended_actor_id"], unique=False)
+        batch_op.create_index("idx_okta_user_group_member_group_ended", ["group_id", "ended_at"], unique=False)
+        batch_op.create_index("idx_okta_user_group_member_group_id", ["group_id"], unique=False)
+        batch_op.create_index("idx_okta_user_group_member_role_group_map_id", ["role_group_map_id"], unique=False)
+        batch_op.create_index("idx_okta_user_group_member_user_group", ["user_id", "group_id"], unique=False)
+        batch_op.create_index("idx_okta_user_group_member_user_id", ["user_id"], unique=False)
+
+    with op.batch_alter_table("role_group_map", schema=None) as batch_op:
+        batch_op.create_index("idx_role_group_map_created_actor_id", ["created_actor_id"], unique=False)
+        batch_op.create_index("idx_role_group_map_ended_actor_id", ["ended_actor_id"], unique=False)
+        batch_op.create_index("idx_role_group_map_group_id", ["group_id"], unique=False)
+        batch_op.create_index("idx_role_group_map_role_id", ["role_id"], unique=False)
+
+    with op.batch_alter_table("role_request", schema=None) as batch_op:
+        batch_op.create_index("idx_role_request_approved_membership_id", ["approved_membership_id"], unique=False)
+        batch_op.create_index("idx_role_request_requested_group_id", ["requested_group_id"], unique=False)
+        batch_op.create_index("idx_role_request_requester_role_id", ["requester_role_id"], unique=False)
+        batch_op.create_index("idx_role_request_requester_user_id", ["requester_user_id"], unique=False)
+        batch_op.create_index("idx_role_request_resolver_user_id", ["resolver_user_id"], unique=False)
+
+    # ### end Alembic commands ###
+
+
+def downgrade():
+    # ### commands auto generated by Alembic - please adjust! ###
+    with op.batch_alter_table("role_request", schema=None) as batch_op:
+        batch_op.drop_index("idx_role_request_resolver_user_id")
+        batch_op.drop_index("idx_role_request_requester_user_id")
+        batch_op.drop_index("idx_role_request_requester_role_id")
+        batch_op.drop_index("idx_role_request_requested_group_id")
+        batch_op.drop_index("idx_role_request_approved_membership_id")
+
+    with op.batch_alter_table("role_group_map", schema=None) as batch_op:
+        batch_op.drop_index("idx_role_group_map_role_id")
+        batch_op.drop_index("idx_role_group_map_group_id")
+        batch_op.drop_index("idx_role_group_map_ended_actor_id")
+        batch_op.drop_index("idx_role_group_map_created_actor_id")
+
+    with op.batch_alter_table("okta_user_group_member", schema=None) as batch_op:
+        batch_op.drop_index("idx_okta_user_group_member_user_id")
+        batch_op.drop_index("idx_okta_user_group_member_user_group")
+        batch_op.drop_index("idx_okta_user_group_member_role_group_map_id")
+        batch_op.drop_index("idx_okta_user_group_member_group_id")
+        batch_op.drop_index("idx_okta_user_group_member_group_ended")
+        batch_op.drop_index("idx_okta_user_group_member_ended_actor_id")
+        batch_op.drop_index("idx_okta_user_group_member_created_actor_id")
+
+    with op.batch_alter_table("okta_user", schema=None) as batch_op:
+        batch_op.drop_index("idx_okta_user_manager_id")
+
+    with op.batch_alter_table("app_group", schema=None) as batch_op:
+        batch_op.drop_index("idx_app_group_app_id")
+
+    with op.batch_alter_table("access_request", schema=None) as batch_op:
+        batch_op.drop_index("idx_access_request_status_resolved")
+        batch_op.drop_index("idx_access_request_resolver_user_id")
+        batch_op.drop_index("idx_access_request_requester_user_id")
+        batch_op.drop_index("idx_access_request_requested_group_id")
+        batch_op.drop_index("idx_access_request_approved_membership_id")
+
+    # ### end Alembic commands ###

tests/test_group_ownership_sync.py

@@ -396,16 +396,32 @@ def run_sync(
     act_as_authority: bool,
     groups_with_rules: set[str] = set(),
 ) -> list[OktaGroup]:
-    with Session(db.engine) as session:
-        mocker.patch.object(okta, "list_groups", return_value=okta_groups)
-
-        mocker.patch.object(okta, "list_owners_for_group", side_effect=user_ownership_func)
-
-        mocker.patch.object(okta, "list_groups_with_active_rules", return_value=groups_with_rules)
+    # Initialize Okta service first
+    okta.initialize("test.okta.com", "test-token")
+
+    # Mock the okta module functions
+    mocker.patch.object(okta, "list_groups", return_value=okta_groups)
+    mocker.patch.object(okta, "list_owners_for_group", side_effect=user_ownership_func)
+    mocker.patch.object(okta, "list_groups_with_active_rules", return_value=groups_with_rules)
+
+    # Check if these are already mocked by the test
+    if not hasattr(okta.add_owner_to_group, "_mock_name"):
+        mocker.patch.object(okta, "add_owner_to_group", return_value=None)
+    if not hasattr(okta.remove_owner_from_group, "_mock_name"):
+        mocker.patch.object(okta, "remove_owner_from_group", return_value=None)
+
+    # Mock OktaService for ModifyGroupUsers
+    mock_service = mocker.MagicMock()
+    mock_service.okta_client = mocker.MagicMock()
+    mock_service.use_group_owners_api = False
+    mocker.patch("api.services.okta_service.OktaService", return_value=mock_service)
 
+    with Session(db.engine) as session:
         sync_group_ownerships(act_as_authority)
+        session.commit()
 
-        return session.query(OktaUserGroupMember).all()
+    db.session.expire_all()
+    return db.session.query(OktaUserGroupMember).all()
 
 
 def _get_group_owners(db: SQLAlchemy, group_id: str) -> dict[str, OwnershipDetails]: