fix(config): route group avatars to avatar bucket, not group bucket

[lml2468]

Problem

GetGroupAvatarFilePath returned paths prefixed with group/ which routed group avatar images into the private group MinIO/OSS bucket (intended for group exports, not publicly served images). The splitBucketAndObject helper in octo-server selects the destination bucket from the first path segment, so group/... → private group bucket → HTTP 403 on anonymous GET → all auto-generated group avatars broken.

Per the deployment README bucket policy:

Bucket	Policy	Purpose
avatar	download (anonymous GET)	user / group avatars
group	private (signed only)	group exports

Fix

Changed the path prefix from group/ to avatar/group/ so group avatars land in the publicly readable avatar bucket alongside user avatars.

Before: group/{partition}/{groupNo}.png → group bucket (private) → 403
After: avatar/group/{partition}/{groupNo}.png → avatar bucket (public) → ✅

Tests

Added 3 regression tests:

• TestGetGroupAvatarFilePath_BucketPrefix — asserts avatar/ prefix, rejects group/ prefix
• TestGetGroupAvatarFilePath_Deterministic — same input → same output
• TestGetGroupAvatarFilePath_DifferentGroups — distinct groups → distinct paths

Migration note

Deployments upgrading from a version that stored group avatars under the old group/ path should either:

1. Re-generate group avatars (trigger a group-member change event), or
2. Copy existing objects from the group bucket to avatar bucket under the new avatar/group/{partition}/{groupNo}.png key layout.

In practice most deployments already have broken group avatars (403) because the group bucket is private, so there are no working avatars to preserve.

Related issues

• Closes Bug: GetGroupAvatarFilePath returns path under private group bucket, breaking group avatar display #21
• Fixes Bug: auto-generated group avatar stored in private group bucket instead of avatar bucket octo-server#103

[Jerry-Xin]

Summary: Project relevance gate passed; this PR fixes shared octo-lib config behavior and is in scope.

💬 Non-blocking:

• 🔵 Suggestion: config/config_avatar_path_test.go could assert the full expected shape, such as avatar/group/<partition>/<groupNo>.png, instead of only checking prefix/contains/suffix. The current tests still cover the regression adequately.

✅ Highlights:

• config/config.go preserves the existing CRC32 partitioning and only changes the bucket-routing prefix.
• config/config_avatar_path_test.go adds direct regression coverage for avoiding the private group/ bucket route.
• Verified with go test ./...; all tests pass.

[lml2468]

Summary

Clean, correct fix for group avatar bucket routing. The root cause analysis in the PR description is thorough and the fix is minimal.

Findings

No blocking issues.

🟡 Non-blocking observation — GetCommunityAvatarFilePath may have the same class of bug:

GetCommunityAvatarFilePath returns community/%d/%s.png, which would route to a community/ bucket. However, the nginx allow-list only includes file|chat|moment|sticker|report|chatbg|common|download|group|avatar — community is not on this list. If community avatars are publicly served the same way, they may have a similar routing issue. Worth checking as a follow-up (separate PR, not this one).

✅ What this PR does well:

• Precise one-line fix with clear bucket-routing rationale
• Excellent PR description documenting the splitBucketAndObject routing mechanism
• Migration note covers both re-generation and manual copy paths
• 3 focused regression tests: prefix routing, determinism, uniqueness
• Tests verify both positive (avatar/ prefix) and negative (group/ prefix rejected) conditions

Verification

• GetGroupAvatarFilePath now returns avatar/group/{partition}/{groupNo}.png — first segment avatar matches the publicly readable bucket ✅
• CRC32 partition logic unchanged — no behavioral regression ✅
• GetAvatarPath (user avatars) uses a different pattern (users/...) — not affected ✅
• Build ✅, Test ✅, Vet ✅

Verdict

APPROVED — minimal, well-tested fix that correctly routes group avatars to the public avatar bucket.

[lml2468]

[APPROVE]

Correct fix: prefixes the group avatar path with avatar/ so splitBucketAndObject routes it to the publicly readable avatar bucket instead of the private group bucket.

🔵 Suggestion (non-blocking):

1. GetCommunityAvatarFilePath (config.go:901-903) follows the same community/ prefix pattern — if community is also a private bucket, it may have the same 403 issue for community avatars. Worth verifying separately.

✅ Highlights:

• Minimal, surgical fix — only the prefix changes, CRC32 partitioning logic untouched
• Three well-structured regression tests covering prefix, determinism, and uniqueness
• Thorough PR description with bucket policy table, migration guidance, and cross-repo issue links

[yujiawei]

Code Review — PR #52 (octo-lib)

Summary

The change is correct and the fix is sound. GetGroupAvatarFilePath now returns avatar/group/{partition}/{groupNo}.png instead of group/{partition}/{groupNo}.png, which routes group avatars into the public-read avatar bucket on the multi-bucket MinIO backend instead of the private group bucket. This resolves the anonymous-GET 403 on auto-generated group avatars (GH#21 / octo-server#103).

I traced the cross-repo behavior end to end in octo-server and verified there are no functional regressions. No P0/P1 issues. A few non-blocking documentation nits below.

Verification (against octo-server)

✅ MinIO multi-bucket routing is correct. splitBucketAndObject (modules/file/helpers.go:31) selects the bucket from the first path segment only when it is in allowedMinioBuckets (helpers.go:9), which whitelists both avatar and group. The new path avatar/group/{id}/{groupNo}.png therefore resolves to bucket=avatar, object=group/{id}/{groupNo}.png — exactly the intended public bucket.

✅ No path-shape hazards introduced. The object key group/{id}/{groupNo}.png passes validatePresignObjectKey (modules/file/service_minio.go) — no leading slash, no trailing slash, no embedded //. Upload (UploadFile), download (DownloadURL), and presigned URL paths all accept it.

✅ Bucket privacy model holds in production. On managed deployments the avatar (public/download) and group (private) buckets are pre-provisioned by minio-init, and the app credentials deliberately lack s3:CreateBucket (octo-deployment docker/README.zh.md). The server's ensureBucket only applies the anonymous-read policy to buckets it creates and skips SetBucketPolicy for pre-existing buckets (service_minio.go), so it never weakens the pre-provisioned group bucket. The fix relies on this provisioning, which matches the documented deployment contract.

✅ All three consumers are consistent. GetGroupAvatarFilePath is used by group avatar download (modules/group/api.go:389), upload (modules/group/api.go:443), and the compose-on-member-change event (modules/base/event/handler.go:182) — all read the same path, so upload and serve stay in sync.

✅ Regression tests are appropriate and pass locally (go test ./config/ -run TestGetGroupAvatarFilePath). They assert the avatar/ prefix, reject the group/ prefix, and cover determinism and per-group uniqueness.

Security note (security-sensitive PR)

The change broadens read exposure for group avatars by design: group avatar images move from a signed-only bucket to an anonymous-GET bucket, which is the intended and documented policy (group avatars are non-sensitive public images, same class as user avatars). Object keys are derived from groupNo via CRC32 partitioning, not high-entropy UUIDs, so the keys are guessable — but the avatar bucket is explicitly an anonymous-read bucket and group avatars are not confidential, so this is acceptable. Worth a human confirming that no deployment treats the group bucket's privacy as a control over group avatar confidentiality (as opposed to group exports, which keep their own group-bucket path and are unaffected by this change).

Non-blocking suggestions (P2 / nits)

1. Doc-comment inaccuracy — community avatars. The new comment states the avatar bucket is "used by all avatars (user, group, community)". This is inaccurate for community avatars: GetCommunityAvatarFilePath returns community/..., and community is not in allowedMinioBuckets, so community avatars fall back to the default file bucket — they do not land in the avatar bucket. Suggest narrowing the wording to "user and group avatars".

2. Migration note slightly overstated. The PR body says "in practice most deployments already have broken group avatars (403)". That holds for the multi-bucket MinIO backend. On single-bucket backends (OSS / S3 / COS), the first path segment is part of the object key within one public bucket (service_oss.go, service_s3.go, service_cos.go), so those deployments did not have a 403 — their existing group avatars were served fine under the old group/... key and will now 404 under the new avatar/group/... key until regenerated or copied. The fix is still correct for them (regeneration on the next group-member change restores the avatar), but the migration impact differs by backend and is worth calling out so single-bucket operators know to expect a transient 404 rather than assuming nothing was working before.

Neither item blocks merge; both are documentation-only.

Verdict

APPROVED. Functionally correct, well-tested, and the security broadening is intentional and matches the documented bucket policy. The two nits are doc-only and can be addressed in this PR or a follow-up.