feat: improve ownership of new objects like tags & correspondents#967
Open
jchnkl wants to merge 10 commits into
Open
feat: improve ownership of new objects like tags & correspondents#967jchnkl wants to merge 10 commits into
jchnkl wants to merge 10 commits into
Conversation
Paperless-gpt may access paperless-ngx through an arbitrary account, e.g. admin, regular, or a dedicated account set up specifically for paperless-gpt. As of now new objects (e.g. tags, correspondents) are created with no owner or permission handling at all resulting in objects which belong to the user account which is configured to provide access for paperless-gpt. If the account is, e.g. a dedicated account, most other accounts won't be able to see the objects of a document, which will be displayed as "Private" by paperless-ngx. To solve this problem, this patch introduces three different options for setting ownership of new objects: * `client`: this is the default behaviour and will create an object owned by the user (and its permissions) which is configured to provide access for paperless-gpt. Default permissions are configurable through the web ui of paperless-ngx (Administration/Settings/Permissions) * `none`: No ownership or permissions are set on objects, making them globally read- and write(!)able for every user * `document`: The owner and the permissions of the current document will be used for new objects.
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds object-level permission types, an ChangesObject-Level Permission Handling
Sequence Diagram(s)sequenceDiagram
participant App
participant PaperlessClient
participant PaperlessAPI
App->>PaperlessClient: GetPermissions(originalDoc)
alt mode = "client"
PaperlessClient->>PaperlessAPI: GET api/ui_settings/
PaperlessAPI-->>PaperlessClient: UiSettings
PaperlessClient->>PaperlessClient: mapPermissions()
else mode = "document"
PaperlessClient->>PaperlessClient: use doc.Owner/doc.Permissions
else mode = "none"
PaperlessClient-->>App: nil
end
PaperlessClient-->>App: ObjPermissions
App->>PaperlessClient: CreateTag(name, objPerms)
PaperlessClient->>PaperlessAPI: POST api/tags/ with owner & set_permissions
PaperlessAPI-->>PaperlessClient: created id
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
types.go (2)
63-64: 💤 Low value
gofmtrealignment needed for addedOwner/Permissionsfields.The newly added
Owner intandPermissions DocumentPermissionsfields inGetDocumentApiResponseResult,GetDocumentApiResponse, andDocumentbreak the existing column alignment that the surrounding fields use.gofmtwill re-align them once run.As per coding guidelines: "Ensure all Go source files are formatted with gofmt before committing".
Also applies to: 100-101, 114-115
🤖 Prompt for 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. In `@types.go` around lines 63 - 64, The new struct fields Owner and Permissions in GetDocumentApiResponseResult, GetDocumentApiResponse, and Document have misaligned columns; run gofmt (or gofmt -w types.go) to reformat the file so field names, types, and tags are properly aligned (ensure the added fields Owner int and Permissions DocumentPermissions are aligned consistently with surrounding fields and that JSON tags remain intact).
146-161: 💤 Low value
gofmtformatting for new UI settings types.
UiSettingsUser,UiSettingsSettings, andUiSettingsfield declarations have inconsistent tab/space alignment. Runninggofmt/goimportswill normalize them.As per coding guidelines: "Ensure all Go source files are formatted with gofmt before committing".
🤖 Prompt for 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. In `@types.go` around lines 146 - 161, The new struct declarations UiSettingsUser, UiSettingsSettings, and UiSettings have inconsistent spacing/alignment in their field tags; run gofmt (or goimports) to normalize indentation and import formatting and ensure each struct field (Id, Username, IsStaff, IsSuperuser, Groups, Permissions, User, Settings) follows standard gofmt layout, then re-save/commit the file so types.go is properly formatted.paperless.go (3)
56-60: 💤 Low value
gofmtformatting forTagRequest.Field alignment is off;
gofmtwill reflow.-type TagRequest struct { - Name string `json:"name"` - Owner *int `json:"owner"` - SetPermissions *SetPermissions `json:"set_permissions,omitempty"` -} +type TagRequest struct { + Name string `json:"name"` + Owner *int `json:"owner"` + SetPermissions *SetPermissions `json:"set_permissions,omitempty"` +}As per coding guidelines: "Ensure all Go source files are formatted with gofmt before committing".
🤖 Prompt for 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. In `@paperless.go` around lines 56 - 60, The TagRequest struct fields (Name, Owner, SetPermissions) are misaligned and need gofmt formatting; run gofmt (e.g., gofmt -w paperless.go) or your editor's Go formatter to reflow and align the struct field declarations and tags so the file is properly formatted.
226-240: 💤 Low valueStale
clientpermissions when paperless-ngx UI defaults change.In
clientmode, onceObjPermissionsis populated it is never refreshed for the lifetime of the process. If an operator changes the default ownership/permissions in paperless-ngx (Administration → Settings → Permissions), paperless-gpt will keep using the old values until restarted. Consider re-fetching periodically (e.g., piggy-back on the existing hourlyrefreshCustomFieldsCachegoroutine) or unconditionally on each call if the request volume is low.🤖 Prompt for 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. In `@paperless.go` around lines 226 - 240, The code caches client.ObjPermissions (Owner and SetPermissions) in the branch handling objPermissions == "client" and never refreshes it, causing stale permissions when paperless-ngx UI defaults change; update the logic so ObjPermissions is refreshed periodically or on demand: either call client.GetUiSettings() to repopulate client.ObjPermissions inside the existing refreshCustomFieldsCache goroutine (piggy-back the update there) or, if traffic is low, re-fetch UI settings at the start of the code path that checks objPermissions == "client" (replace the current one-time population logic that sets client.ObjPermissions.Owner and client.ObjPermissions.SetPermissions). Ensure you use the existing client.GetUiSettings(ctx), update client.ObjPermissions.Owner and client.ObjPermissions.SetPermissions via mapPermissions(&uiSettings.Settings.Permissions), and avoid leaking errors (return or log errors as appropriate).
243-245: ⚡ Quick win
GetPermissionsalways returns nil error and exposes internal state.Two concerns worth tightening:
- The function signature returns
errorbut the implementation cannot fail — callers (CreateTag,CreateOrGetCorrespondent) defensively wrap withfmt.Errorf(... %w), which is now misleading. Either drop theerrorreturn, or have it actively populateObjPermissionson first use (callingUpdatePermissions(ctx, nil)forclient/nonemodes) so the call is meaningful.- Returning a pointer directly into
client.ObjPermissionslets callers mutate the cached state. Returning a value (or copy) is safer and pairs with the concurrency fix flagged above.🤖 Prompt for 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. In `@paperless.go` around lines 243 - 245, GetPermissions currently returns a pointer into client.ObjPermissions and a nil error which is misleading and unsafe; change it to actively ensure permissions are initialized and return a copy and a real error: have GetPermissions(ctx) call UpdatePermissions(ctx, nil) when ObjPermissions is uninitialized (covering client/none modes) so any initialization error is returned, and return a value copy (not &client.ObjPermissions) to avoid external mutation; update callers (CreateTag, CreateOrGetCorrespondent) to accept the value return semantics and propagate any returned error rather than wrapping a guaranteed nil.
🤖 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 `@main.go`:
- Line 66: OBJ_PERMISSIONS is read into objPermissions in main.go but no
default/validation is applied, causing UpdatePermissions in PaperlessClient to
error when objPermissions == ""; update validateOrDefaultEnvVars to set the
default and validate: if objPermissions == "" set it to "client", accept only
"client", "none", or "document" and call log.Fatalf on invalid values, and log
the chosen value (reference symbols: objPermissions variable,
validateOrDefaultEnvVars function, PaperlessClient.UpdatePermissions).
In `@ocr.go`:
- Around line 484-488: The call to app.Client.UpdatePermissions(ctx,
&originalDoc) is discarding its returned error (variable shadowing), so
permission-resolution failures are ignored; change the invocation to capture and
check the error returned by UpdatePermissions (e.g., assign to err returned from
app.Client.UpdatePermissions or a new variable and then run the existing if err
!= nil block) so that any error from UpdatePermissions is handled and returned;
locate the call to app.Client.UpdatePermissions in ocr.go near the originalDoc
variable and ensure the error is not shadowed by a previous err from
GetDocument.
In `@paperless.go`:
- Around line 202-240: The UpdatePermissions method mutates shared
client.ObjPermissions without synchronization, causing races and cross-request
leakage (e.g., between concurrent UpdatePermissions for different documents and
operations like CreateTag/CreateOrGetCorrespondent reading permissions). Fix by
removing the shared mutation: change UpdatePermissions (and GetPermissions) to
resolve and return an ObjPermissions value (not modify client.ObjPermissions)
and thread that returned ObjPermissions through callers such as
CreateTag/CreateOrGetCorrespondent/CreateOrGetCorrespondent so each operation
uses its own copy; alternatively, if you prefer to keep client caching, add a
sync.RWMutex on PaperlessClient protecting ObjPermissions, ensure
UpdatePermissions locks for write, GetPermissions locks for read and returns a
deep copy, and update all callers to use the copied value.
- Around line 569-573: The call to UpdatePermissions is discarding its returned
error (variable shadowing), so replace the standalone call to
client.UpdatePermissions(ctx, &originalDoc) with an assignment capturing the
error (err := client.UpdatePermissions(ctx, &originalDoc) or err =
client.UpdatePermissions(...)) and then immediately check if err != nil and
return fmt.Errorf("error updating permissions: %w", err); ensure you reference
the same originalDoc passed in and avoid redeclaring a new err variable that
would shadow the outer err from GetAllDocumentTypes/GetUiSettings so
permission-resolution failures are propagated correctly.
In `@types.go`:
- Around line 9-34: The new struct definitions (UiSettingsPermissions,
Permissions and its anonymous inner structs, ObjPermissions, plus
UiSettingsUser, UiSettingsSettings, UiSettings and the modified Correspondent)
are not gofmt-formatted; run the Go formatter (gofmt or go fmt) on the file to
reflow and align field declarations and struct tags so types and `json` tags
line up consistently, then re-commit the formatted file so
UiSettingsPermissions, Permissions (and its inner Users/Groups blocks),
ObjPermissions, UiSettingsUser, UiSettingsSettings, UiSettings and Correspondent
match gofmt style.
- Around line 181-188: The Correspondent struct change causes Owner to be
serialized as "owner": null when nil which may break older paperless-ngx
endpoints; update the json tag for the Owner field in the Correspondent type to
include omitempty (i.e., change the Owner field's tag to drop the field when
nil) so instantiateCorrespondent in paperless.go won't emit "owner": null, and
ensure SetPermissions remains *SetPermissions `json:"set_permissions,omitempty"`
as-is; alternatively, if you verify paperless-ngx POST /api/correspondents/
reliably accepts explicit null for owner, leave as-is — otherwise add omitempty
to Owner.
---
Nitpick comments:
In `@paperless.go`:
- Around line 56-60: The TagRequest struct fields (Name, Owner, SetPermissions)
are misaligned and need gofmt formatting; run gofmt (e.g., gofmt -w
paperless.go) or your editor's Go formatter to reflow and align the struct field
declarations and tags so the file is properly formatted.
- Around line 226-240: The code caches client.ObjPermissions (Owner and
SetPermissions) in the branch handling objPermissions == "client" and never
refreshes it, causing stale permissions when paperless-ngx UI defaults change;
update the logic so ObjPermissions is refreshed periodically or on demand:
either call client.GetUiSettings() to repopulate client.ObjPermissions inside
the existing refreshCustomFieldsCache goroutine (piggy-back the update there)
or, if traffic is low, re-fetch UI settings at the start of the code path that
checks objPermissions == "client" (replace the current one-time population logic
that sets client.ObjPermissions.Owner and client.ObjPermissions.SetPermissions).
Ensure you use the existing client.GetUiSettings(ctx), update
client.ObjPermissions.Owner and client.ObjPermissions.SetPermissions via
mapPermissions(&uiSettings.Settings.Permissions), and avoid leaking errors
(return or log errors as appropriate).
- Around line 243-245: GetPermissions currently returns a pointer into
client.ObjPermissions and a nil error which is misleading and unsafe; change it
to actively ensure permissions are initialized and return a copy and a real
error: have GetPermissions(ctx) call UpdatePermissions(ctx, nil) when
ObjPermissions is uninitialized (covering client/none modes) so any
initialization error is returned, and return a value copy (not
&client.ObjPermissions) to avoid external mutation; update callers (CreateTag,
CreateOrGetCorrespondent) to accept the value return semantics and propagate any
returned error rather than wrapping a guaranteed nil.
In `@types.go`:
- Around line 63-64: The new struct fields Owner and Permissions in
GetDocumentApiResponseResult, GetDocumentApiResponse, and Document have
misaligned columns; run gofmt (or gofmt -w types.go) to reformat the file so
field names, types, and tags are properly aligned (ensure the added fields Owner
int and Permissions DocumentPermissions are aligned consistently with
surrounding fields and that JSON tags remain intact).
- Around line 146-161: The new struct declarations UiSettingsUser,
UiSettingsSettings, and UiSettings have inconsistent spacing/alignment in their
field tags; run gofmt (or goimports) to normalize indentation and import
formatting and ensure each struct field (Id, Username, IsStaff, IsSuperuser,
Groups, Permissions, User, Settings) follows standard gofmt layout, then
re-save/commit the file so types.go is properly formatted.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a9600f7-5349-4257-9969-bb6cad08563f
📒 Files selected for processing (5)
README.mdmain.goocr.gopaperless.gotypes.go
Although unlikely, client permissions may change during runtime. However, caching may lead to inconsistency as long as paperless does not provide a callback / event mechanism. Therefore we need to check for updated permissions on every call. The alternative would be pull the permissions only once and mention it in the documentation such that users may restart paperless-gpt when the configuration changes.
CodeRabbit: 1. The function signature returns error but the implementation cannot fail — callers (CreateTag, CreateOrGetCorrespondent) defensively wrap with fmt.Errorf(... %w), which is now misleading. Either drop the error return, or have it actively populate ObjPermissions on first use (calling UpdatePermissions(ctx, nil) for client/none modes) so the call is meaningful. 2. Returning a pointer directly into client.ObjPermissions lets callers mutate the cached state. Returning a value (or copy) is safer and pairs with the concurrency fix flagged above.
Caching ObjPermissions in Client opens the door for data inconsistency through concurrent mutation. This fix solves this issue by querying the API every time for the permissions. Alternatively a locking mechanism (e.g. mutex) could be implemented.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
paperless.go (1)
1231-1245:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard
objPermsbefore dereferencing it.
GetPermissionsfailures are already logged-and-continued by callers in this PR, soobjPerms == nilis reachable here. Both of these dereferences panic on that path.🛠️ Minimal guard
func (client *PaperlessClient) CreateOrGetCorrespondent(ctx context.Context, correspondent Correspondent, objPerms *ObjPermissions) (int, error) { @@ - correspondent.Owner = objPerms.Owner - correspondent.SetPermissions = objPerms.SetPermissions + if objPerms != nil { + correspondent.Owner = objPerms.Owner + correspondent.SetPermissions = objPerms.SetPermissions + } @@ func (client *PaperlessClient) CreateTag(ctx context.Context, tagName string, objPerms *ObjPermissions) (int, error) { @@ - tagRequest.Owner = objPerms.Owner - tagRequest.SetPermissions = objPerms.SetPermissions + if objPerms != nil { + tagRequest.Owner = objPerms.Owner + tagRequest.SetPermissions = objPerms.SetPermissions + }Also applies to: 1427-1432
🤖 Prompt for 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. In `@paperless.go` around lines 1231 - 1245, Guard against a nil objPerms before dereferencing in CreateOrGetCorrespondent: check if objPerms == nil and return a clear error (or handle the nil case) instead of unconditionally assigning correspondent.Owner and correspondent.SetPermissions; update the same pattern found around the other occurrence (the block that sets Owner/SetPermissions later in the file) to prevent panics when GetPermissions returned nil. Ensure you reference objPerms, CreateOrGetCorrespondent, correspondent.Owner and correspondent.SetPermissions when making the change.
🤖 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 `@main.go`:
- Around line 778-785: The validation is incorrectly comparing ocrProcessMode
instead of objPermissions, causing valid OBJ_PERMISSIONS=document and
OBJ_PERMISSIONS=none to be rejected; update the conditional that currently reads
else if objPermissions != "client" && ocrProcessMode != "document" &&
ocrProcessMode != "none" to compare objPermissions against the allowed values
("client", "document", "none") so only unknown objPermissions values are reset
to "client", and keep the same log.Warnf and assignment to objPermissions =
"client" for invalid cases.
In `@paperless.go`:
- Around line 603-605: In UpdateDocuments the variable logger is undefined
causing compile errors in the new warning branches (e.g., around the
client.GetPermissions call and the later branch at lines ~636-638); fix by
either using the package-level logger (e.g., the existing package logger object)
or by creating a local logger entry at the top of UpdateDocuments (e.g., logger
:= <packageLogger>.WithFields(...) or similar) and replace the undefined logger
references (used with .WithError(err).Warn calls) with that defined logger so
the warnings compile and include context.
---
Outside diff comments:
In `@paperless.go`:
- Around line 1231-1245: Guard against a nil objPerms before dereferencing in
CreateOrGetCorrespondent: check if objPerms == nil and return a clear error (or
handle the nil case) instead of unconditionally assigning correspondent.Owner
and correspondent.SetPermissions; update the same pattern found around the other
occurrence (the block that sets Owner/SetPermissions later in the file) to
prevent panics when GetPermissions returned nil. Ensure you reference objPerms,
CreateOrGetCorrespondent, correspondent.Owner and correspondent.SetPermissions
when making the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6745e939-5e01-487a-873f-6e2c67d51884
📒 Files selected for processing (4)
main.goocr.gopaperless.gotypes.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Paperless-gpt may access paperless-ngx through an arbitrary account, e.g. admin, regular, or a dedicated account set up specifically for paperless-gpt.
As of now new objects (e.g. tags, correspondents) are created with no owner or permission handling at all resulting in objects which belong to the user account which is configured to provide access for paperless-gpt.
If the account is, e.g. a dedicated account, most other accounts won't be able to see the objects of a document, which will be displayed as "Private" by paperless-ngx. To solve this problem, this patch introduces three different options for setting ownership of new objects:
client: this is the default behaviour and will create an object owned by the user (and its permissions) which is configured to provide access for paperless-gpt. Default permissions are configurable through the web ui of paperless-ngx (Administration/Settings/Permissions)none: No ownership or permissions are set on objects, making them globally read- and write(!)able for every userdocument: The owner and the permissions of the current document will be used for new objects.Summary by CodeRabbit