mirror of
https://github.com/simplex-chat/simplex-chat.git
synced 2026-10-06 03:27:58 +00:00
core: don't re-approve directory channels on stale publicGroupId
updateGroupFromLinkData reported profileChanged via a full structural (/=) that includes publicGroupId - an immutable identity field updateGroupProfile never re-syncs. A directory whose stored public_group_id drifts from the link's therefore re-approved the channel on every link check (~30 min). Fix: normalize publicGroupId out of the comparison (mirrors sameGroupProfileInfo), so only fields updateGroupProfile actually manages drive the change signal. The (pg :: PublicGroupProfile) annotation is required (DuplicateRecordFields). Adds a directory regression test: register+approve a channel, corrupt the stored public_group_id via /x /sql, run a link check, assert the channel stays listed (no re-approval). Builds verified: simplex-directory-service and simplex-chat-test both compile.
This commit is contained in:
@@ -1,6 +1,6 @@
|
||||
# Fix: SimpleX directory re-approves a channel every 30 min after a blockchain name is added
|
||||
|
||||
Status: proposed — **root cause confirmed by code (deduction, §3); fix in core (§4)**
|
||||
Status: implemented & verified — **root cause confirmed by code (deduction, §3); fix in core (§4); regression test passes and fails without the fix (§8)**
|
||||
Branch: `nd/fix-directory-names`
|
||||
Base: `origin/master` (`b38015c7b`)
|
||||
|
||||
@@ -107,7 +107,7 @@ sites — the guard, the `if … then updateGroupProfile`, and the returned flag
|
||||
-- Internal.hs:2986, which normalizes groupPreferences the same way).
|
||||
profileChanged = clearId p /= clearId groupProfile
|
||||
clearId gp@GroupProfile {publicGroup} =
|
||||
gp {publicGroup = (\pg -> pg {publicGroupId = B64UrlByteString ""}) <$> publicGroup}
|
||||
gp {publicGroup = (\pg -> (pg :: PublicGroupProfile) {publicGroupId = B64UrlByteString ""}) <$> publicGroup}
|
||||
```
|
||||
|
||||
The `| profileChanged || countChanged || verifyChanged` guard, the
|
||||
@@ -207,16 +207,24 @@ not a directory policy, so it belongs where the comparison lives:
|
||||
`profileChanged` true and stores). `Commands.hs:4358` is the directory path this
|
||||
fix targets.
|
||||
|
||||
## 8. Verification plan
|
||||
## 8. Verification (done)
|
||||
|
||||
1. **Regression test** in `tests/Bots/DirectoryTests.hs` (beside the existing
|
||||
"link/whitespace-only must not re-approve" tests, `:296-326`): register a channel
|
||||
with one `public_group_id`, then make the fetched link carry a *different*
|
||||
`publicGroupId` (all other fields equal), run ≥2 link checks, and assert the reg
|
||||
stays `GRSActive` (no `GRSPendingApproval`, no owner re-approval message). Add a
|
||||
positive control: a display-name change *does* re-approve.
|
||||
2. Apply §4.
|
||||
3. Build the directory service (statically links core); run the directory suite.
|
||||
Regression test `testLinkCheckStalePublicGroupId` in `tests/Bots/DirectoryTests.hs`
|
||||
(beside the existing "link/whitespace-only must not re-approve" tests): register +
|
||||
approve a channel, then corrupt the directory's *stored* `public_group_id` via
|
||||
`/x /sql chat UPDATE group_profiles SET public_group_id = randomblob(32) …` (so it
|
||||
differs from the link's; the value stays non-NULL, so `publicGroup` remains `Just`
|
||||
and the link check still runs), let a link check run (`linkCheckInterval = 1`), and
|
||||
assert the channel stays listed with no re-approval.
|
||||
|
||||
Results:
|
||||
- `cabal build simplex-directory-service` — links (the fix compiles into the bot).
|
||||
- `cabal build simplex-chat-test` — the test compiles.
|
||||
- Test **passes** with the fix (1 example, 0 failures).
|
||||
- **Negative control** (fix reverted, test kept): the test **fails** with
|
||||
`but got: Just "…The channel ID 1 (news) profile changed."` — the exact production
|
||||
re-approval message. So the test genuinely guards the bug, and the root cause is
|
||||
demonstrated end-to-end (not just deduced).
|
||||
|
||||
## 9. Files touched
|
||||
|
||||
|
||||
@@ -1479,7 +1479,13 @@ updateGroupFromLinkData user gInfo@GroupInfo {groupProfile = p, groupDomainVerif
|
||||
pure (g'', profileChanged)
|
||||
| otherwise = pure (gInfo, False)
|
||||
where
|
||||
profileChanged = p /= groupProfile
|
||||
-- publicGroupId is immutable identity, not content, and is not a column
|
||||
-- updateGroupProfile syncs (it is set at group creation / by updateRelayGroupKeys).
|
||||
-- A stale stored value would otherwise make this (/=) re-trigger directory
|
||||
-- approval on every link check. Normalize it out (mirrors sameGroupProfileInfo).
|
||||
profileChanged = clearId p /= clearId groupProfile
|
||||
clearId gp@GroupProfile {publicGroup} =
|
||||
gp {publicGroup = (\pg -> (pg :: PublicGroupProfile) {publicGroupId = B64UrlByteString ""}) <$> publicGroup}
|
||||
countChanged = case publicGroupData of
|
||||
Just PublicGroupData {publicMemberCount} -> Just publicMemberCount /= localCount
|
||||
_ -> False
|
||||
|
||||
@@ -99,6 +99,7 @@ directoryServiceTests = do
|
||||
it "should delete channel registration and leave" testDeleteChannelRegistration
|
||||
it "should handle re-registration when already listed" testReregistrationAlreadyListed
|
||||
it "should update subscriber count periodically" testLinkCheckUpdatesCount
|
||||
it "should not re-approve when only stored publicGroupId is stale" testLinkCheckStalePublicGroupId
|
||||
|
||||
directoryProfile :: Profile
|
||||
directoryProfile = Profile {displayName = "SimpleX Directory", fullName = "", shortDescr = Nothing, image = Nothing, contactLink = Nothing, peerType = Just CPTBot, preferences = Nothing, badge = Nothing, contactDomain = Nothing}
|
||||
@@ -2307,6 +2308,75 @@ testLinkCheckUpdatesCount ps = do
|
||||
bob <## "You need SimpleX Chat app v6.5 to join."
|
||||
bob <## "3 subscribers"
|
||||
|
||||
-- Regression: a stale stored publicGroupId (which updateGroupProfile never re-syncs)
|
||||
-- differs from the link's on every link check. Before the fix this re-approved the
|
||||
-- channel every linkCheckInterval; after it, a publicGroupId-only difference is
|
||||
-- ignored and the channel stays listed. (See plans/2026-07-06-fix-directory-name-reapproval.md)
|
||||
testLinkCheckStalePublicGroupId :: HasCallStack => TestParams -> IO ()
|
||||
testLinkCheckStalePublicGroupId ps = do
|
||||
dsLink <-
|
||||
withNewTestChatCfg ps testCfg serviceDbPrefix directoryProfile $ \ds ->
|
||||
withNewTestChatCfg ps testCfg "super_user" aliceProfile $ \superUser -> do
|
||||
connectUsers ds superUser
|
||||
ds ##> "/ad"
|
||||
getContactLink ds True
|
||||
let opts = (mkDirectoryOpts ps [KnownContact 2 "alice"] Nothing Nothing) {linkCheckInterval = 1}
|
||||
runDirectory testCfg opts $
|
||||
withTestChatCfg ps testCfg "super_user" $ \superUser -> do
|
||||
superUser <## "subscribed 1 connections on server localhost"
|
||||
withNewTestChatCfg ps testCfg "bob" bobProfile $ \bob ->
|
||||
withRelay ps $ \relay -> do
|
||||
bob `connectVia` dsLink
|
||||
_ <- prepareChannel1Relay "news" bob relay
|
||||
-- register and approve
|
||||
bob ##> "/share chat #news @'SimpleX Directory'"
|
||||
bob <# "@'SimpleX Directory' link to join channel #news (signed):"
|
||||
_ <- getTermLine bob -- short link
|
||||
_ <- getTermLine bob -- ownerSig JSON
|
||||
bob <# "'SimpleX Directory'> Joining the channel news…"
|
||||
concurrentlyN_
|
||||
[ do
|
||||
relay <## "'SimpleX Directory': accepting request to join group #news..."
|
||||
relay <## "#news: 'SimpleX Directory' joined the group",
|
||||
bob <## "#news: relay introduced 'SimpleX Directory_1' in the channel"
|
||||
]
|
||||
bob <# "'SimpleX Directory'> Joined the channel news. Registration is pending approval — it may take up to 48 hours."
|
||||
bob <# "'SimpleX Directory'> We recommend allowing direct messages, media, voice, and SimpleX links only for group moderators and admins. Use group preferences to set them."
|
||||
bob <## "Captcha verification is enabled. Use /'filter 1' to change it."
|
||||
superUser <# "'SimpleX Directory'> bob submitted the channel ID 1:"
|
||||
superUser <## "news"
|
||||
superUser <##. "Link to join channel: "
|
||||
superUser <## "You need SimpleX Chat app v6.5 to join."
|
||||
superUser <## "1 subscribers"
|
||||
superUser <## ""
|
||||
superUser <## "To approve send:"
|
||||
superUser <# "'SimpleX Directory'> /approve 1:news 1"
|
||||
let approve = "/approve 1:news 1"
|
||||
superUser #> ("@'SimpleX Directory' " <> approve)
|
||||
superUser <# ("'SimpleX Directory'> > " <> approve)
|
||||
superUser <## " Channel approved!"
|
||||
bob <# "'SimpleX Directory'> The channel ID 1 (news) is approved and listed in directory - please moderate it!"
|
||||
bob <## "Please note: if you change the channel profile it will be hidden from directory until it is re-approved."
|
||||
-- Corrupt the directory's STORED public_group_id so it differs from the link's
|
||||
-- (simulates the stale identity that updateGroupProfile never re-syncs). Value
|
||||
-- stays non-NULL, so publicGroup remains Just and the link check still runs.
|
||||
let sql = "/x /sql chat UPDATE group_profiles SET public_group_id = randomblob(32) WHERE public_group_id IS NOT NULL"
|
||||
superUser #> ("@'SimpleX Directory' " <> sql)
|
||||
superUser <# ("'SimpleX Directory'> > " <> sql)
|
||||
superUser <## ""
|
||||
-- allow >= 1 link check (interval = 1s) to run
|
||||
threadDelay 1500000
|
||||
-- channel is still listed and active (no re-approval); had it re-approved,
|
||||
-- bob's search would first receive the "profile has changed" message and this
|
||||
-- sequence would fail.
|
||||
bob #> "@'SimpleX Directory' news"
|
||||
bob <# "'SimpleX Directory'> > news"
|
||||
bob <## " Found 1 group(s)."
|
||||
bob <# "'SimpleX Directory'> news"
|
||||
bob <##. "Link to join channel: "
|
||||
bob <## "You need SimpleX Chat app v6.5 to join."
|
||||
bob <## "2 subscribers"
|
||||
|
||||
testGetCaptchaStr :: HasCallStack => TestParams -> IO ()
|
||||
testGetCaptchaStr _ps = do
|
||||
s0 <- getCaptchaStr 0 ""
|
||||
|
||||
Reference in New Issue
Block a user