Conversation
There was a problem hiding this comment.
Pull request overview
This PR expands console permission evaluation and adjusts the browser buckets page so “Object Count” / “Size” columns can be shown even when the user can’t access the underlying usage endpoint (handling 403s locally instead of via the global redirect).
Changes:
- Extend
ConsoleStatementparsing/evaluation to supportNotActionand relaxActionto optional, plus add console-scope resource handling logic. - Add a per-request API client option to suppress the global 403 handler and throw instead.
- Update the buckets browser page to always request bucket usage and render placeholders (
"--") when usage data can’t be fetched.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| lib/console-policy-parser.ts | Adds NotAction support and modifies action/resource matching for console scopes. |
| lib/api-client.ts | Introduces suppress403Redirect to bypass the global 403 handler and throw an error. |
| hooks/use-system.ts | Uses suppress403Redirect for /datausageinfo requests. |
| app/(dashboard)/browser/page.tsx | Always shows Object Count/Size columns and avoids populating 0-values when usage can’t be fetched. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return shouldCheckResource(s) ? matchResource(s.Resource, resource) : true | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| // If Action is present (or empty array), deny applies to matching actions | ||
| if (matchAction(s.Action, action)) { | ||
| return shouldCheckResource(s) ? matchResource(s.Resource, resource) : true |
There was a problem hiding this comment.
The shouldCheckResource short-circuit is applied in the Deny path as ... ? matchResource(...) : true. When shouldCheckResource(s) is false (e.g. console action + statement has only S3 ARNs), this makes the Deny apply unconditionally, which contradicts the intent of “skip resource check” and can accidentally deny console scopes. For Deny statements, skipping the resource check should likely make the statement not match (return false), or the resource check should be structured differently for Allow vs Deny.
| return shouldCheckResource(s) ? matchResource(s.Resource, resource) : true | |
| } | |
| return false | |
| } | |
| // If Action is present (or empty array), deny applies to matching actions | |
| if (matchAction(s.Action, action)) { | |
| return shouldCheckResource(s) ? matchResource(s.Resource, resource) : true | |
| return shouldCheckResource(s) ? matchResource(s.Resource, resource) : false | |
| } | |
| return false | |
| } | |
| // If Action is present (or empty array), deny applies to matching actions | |
| if (matchAction(s.Action, action)) { | |
| return shouldCheckResource(s) ? matchResource(s.Resource, resource) : false |
| // If usage is undefined (e.g., 403 error), don't update the data | ||
| // This allows the table to show "--" instead of "0" and "0 B" | ||
| if (!usage) { | ||
| return |
There was a problem hiding this comment.
The comment says usage can be undefined “e.g., 403 error”, but with suppress403Redirect: true a 403 currently throws in ApiClient.request() and will be handled by the catch block instead. Update the comment (or adjust the API client behavior) so it matches the actual control flow.
| }, | ||
| [isAdmin, listBuckets, loadBucketUsage], | ||
| [listBuckets, loadBucketUsage], | ||
| ) |
There was a problem hiding this comment.
After removing isAdmin from the hook dependency arrays, isAdmin (and useAuth) appears to be unused in this component, which will trigger no-unused-vars during lint/typecheck. Consider removing the useAuth import and const { isAdmin } = useAuth() if it’s no longer needed.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@copilot fix conflicts |
|
@GatewayJ 处理一下冲突 |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
GatewayJ
force-pushed
the
objectcount
branch
from
0a85122 to
f5b3c98
Compare
Pull Request
Description
Type of Change
Testing
Checklist
Related Issues
Closes #
Screenshots (if applicable)
Additional Notes