Skip to content

[codex] Add app-based permission management - #378

Merged
ravilodhi merged 10 commits into
mainfrom
codex/permission-by-app-pr
Jun 12, 2026
Merged

[codex] Add app-based permission management#378
ravilodhi merged 10 commits into
mainfrom
codex/permission-by-app-pr

Conversation

@dt2patel

Copy link
Copy Markdown
Contributor

Business summary

This PR gives Users admins an app-first way to understand and manage permissions. Instead of asking users to interpret abstract permission categories, the experience shows each app, the actions available in that app, and which security groups have those permissions.

Closes #377

What changed

  • Added app permission catalog files for Users, Inventory Count, and the other app permission sets discovered during the audit.
  • Added a script and generated audit output for one-time local permission scanning while keeping deployed permission data stable.
  • Added an app permissions page with By app and By group segments.
  • Moved the legacy security-group permission workflow into the new page and redirected the old /tabs/permissions route to the group segment.
  • Updated security group create/add-permissions navigation to return to the new group segment.
  • Grouped security-group permission assignment cards by app, with uncataloged permissions under Other permissions.
  • Removed Product Updates and Order Manager Solr Query as standalone app catalog entries.

Validation

  • npm run build

Notes

The temporary local Moqui login commit was intentionally left out of this clean PR branch.

@dt2patel
dt2patel marked this pull request as ready for review June 10, 2026 17:32

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a comprehensive "Permissions by App" feature, allowing administrators to manage permissions grouped by application. It includes a local audit script to scan for permission usages, new reusable UI components (cards and modals), a dedicated AppPermissions.vue view, and a new AppPermissionService for managing assignments. The feedback identifies several critical issues and improvements: a bug where .length is checked on an object dictionary (allPermissions), potential TypeError risks due to missing optional chaining or guard checks on currentGroup, a potential race condition in Vuex state updates, and a Windows compatibility issue in the audit script's path resolution.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +253 to +255
if(this.securityGroupUsers[this.currentGroup.groupId]) {
return;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Add a guard check to ensure this.currentGroup?.groupId is defined before accessing it. If currentGroup is null or undefined initially, accessing groupId directly will throw a TypeError.

      if(!this.currentGroup?.groupId || this.securityGroupUsers[this.currentGroup.groupId]) {
        return;
      }

async loadGroupPermissions() {
await this.store.dispatch('util/getSecurityGroups');
await this.store.dispatch('util/getClassificationSecurityGroups');
if(!this.allPermissions.length) await this.store.dispatch('permission/getAllPermissions');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

Since allPermissions is a dictionary/object (as evidenced by this.allPermissions?.[permissionId] and Object.values(this.allPermissions) in PermissionItems.vue), checking !this.allPermissions.length will always evaluate to true because objects do not have a length property. This results in redundant API calls to fetch all permissions every time loadGroupPermissions is executed. Use Object.keys(this.allPermissions || {}).length instead.

      if(!Object.keys(this.allPermissions || {}).length) await this.store.dispatch('permission/getAllPermissions');

Comment on lines +1 to +6
import { promises as fs } from "node:fs";
import path from "node:path";
import process from "node:process";

// Local one-time audit tool only. Runtime code must use committed catalog modules, not repo scans.
const repoRoot = path.resolve(new URL("..", import.meta.url).pathname);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Using new URL(..., import.meta.url).pathname to resolve file paths can fail on Windows platforms because it retains URL-encoded characters and can prepend an extra leading slash (e.g., /C:/...). Use fileURLToPath from the node:url module to safely convert file URLs to native system paths.

Suggested change
import { promises as fs } from "node:fs";
import path from "node:path";
import process from "node:process";
// Local one-time audit tool only. Runtime code must use committed catalog modules, not repo scans.
const repoRoot = path.resolve(new URL("..", import.meta.url).pathname);
import { promises as fs } from "node:fs";
import path from "node:path";
import process from "node:process";
import { fileURLToPath } from "node:url";
// Local one-time audit tool only. Runtime code must use committed catalog modules, not repo scans.
const repoRoot = path.resolve(fileURLToPath(new URL("..", import.meta.url)));

},
methods: {
createGroup() {
this.$router.push({ path: `/create-security-group/` });

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

For consistency with other routing calls in this component (which use this.router returned from setup()), use this.router.push instead of this.$router.push.

      this.router.push({ path: `/create-security-group/` });

await this.store.dispatch('util/getClassificationSecurityGroups');
if(!this.allPermissions.length) await this.store.dispatch('permission/getAllPermissions');
if(!Object.keys(this.permissionsByClassificationGroups).length) await this.store.dispatch('permission/getPermissionsByClassificationGroups');
if(this.currentGroup.groupId) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Use optional chaining this.currentGroup?.groupId to prevent a potential TypeError if currentGroup is null or undefined.

      if(this.currentGroup?.groupId) {

Comment on lines +403 to +404
emitter.emit('presentLoader');
await this.store.dispatch('permission/updateCurrentGroup', group);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Instead of relying on the Vuex store to synchronously update this.currentGroup before calling getPermissionsByGroup, use group.groupId directly from the function parameter. This avoids potential race conditions or stale state issues.

      await this.store.dispatch('permission/updateCurrentGroup', group);
      await this.store.dispatch('permission/getPermissionsByGroup', group.groupId);

@ravilodhi
ravilodhi merged commit 266b78f into main Jun 12, 2026
2 checks passed
@dt2patel
dt2patel deleted the codex/permission-by-app-pr branch July 21, 2026 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add app-based permission management in Users

2 participants