diff --git a/README.md b/README.md index 33d5562d..0fa41ce9 100644 --- a/README.md +++ b/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 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. diff --git a/api/models/core_models.py b/api/models/core_models.py index 602bc2e0..6dfaec3f 100644 --- a/api/models/core_models.py +++ b/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", diff --git a/migrations/env.py b/migrations/env.py index 0f2292e1..aacd10fe 100644 --- a/migrations/env.py +++ b/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, diff --git a/migrations/versions/0a113a8bc0df_create_new_indexes.py b/migrations/versions/0a113a8bc0df_create_new_indexes.py new file mode 100644 index 00000000..17b049ac --- /dev/null +++ b/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 ### diff --git a/tests/test_group_ownership_sync.py b/tests/test_group_ownership_sync.py index a4d03ac8..9ac7d14e 100644 --- a/tests/test_group_ownership_sync.py +++ b/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]: