zhoujinsong commented on PR #4118: URL: https://github.com/apache/amoro/pull/4118#issuecomment-4108799576
Thanks for the PR! A few thoughts: **1. Consider using jCasbin for the permission model** There is a prior PR [#3544](https://github.com/apache/amoro/pull/3544) that implemented readonly user support using jCasbin. The jCasbin approach has some advantages worth considering: - Permission rules are externalized from code — adding new roles or changing resource granularity only requires policy changes, not code changes - A unified enforcer intercept is more reliable than manually maintaining per-endpoint checks in `DashboardServer`, which is easy to miss - It opens the door for future fine-grained resource permissions (e.g., per-catalog access control) without a major refactor The complexity of integrating jCasbin is not high (PR #3544 shows +254/-37), and it would make the permission layer more extensible and maintainable. **2. Role fallback defaults to `ADMIN` in login (security concern)** ```typescript // amoro-web/src/views/login/index.vue role: result.role || 'ADMIN', ``` If `result.role` is absent (e.g., older backend or unexpected response), the client silently gets admin privileges. Suggest defaulting to `'READ_ONLY'` to follow the principle of least privilege. **3. Backend coverage of write endpoints** The frontend hides buttons based on `canWrite()`, but this is UI-only protection. Please confirm that **all** write endpoints in the backend also enforce 403 for `READ_ONLY` users — not just the ones currently guarded in `DashboardServer`. **4. `normalizeUsername()` scope** The LDAP provider strips email domain suffixes (e.g., `[email protected]` → `xuba`). Please confirm this normalization only applies in the LDAP authentication path and does not affect `DefaultPasswdAuthenticationProvider` users. **5. `bind-password` encryption note missing in docs** The `bind-password` field is added to `sensitive-keywords` (good), but the RBAC example in `ams-config.md` shows it in plaintext without any note about encryption. Suggest adding a brief note that users should configure `shade` encryption for this field in production. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
