Skip to content

[AIDAPP-1668]: Improve access management by consolidating Change Requests permissions - #1701

Open
rebecca-canyon wants to merge 5 commits into
mainfrom
AIDAPP-1668-Improve-access-management-by-consolidating-Change-Requests-permissions
Open

[AIDAPP-1668]: Improve access management by consolidating Change Requests permissions#1701
rebecca-canyon wants to merge 5 commits into
mainfrom
AIDAPP-1668-Improve-access-management-by-consolidating-Change-Requests-permissions

Conversation

@rebecca-canyon

@rebecca-canyon rebecca-canyon commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

Ticket(s) or GitHub Issue

Technical Description

Flatten various Change Request related permissions to only use Change Request permissions; reassigns existing Change Request related permissions for Users and Roles to use Change Request equivalents

Any deployment steps required?

No

Cleanup Tasks

.cleanup-tasks/2026_11_06_change_request_permission_compression.md

Before contributing and submitting this PR, make sure you have Read, agree, and are compliant with the contributing guidelines.

@rebecca-canyon rebecca-canyon added Change Type | Chore Cleanup and refactors that improve performance or the codebase that do not largely affect logic. Cleanup Task labels Nov 7, 2026
@rebecca-canyon
rebecca-canyon force-pushed the AIDAPP-1668-Improve-access-management-by-consolidating-Change-Requests-permissions branch from 8274be1 to b3498b9 Compare November 7, 2026 00:55
@canyon-gbs-automation
canyon-gbs-automation Bot requested a balanced review from Copilot November 7, 2026 01:04
Copilot AI reviewed Nov 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The migration bodies are reversed, and the permission backfill omits API accounts and soft-deleted users, leaving obsolete permissions assignable and former grants without equivalents under the activated policies.

2 open issues
What changed in this PR

Consolidates Change Request status and type permissions under Change Request permissions, simplifying access management while migrating existing user and role grants.

Changes:

  • Adds a permission backfill and feature-flagged policy transition.
  • Introduces a migration intended to remove obsolete permissions and updates permission descriptions.
  • Updates access tests and records post-deployment cleanup work.
File Description
tests/​TenantMigrations/​TenantMigrationsTest.php Tests permission mapping for users and roles.
app/​Features/​ChangeRequestPermissionCompressionFeature.php Gates the transition to consolidated permissions.
app-modules/​service-management/​tests/​Tenant/​Filament/​Resources/​ChangeRequestTypes/​Pages/​ListChangeRequestTypesTest.php Updates type access tests to consolidated permissions.
app-modules/​service-management/​tests/​Tenant/​Filament/​Resources/​ChangeRequestStatuses/​Pages/​ListChangeRequestStatusesTest.php Updates status access tests to consolidated permissions.
app-modules/​service-management/​src/​Policies/​ChangeRequestTypePolicy.php Switches type authorization based on the feature flag.
app-modules/​service-management/​src/​Policies/​ChangeRequestStatusPolicy.php Switches status authorization based on the feature flag.
app-modules/​service-management/​database/​migrations/​2026_11_07_000759_purge_unneeded_change_request_permissions.php Adds the intended legacy-permission purge.
app-modules/​service-management/​database/​migrations/​2026_11_06_233425_tmp_grant_change_request_permissions_to_users_and_roles_with_related_permissions.php Backfills equivalent permissions and activates the flag.
app-modules/​authorization/​src/​Enums/​PermissionDescription.php Consolidates descriptions for Change Request permissions.
.cleanup-tasks/​2026_11_06_change_request_permission_compression.md Tracks temporary migration, flag, and test cleanup.

Review effort: Balanced

Comment thread ..._233425_tmp_grant_change_request_permissions_to_users_and_roles_with_related_permissions.php Resolved
Comment thread ...nagement/database/migrations/2026_11_07_000759_purge_unneeded_change_request_permissions.php Resolved
@rebecca-canyon
rebecca-canyon requested a balanced review from Copilot November 7, 2026 01:44
Copilot AI reviewed Nov 7, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The migrations leave permission caches stale, and tests lack coverage for action-level access restrictions and SystemUser/API-guard recipients.

2 open issues
1 previously missed issue

In code that hasn't changed since last review

Medium severity Test exact action-level permissions to prevent access escalation

tests/​TenantMigrations/​TenantMigrationsTest.php:1971

Every affected fixture receives all seven legacy actions, so this test would also pass if the migration granted every Change Request action to someone with only a legacy view permission. Add single-permission dataset cases for users and roles under each source prefix, and assert the exact newly granted permissions, including the absence of unrelated actions. This verifies that consolidation preserves action-level restrictions rather than escalating access.

2 resolved since last review

Review effort: Balanced

Comment thread ..._233425_tmp_grant_change_request_permissions_to_users_and_roles_with_related_permissions.php
Comment on lines +78 to +81
SystemUser::query()
->withTrashed()
->whereHas('permissions', fn (Builder $query) => $query->where('name', $relatedPermission))
->each(fn (SystemUser $systemUser) => $systemUser->givePermissionTo($permission));

@rebecca-canyon rebecca-canyon Nov 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added System User to the migration test, which I believe to be sufficient coverage for this test

Comment thread ...nagement/database/migrations/2026_11_07_000759_purge_unneeded_change_request_permissions.php
Comment on lines +75 to +93
DB::transaction(function (): void {
collect($this->guards)
->each(fn (string $guard) => $this->deletePermissions(array_keys($this->permissions), $guard));
});
}

public function down(): void
{
DB::transaction(function (): void {
collect($this->guards)
->each(function (string $guard): void {
$permissions = Arr::except($this->permissions, keys: DB::table('permissions')
->where('guard_name', $guard)
->pluck('name')
->all());

$this->createPermissions($permissions, $guard);
});
});
@rebecca-canyon
rebecca-canyon marked this pull request as ready for review November 7, 2026 02:07
@rebecca-canyon
rebecca-canyon requested a review from a team as a code owner November 7, 2026 02:07

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@Orrison Orrison Awaiting requested review from Orrison Orrison is a code owner automatically assigned from canyongbs/canyon-gbs-lead-developers

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

Change Type | Chore Cleanup and refactors that improve performance or the codebase that do not largely affect logic. Cleanup Task

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rebecca-canyon