Skip to content

Remove unused resource parameter from getPermissionsFor - #1870

Open
NaderMohamed325 wants to merge 2 commits into
nodeSolidServer:mainfrom
NaderMohamed325:fix/remove-unused-resource-param
Open

Remove unused resource parameter from getPermissionsFor#1870
NaderMohamed325 wants to merge 2 commits into
nodeSolidServer:mainfrom
NaderMohamed325:fix/remove-unused-resource-param

Conversation

@NaderMohamed325

Copy link
Copy Markdown

What

Removes the unused resource parameter from getPermissionsFor in lib/header.js.

Why

resource is passed into getPermissionsFor but never used inside the function body —
acl.can(user, mode) doesn't take a resource argument. As confirmed in #973, the
resource is already implicit in the acl object itself (scoped via
ACLChecker.createFromLDPAndRequest, see lib/acl-checker.js), so passing resource
again here is dead code.

Closes #973

Testing

Ran the full test suite locally on Windows. 4 pre-existing failures unrelated to this
change (CRLF/LF line-ending mismatches in OIDC integration tests, and a signup page
timeout) — confirmed these fail identically with this change stashed out.

Copilot AI review requested due to automatic review settings July 28, 2026 12:19

Copilot AI 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.

Pull request overview

Removes an unused parameter from the internal getPermissionsFor helper used to populate the WAC-Allow response header, relying on the acl instance to already be scoped to the requested resource.

Changes:

  • Removed the unused req/resource-related parameter from getPermissionsFor.
  • Updated addPermissions to call getPermissionsFor with the simplified signature.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/header.mjs Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@NaderMohamed325

Copy link
Copy Markdown
Author

Hi @bourgeoa / @kjetilk / @RubenVerborgh — this addresses #973 that one of you
already confirmed. Happy to make any changes if needed, just flagging in case
it fell off the radar.

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.

WAC-Allow doesn't seem to depend on resource

2 participants