WEB-812: Fix the WebApp to show/hide the Menus, Submenus and buttons as per the grants of each profile#3360
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Toolbar Permission Arrays src/app/core/shell/toolbar/toolbar.component.html |
Replaced individual string permission tokens with multi-permission arrays (e.g., READ_INSTITUTION → ['READ_CLIENT', 'READ_GROUP', 'READ_CENTER']) across toolbar items and institution submenu for visibility gating. |
HasPermission Directive Multi-Permission Support src/app/directives/has-permission/has-permission.directive.ts |
Extended directive to accept `string |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~22 minutes
Possibly related PRs
- WEB-575 Fix the web-app to show the menus and buttons create import delete edit as per the grants of each profile #3102: Modifies the same permission-visibility code paths (toolbar template and has-permission directive) with RBAC gating and early-return behavior changes.
- WEB-106 Do not offer functionality to a user that is not within her/his rights #2900: Updates toolbar permission checks using mifosxHasPermission guards that this PR extends to support array-aware permissions.
Suggested reviewers
- IOhacker
- alberto-art3ch
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately describes the main change: fixing menu/submenu/button visibility based on user role permissions, which is the core objective of the PR. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
env.sample (1)
52-52: Consider keeping RBAC disabled by default in sample configuration.Changing
MIFOS_PRODUCTION_MODE_ENABLE_RBACdefault fromfalsetotruein the sample file changes the expected behavior for new deployments. The comment on lines 50-51 indicatesfalseprovides "backward compatibility."New users copying this sample may unexpectedly have restricted menu visibility if their Fineract backend doesn't have granular permissions configured.
Suggested change
-MIFOS_PRODUCTION_MODE_ENABLE_RBAC=true +MIFOS_PRODUCTION_MODE_ENABLE_RBAC=false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@env.sample` at line 52, The sample env flips MIFOS_PRODUCTION_MODE_ENABLE_RBAC to true which can surprise new deployments; change the sample value back to false so RBAC remains disabled by default, keep or adjust the adjacent comment about "backward compatibility" to match, and ensure any README or docs still recommend enabling RBAC only after Fineract granular permissions are configured.src/app/directives/has-permission/has-permission.directive.ts (1)
97-110: Minor: PreferstartsWith()oversubstring()comparison.Using
startsWith()is more readable and idiomatic for prefix checking.Optional refactor
private checkSinglePermission(permission: string): boolean { permission = permission.trim(); if (permission !== '') { - if (permission.substring(0, 5) === 'READ_' && this.userPermissions.includes('ALL_FUNCTIONS_READ')) { + if (permission.startsWith('READ_') && this.userPermissions.includes('ALL_FUNCTIONS_READ')) { return true; } else if (this.userPermissions.includes(permission)) { return true; } else { return false; } } else { return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/directives/has-permission/has-permission.directive.ts` around lines 97 - 110, In checkSinglePermission replace the prefix check permission.substring(0, 5) === 'READ_' with permission.startsWith('READ_') to make intent clearer and more idiomatic; keep the rest of the logic intact so that if permission is non-empty and startsWith('READ_') and this.userPermissions includes 'ALL_FUNCTIONS_READ' it returns true, otherwise fall back to the existing this.userPermissions.includes(permission) check.src/app/core/shell/toolbar/toolbar.component.html (1)
57-63: Consider extracting repeated permission arrays to component properties.The Accounting permission array
['READ_JOURNALENTRY', 'READ_GLACCOUNT', 'READ_GLCLOSURE', 'READ_ACCOUNTINGRULE', 'READ_PROVISIONINGCRITERIA']is duplicated at lines 57-63 and 153-159. Similarly, the Admin permissions['READ_USER', 'READ_ROLE']appear at lines 85 and 170.Defining these as component constants would improve maintainability and ensure consistency.
Example in toolbar.component.ts
readonly accountingPermissions = [ 'READ_JOURNALENTRY', 'READ_GLACCOUNT', 'READ_GLCLOSURE', 'READ_ACCOUNTINGRULE', 'READ_PROVISIONINGCRITERIA' ]; readonly adminPermissions = ['READ_USER', 'READ_ROLE'];Then in template:
*mifosxHasPermission="accountingPermissions"Also applies to: 153-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/core/shell/toolbar/toolbar.component.html` around lines 57 - 63, The template repeats permission arrays; add readonly properties on the ToolbarComponent such as accountingPermissions and adminPermissions (or similarly named constants) containing the respective permission string arrays, then replace the inline arrays in toolbar.component.html (the *mifosxHasPermission bindings) with those property names (e.g., *mifosxHasPermission="accountingPermissions" and *mifosxHasPermission="adminPermissions") to centralize and reuse the permission lists across the template.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@proxy.conf.js`:
- Line 22: The proxy.conf.js change sets target:
'https://mifos-bank-2.mifos.community', which alters the default dev backend for
all developers; revert the default target to 'https://demo.mifos.community' and
make the target configurable via an environment variable (e.g.,
process.env.PROXY_TARGET) so developers can override it for testing specific
tenants; update the proxy configuration to use the env var fallback pattern
(keep the identifier 'target' and the string 'https://demo.mifos.community' as
the default) and add a short comment or README note explaining how to override
PROXY_TARGET for local testing.
In `@src/assets/env.js`:
- Line 103: The default hardcoded flag window['env']['productionModeEnableRBAC']
is set to true which forces RBAC on for all deployments; change the default to
false in the env.js fallback (modify the assignment of
window['env']['productionModeEnableRBAC']) so RBAC is opt-in, and ensure the
code path that loads environment overrides can toggle this value at deploy time
(keep the override mechanism intact so deployments can set
productionModeEnableRBAC=true when desired).
---
Nitpick comments:
In `@env.sample`:
- Line 52: The sample env flips MIFOS_PRODUCTION_MODE_ENABLE_RBAC to true which
can surprise new deployments; change the sample value back to false so RBAC
remains disabled by default, keep or adjust the adjacent comment about "backward
compatibility" to match, and ensure any README or docs still recommend enabling
RBAC only after Fineract granular permissions are configured.
In `@src/app/core/shell/toolbar/toolbar.component.html`:
- Around line 57-63: The template repeats permission arrays; add readonly
properties on the ToolbarComponent such as accountingPermissions and
adminPermissions (or similarly named constants) containing the respective
permission string arrays, then replace the inline arrays in
toolbar.component.html (the *mifosxHasPermission bindings) with those property
names (e.g., *mifosxHasPermission="accountingPermissions" and
*mifosxHasPermission="adminPermissions") to centralize and reuse the permission
lists across the template.
In `@src/app/directives/has-permission/has-permission.directive.ts`:
- Around line 97-110: In checkSinglePermission replace the prefix check
permission.substring(0, 5) === 'READ_' with permission.startsWith('READ_') to
make intent clearer and more idiomatic; keep the rest of the logic intact so
that if permission is non-empty and startsWith('READ_') and this.userPermissions
includes 'ALL_FUNCTIONS_READ' it returns true, otherwise fall back to the
existing this.userPermissions.includes(permission) check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d0346b42-2115-4437-be63-349be3a4b14e
📒 Files selected for processing (5)
env.sampleproxy.conf.jssrc/app/core/shell/toolbar/toolbar.component.htmlsrc/app/directives/has-permission/has-permission.directive.tssrc/assets/env.js
| { | ||
| context: ['/fineract-provider'], | ||
| target: 'https://demo.mifos.community', | ||
| target: 'https://mifos-bank-2.mifos.community', |
There was a problem hiding this comment.
Reconsider changing the default proxy target.
The proxy target is changed from demo.mifos.community to mifos-bank-2.mifos.community. This affects all developers using npm start for local development. Per coding guidelines, demo.mifos.community is the recommended default proxy target.
If this change is needed for testing specific tenant RBAC configurations, consider:
- Keep
demo.mifos.communityas the default inproxy.conf.js - Document how to override for specific testing needs
- Use environment variables to make the target configurable
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@proxy.conf.js` at line 22, The proxy.conf.js change sets target:
'https://mifos-bank-2.mifos.community', which alters the default dev backend for
all developers; revert the default target to 'https://demo.mifos.community' and
make the target configurable via an environment variable (e.g.,
process.env.PROXY_TARGET) so developers can override it for testing specific
tenants; update the proxy configuration to use the env var fallback pattern
(keep the identifier 'target' and the string 'https://demo.mifos.community' as
the default) and add a short comment or README note explaining how to override
PROXY_TARGET for local testing.
| // Enable Role-Based Access Control (RBAC) for menu/button permissions | ||
| // Set to true to enable RBAC, false (default) for backward compatibility | ||
| window['env']['productionModeEnableRBAC'] = false; | ||
| window['env']['productionModeEnableRBAC'] = true; |
There was a problem hiding this comment.
Hardcoding RBAC enabled in env.js may cause unexpected behavior.
Setting productionModeEnableRBAC to true in this file affects all environments that don't override it. This file is typically used as a fallback/default configuration.
For organizations without granular Fineract permissions configured, menus will be hidden unexpectedly. Consider keeping false as the safe default and letting deployments opt-in via environment variable overrides.
Suggested change
// Enable Role-Based Access Control (RBAC) for menu/button permissions
// Set to true to enable RBAC, false (default) for backward compatibility
- window['env']['productionModeEnableRBAC'] = true;
+ window['env']['productionModeEnableRBAC'] = false;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| window['env']['productionModeEnableRBAC'] = true; | |
| window['env']['productionModeEnableRBAC'] = false; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/assets/env.js` at line 103, The default hardcoded flag
window['env']['productionModeEnableRBAC'] is set to true which forces RBAC on
for all deployments; change the default to false in the env.js fallback (modify
the assignment of window['env']['productionModeEnableRBAC']) so RBAC is opt-in,
and ensure the code path that loads environment overrides can toggle this value
at deploy time (keep the override mechanism intact so deployments can set
productionModeEnableRBAC=true when desired).
IOhacker
left a comment
IOhacker
left a comment
There was a problem hiding this comment.
@ansh-varshney I am going to merge this, there is another section that should be fixed, which are the options in the hamburger menu
IOhacker
left a comment
IOhacker
left a comment
There was a problem hiding this comment.
I have noticed something.... the proxy.conf.js should not be changed
There was a problem hiding this comment.
Please exclude this file, do not commit it
Update *mifosxHasPermission directives on parent menus to accept arrays of granular permissions. Hide Accounting and Admin tabs from restricted tenant roles (Ejecutivo, Cajero, Tesorero, Supervisor, Oficial de Control) based on Fineract permission grants. Fixes: WEB-812
ansh-varshney
force-pushed
the
WEB-812-show/hide-menus-as-per-role
branch
from
517b844 to
ed83b0f
Compare
|
Hi, @IOhacker |
2 participants
JIRA TICKET: WEB-812
Description
Modified the mifosxHasPermission directive to accept a string array of permissions. Updated the toolbar.component.html menu items (Institution, Accounting, Reports, Admin) to use arrays of specific Fineract read permissions rather than generic module strings.
This ensures that restricted tenant roles like Ejecutivo, Cajero, and Tesorero do not see the Admin or Accounting tabs unless explicitly granted granular configuration or ledger permissions in the backend.
Screenshots, if any
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
If you have multiple commits please combine them into one commit by squashing them.
Read and understood the contribution guidelines at
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit