fix: private knowledgebase access check in Go DAO

[jonathanchang31]

Summary

This PR fixes KnowledgebaseDAO.Accessible() so private knowledge bases are not exposed to tenant members unless the knowledge base permission is explicitly set to team.

Before this change, any user joined to the same tenant could access a knowledge base because the access query only checked user_tenant membership and ignored the knowledgebase.permission field.

After this change:

• Owners can still access their own knowledge bases.
• Tenant members can access only team-shared knowledge bases.
• Tenant members cannot access private knowledge bases.
• Unrelated users cannot access team knowledge bases.

Related Issue

Closes: #15647

Change Type

• Bug fix
• Security fix
• Test
• Feature
• Refactor
• Documentation
• Breaking change

Real Behavior Proof

Issue validated in current code:

• KnowledgebaseDAO.Accessible() joined user_tenant.
• It checked tenant membership.
• It did not check knowledgebase.permission.
• This allowed tenant members to access private knowledge bases.
  Fixed behavior is covered by new DAO tests:

PATH=/tmp/go/bin:$PATH go test ./internal/dao -run 'TestKnowledgebaseDAO' -count=1

Result:

ok  	ragflow/internal/dao	0.323s

Full DAO package validation:

PATH=/tmp/go/bin:$PATH go test ./internal/dao -count=1

Result:

ok  	ragflow/internal/dao	0.295s

Whitespace validation:

git diff --check

Result:

Passed

Checklist

• Verified the issue exists in the current project.
• Compared behavior with the Python reference implementation.
• Applied a minimal DAO-level fix.
• Added regression tests.
• Ran focused DAO tests successfully.
• Ran full DAO package tests successfully.
• Ran git diff --check.
• Avoided unrelated API or service changes.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f9042c40-6966-485e-bcd1-483739c534a7

📥 Commits

Reviewing files that changed from the base of the PR and between 4b79ce9 and 69b0bab.

📒 Files selected for processing (1)

• internal/dao/kb.go

🚧 Files skipped from review as they are similar to previous changes (1)

• internal/dao/kb.go

---

📝 Walkthrough

Walkthrough

Accessible() now allows access when the requester is the KB creator or when the KB permission is team and the requester has an active tenant membership; status = valid remains enforced. Tests and DB migration were added to cover private vs. team access scenarios; test helpers now populate tenant and membership rows.

Changes

Knowledge Base Access Control Authorization

Layer / File(s)	Summary
Authorization logic with permission checks internal/dao/kb.go	Accessible replaces a two-step check with a single query that LEFT JOINs user_tenant (pre-filtered by user and active status) and authorizes if the requester is knowledgebase.created_by or if knowledgebase.permission = 'team' and a matching user_tenant row exists; knowledgebase.status = valid remains enforced.
Test migration, helpers, and access-control tests internal/dao/kb_test.go	Test DB auto-migrates UserTenant; testKnowledgebase now sets tenant/creator/permission/status fields; testUserTenant helper added; six new tests cover owner access for private KBs, tenant-member rejection for private KBs, non-creator rejection when tenant IDs match, tenant-member allowance for team KBs, revoked-member rejection for team KBs, and unrelated-user rejection for team KBs.

Sequence Diagram

sequenceDiagram
  participant Caller
  participant KnowledgebaseDAO
  participant DB
  participant UserTenant
  Caller->>KnowledgebaseDAO: Accessible(kbID, userID)
  KnowledgebaseDAO->>DB: Query knowledgebase LEFT JOIN user_tenant (filter user_id, active)
  DB->>UserTenant: check matching user_tenant row
  DB-->>KnowledgebaseDAO: result (creator match OR permission='team' + user_tenant present) with status=valid
  KnowledgebaseDAO-->>Caller: allow/deny

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• infiniflow/ragflow#15270: Adds an UpdateMetadataSetting authorization guard that calls kbDAO.Accessible, directly using the access-control mechanism fixed in this PR.
• infiniflow/ragflow#15028: Enforces/depends on Knowledgebase*.accessible(...) access checks in related endpoints.

Suggested labels

lgtm, size:S

Suggested reviewers

• wangq8
• Haruko386

Poem

🐰 I hopped through joins and rows tonight,
checked creator first and team flags right,
tests planted tenants, helpers in tow,
private stays closed, team members may go,
a rabbit hums, "access guards all set to go."

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly reflects the core fix: addressing the private knowledgebase access check vulnerability in the Go DAO layer.
Description check	✅ Passed	The description comprehensively covers the problem, solution, related issue, change type, validation steps, and a detailed checklist; it aligns with the template structure.
Linked Issues check	✅ Passed	The PR meets the core objectives from #15647: rewritten Accessible() now enforces permission semantics by checking both owner status and team permission, and new tests verify the behavior.
Out of Scope Changes check	✅ Passed	All changes are narrowly scoped to fixing KnowledgebaseDAO.Accessible() and adding corresponding DAO-layer tests; no unrelated API or service changes detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/dao/kb_test.go`:
- Around line 141-197: Add a regression test for the branch in
KnowledgebaseDAO.Accessible where tenant_id == requester but the requester is
not the creator and permission == "me": create a knowledgebase via
testKnowledgebase with ID like "kb-me", set kb.TenantID to "tenant-1" (or the
same value you will use for the requester), set kb.CreatedBy to a different user
(e.g. "other-1"), set kb.Permission = string(entity.TenantPermissionMe), save it
(db.Save), ensure a user/tenant mapping exists for the requester via
testUserTenant (e.g. user "tenant-1" mapped to tenant "tenant-1"), then assert
dao.Accessible("kb-me", "tenant-1") returns false. Reference functions/types:
Accessible, testKnowledgebase, testUserTenant, entity.TenantPermissionMe.

In `@internal/dao/kb.go`:
- Around line 249-253: The access WHERE clause in internal/dao/kb.go currently
includes an authorization bypass `knowledgebase.tenant_id = ?` inside the
Where(...) predicate; remove that `OR knowledgebase.tenant_id = ?` condition so
access is only granted by `knowledgebase.created_by = ?` or
`(knowledgebase.permission = ? AND user_tenant.user_id IS NOT NULL)`. After
removing the condition, update the parameter list passed to Where(...) to drop
the extra userID argument so the placeholders match (keep kbID,
string(entity.StatusValid), userID, string(entity.TenantPermissionTeam)). Ensure
the modified predicate and args are used wherever this query fragment is built.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 196c1000-42e4-4ae5-a20a-49378d620abc

📥 Commits

Reviewing files that changed from the base of the PR and between 7789862 and 21d437b.

📒 Files selected for processing (2)

• internal/dao/kb.go
• internal/dao/kb_test.go

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch PR diff
gh pr diff 15680 --repo infiniflow/ragflow

echo "=== kb.go full content ==="
cat internal/dao/kb.go

echo "=== kb_test.go full content ==="
cat internal/dao/kb_test.go

Length of output: 30624

---

---

@jonathanchang31 Happy to review! The core fix is logically sound and the test suite covers the key scenarios well. I found one security gap and a few minor issues.

---

Review of internal/dao/kb.go — Accessible()

✅ What's correct

The LEFT JOIN + userID-filtered ON clause is the right technique, and the WHERE predicate correctly implements the two-path access model:

knowledgebase.created_by = ?          -- owner always allowed
OR
(knowledgebase.permission = ? AND user_tenant.user_id IS NOT NULL)  -- team member allowed

⚠️ Critical: missing user_tenant.status filter

The LEFT JOIN does not filter revoked/invalid tenant memberships:

// Current — no status guard
Joins("LEFT JOIN user_tenant ON user_tenant.tenant_id = knowledgebase.tenant_id AND user_tenant.user_id = ?", userID)

A user whose tenant membership has been soft-deleted (e.g., status = 'invalid') still has a user_tenant row, so user_tenant.user_id IS NOT NULL evaluates to true and they can still access team KBs. This is a privilege-escalation path.

Suggested fix:

Joins("LEFT JOIN user_tenant ON user_tenant.tenant_id = knowledgebase.tenant_id AND user_tenant.user_id = ? AND user_tenant.status = ?",
    userID, string(entity.StatusValid)).

⚠️ Missing test: revoked tenant member should be rejected

All five new tests create user_tenant with status = 'valid'. You should add a test that creates a user_tenant with status = 'invalid' (or no status) for a team KB and asserts Accessible() returns false:

func TestKnowledgebaseDAO_AccessibleRejectsRevokedTenantMemberForTeamKnowledgebase(t *testing.T) {
    db := setupKBTestDB(t)
    pushDB(t, db)
    dao := NewKnowledgebaseDAO()

    kb := testKnowledgebase(t, db, "kb-team", 0, 0, 0)
    kb.Permission = string(entity.TenantPermissionTeam)
    if err := db.Save(kb).Error; err != nil {
        t.Fatalf("failed to update test kb permission: %v", err)
    }

    // Create a revoked membership
    invalidStatus := string(entity.StatusInvalid)
    db.Create(&entity.UserTenant{
        ID: "ut-revoked", UserID: "revoked-member", TenantID: "tenant-1",
        Role: "normal", InvitedBy: "tenant-1", Status: &invalidStatus,
    })

    if dao.Accessible("kb-team", "revoked-member") {
        t.Fatal("revoked tenant member should not access a team knowledgebase")
    }
}

ℹ️ Minor: GetKBByIDAndUserID has the same original bug pattern

// internal/dao/kb.go — GetKBByIDAndUserID
Joins("JOIN user_tenant ON user_tenant.tenant_id = knowledgebase.tenant_id").
Where("knowledgebase.id = ? AND user_tenant.user_id = ?", kbID, userID).

This is an INNER JOIN with no permission check — the same pattern the PR fixes in Accessible(). If this method is used anywhere for access control, it would expose private KBs. It's out of scope for this PR, but worth tracking as a follow-up.

ℹ️ Cosmetic: whitespace-only changes in AtomicIncreaseDocNumByID / DecreaseDocumentNum

These changes are pure alignment noise and would be cleaner to omit or leave in a separate commit.

---

Summary

	Severity
Missing user_tenant.status = 'valid' in LEFT JOIN	🔴 High — revoked members bypass the guard
Missing test for revoked tenant member	🟡 Medium
GetKBByIDAndUserID same-pattern bug	🔵 Follow-up
Whitespace noise	⚪ Cosmetic

The fix is in the right direction — just needs the status guard added to the join and one more test case to close the gap.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[jonathanchang31]

@coderabbitai Could you plz review my PR quickly?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@jonathanchang31 I already completed a full review just above this comment! Here's a quick recap of what needs attention before this is merge-ready:

#	Finding	Severity
1	user_tenant.status not filtered in the LEFT JOIN — revoked/invalid members can still access team KBs	🔴 High
2	Missing test for revoked membership — add a test asserting Accessible() returns false for an invalid-status user_tenant row	🟡 Medium
3	GetKBByIDAndUserID has the same no-permission-check pattern — worth a follow-up issue	🔵 Low
4	Whitespace-only changes in AtomicIncreaseDocNumByID / DecreaseDocumentNum	⚪ Cosmetic

The one-liner fix for item 1 is:

Joins("LEFT JOIN user_tenant ON user_tenant.tenant_id = knowledgebase.tenant_id AND user_tenant.user_id = ? AND user_tenant.status = ?",
    userID, string(entity.StatusValid)).

And the suggested test for item 2 is in my previous comment. Address those two and the PR should be good to go! 🐇

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@jonathanchang31 I've already done a full review (see my comments above in this thread) — no new commits have been pushed since, so the same findings still stand. Here's the TL;DR:

#	Finding	Severity
1	user_tenant.status not filtered in the LEFT JOIN — revoked members can still access team KBs	🔴 High
2	Missing test for revoked/invalid membership	🟡 Medium
3	GetKBByIDAndUserID — same missing-permission-check pattern	🔵 Follow-up
4	Whitespace-only changes in AtomicIncreaseDocNumByID / DecreaseDocumentNum	⚪ Cosmetic

The one-liner fix for item 1:

Joins("LEFT JOIN user_tenant ON user_tenant.tenant_id = knowledgebase.tenant_id AND user_tenant.user_id = ? AND user_tenant.status = ?",
    userID, string(entity.StatusValid)).

Once you apply that fix and add the revoked-membership test case (see the suggested test in my first review comment), the PR should be in good shape. 🐇

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
gh pr diff 15680 --repo infiniflow/ragflow

echo "=== current kb.go ==="
gh api repos/infiniflow/ragflow/contents/internal/dao/kb.go --jq '.content' | base64 -d 2>/dev/null || true

echo "=== current kb_test.go ==="
gh api repos/infiniflow/ragflow/contents/internal/dao/kb_test.go --jq '.content' | base64 -d 2>/dev/null || true

Length of output: 27862

---

---

@jonathanchang31 Great work — both issues are now properly fixed! Here's the verification:

✅ All previously flagged items resolved

#	Finding	Status
1	user_tenant.status = ? guard in LEFT JOIN	✅ Fixed
2	Test for revoked tenant membership	✅ Added
3	GetKBByIDAndUserID missing-permission-check pattern	🔵 Follow-up (out of scope)
4	Whitespace noise in AtomicIncreaseDocNumByID / DecreaseDocumentNum	✅ Cleaned up

Item 1 — the LEFT JOIN now correctly reads:

Joins("LEFT JOIN user_tenant ON user_tenant.tenant_id = knowledgebase.tenant_id AND user_tenant.user_id = ? AND user_tenant.status = ?",
    userID, string(entity.StatusValid))

This closes the privilege-escalation path for soft-deleted memberships.

Item 2 — TestKnowledgebaseDAO_AccessibleRejectsRevokedTenantMemberForTeamKnowledgebase creates a user_tenant row with status = StatusInvalid and asserts Accessible() returns false. Exactly what was needed.

The only remaining item is GetKBByIDAndUserID (plain INNER JOIN, no permission/status check) — that was always flagged as a follow-up. Would you like me to open a GitHub issue to track it so it doesn't get lost?

🐇 ✨

---

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

[jonathanchang31]

@yingfeng Could you review my PR? I am going to contribute, I am looking forward to your support. Thanks!