Skip to content

fix(auth): don't fail ACL preflight when self-read fails - #284

Open
Hami0095 wants to merge 1 commit into
algolia:mainfrom
Hami0095:fix/acl-preflight-non-fatal
Open

Hami0095 wants to merge 1 commit into
algolia:mainfrom
Hami0095:fix/acl-preflight-non-fatal

Conversation

@Hami0095

Copy link
Copy Markdown

Summary

Fixes #183.

CheckACLs runs as a preflight before every command. For a non-admin key it reads the key's own ACLs via GET /1/keys/{key} to compare against what the command declares it needs (pkg/auth/auth_check.go:122-129).

That self-read requires the search ACL — unrelated to whatever ACL the command actually needs. If the key lacks search (or otherwise can't self-read), the preflight returns a bare 403 and the command never runs, even when the key holds exactly the ACLs it needs. The friendly errMissingACLs remediation message is never reached, since it depends on the self-read having succeeded first.

Fix

Treat a failing self-read as non-fatal — the same way the admin-key branch just above it already treats a failing ListApiKeys as "not admin" rather than an error. The real API call the command makes remains the authority on whether the key is actually sufficient.

Test plan

  • Added two cases to Test_CheckACLs in pkg/auth/auth_check_test.go: self-read failure + non-admin ACL need (should succeed), self-read failure + admin ACL need (should still return errAdminAPIKeyRequired)
  • Confirmed the non-admin case fails against unpatched main and passes with the fix
  • go build ./..., go vet ./... clean
  • go test ./pkg/auth/... passes, no regressions elsewhere

🤖 Generated with Claude Code

CheckACLs reads the API key's own ACLs via GET /1/keys/{key} to
compare against what the command declares it needs. That endpoint
requires the `search` ACL, which is unrelated to most commands, so a
key that's missing it (or otherwise can't self-read) gets a bare
error from the preflight and the command never runs, even when the
key holds every ACL it actually needs.

Make the self-read failure non-fatal, the same way the admin-key
check above it already treats a failing ListApiKeys as "not admin"
rather than an error. The real API call remains the authority on
whether the key is sufficient.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@codacy-production

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity · 0 duplication

Metric Results
Complexity 0
Duplication 0

View in Codacy

TIP This summary will be updated as you push new changes.

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.

API key not working if key is set as local

1 participant