Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
217 changes: 217 additions & 0 deletions CASE_SEARCH_CACHE_FIX.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,217 @@
# Case Search Cache Key Bug Fix

## Problem Description

An intermittent bug was causing wrong cases to be returned from case search requests. The bug only occurred when:
1. A user with **superuser privileges** AND domain membership made a case search request
2. A regular domain member (WITHOUT superuser privileges) made the same case search request
3. Both users accessed the same domain with identical query parameters

The issue was a **cache collision** - the cache key didn't distinguish between superuser and non-superuser domain members, so they shared cached results despite superusers having broader access permissions that bypass normal ownership/location restrictions.

**Note**: The bug could NOT be reproduced when a superuser accessed a domain they were NOT a member of, likely because those requests may not use the same caching mechanism or have different query patterns.

## Root Cause

The cache key generation in `CaseSearchHelper.getCacheKey()` was missing critical authorization context:

### Original Cache Key Components
- Domain name
- Username (scrubbed)
- As Username (if restoring as another user)
- URL
- Query parameters

### What Was Missing
- **Superuser status** - Whether the user has global access to all domains
- **Domain membership status** - Whether the user is an authorized member of the domain

## Why This Caused Wrong Cases

### Authorization Logic
From `HqUserDetailsBean.isAuthorized()`:
```java
public boolean isAuthorized(String domain, String username) {
return isSuperUser || Arrays.asList(domains).contains(domain) && this.username.equals(username);
}
```

**Superusers**: Can access ANY domain, bypass ALL permission checks
**Domain members**: Must be in domain's member list, subject to ownership/location/permission filters

### The Bug Scenario

Both users in this scenario are domain members, but have different permission levels:
- **User A**: Domain member WITH superuser privileges → sees more cases (bypasses restrictions)
- **User B**: Domain member WITHOUT superuser privileges → sees filtered cases (has restrictions)

**Scenario A: Superuser Member First (BUG)**
1. User A (superuser + domain member of `co-carecoordination-dev`) makes case search
2. HQ backend detects superuser → Returns ALL accessible cases, bypassing normal ownership/location restrictions
3. Results cached with key (OLD): `co-carecoordination-dev_mriese@dimagi.com_<url>_<params>`
4. User B (regular domain member, non-superuser) makes identical request
5. Cache key matches! User B gets cached superuser results → **WRONG CASES RETURNED** (sees cases they shouldn't have access to)

**Scenario B: Regular Member First (Appears to Work)**
1. User B (regular domain member) makes request
2. HQ applies normal member filtering (ownership, location restrictions)
3. Results cached with key (OLD): `co-carecoordination-dev_regularuser_<url>_<params>`
4. User A (superuser member) makes identical request
5. Cache key matches! User A gets cached restricted results → Appears to work, but superuser sees fewer cases than they should

This explains the **intermittent nature** - it depends on which user type hits the cache first!

**Example of the Cache Key Collision (OLD CODE)**:
```
Superuser domain member cache key: co-carecoordination-dev_mriese@dimagi.com_http://...
Regular domain member cache key: co-carecoordination-dev_regularuser_http://...
^^ SAME KEY FORMAT - no superuser distinction!
```

When `mriese@dimagi.com` happens to be both the superuser AND the regular user making requests (e.g., testing with the same account), the keys become IDENTICAL:
```
Superuser domain member: co-carecoordination-dev_mriese@dimagi.com_http://...
Regular domain member: co-carecoordination-dev_mriese@dimagi.com_http://...
^^^ EXACT SAME KEY = CACHE COLLISION!
```

The old cache key couldn't distinguish between domain members with different privilege levels.

## The Fix

### Changes Made

**File: `formplayer/src/main/java/org/commcare/formplayer/services/CaseSearchHelper.java`**

1. **Added imports** for user authorization context:
- `org.commcare.formplayer.beans.auth.HqUserDetailsBean`
- `org.commcare.formplayer.util.RequestUtils`
- `java.util.Optional`

2. **Enhanced `getCacheKey()` method** to include authorization context:
```java
// Include user authorization context in cache key
Optional<HqUserDetailsBean> userDetails = RequestUtils.getUserDetails();
if (userDetails.isPresent()) {
HqUserDetailsBean user = userDetails.get();
String domain = restoreFactory.getDomain();
String username = restoreFactory.getScrubbedUsername();

// Include superuser status
boolean isSuperUser = user.isSuperUser();
builder.append("_superuser=").append(isSuperUser);

// Include domain membership status
boolean isDomainMember = user.isAuthorized(domain, username);
builder.append("_member=").append(isDomainMember);
}
```

3. **Added debug logging** to track cache key generation:
- Logs authorization context (domain, user, superuser status, membership status)
- Logs generated cache key
- Enhanced cache hit/miss logging to include cache key

### New Cache Key Format

**Before:**
```
domain_username_asuser_url_queryparams
```

**After:**
```
domain_username_asuser_superuser=[true/false]_member=[true/false]_url_queryparams
```

### Example Cache Keys

**Domain member WITH superuser privileges:**
```
co-carecoordination-dev_mriese@dimagi.com_superuser=true_member=true_http://...
```

**Domain member WITHOUT superuser privileges:**
```
co-carecoordination-dev_regularuser_superuser=false_member=true_http://...
```
**Key Point**: Both users have `member=true`, but different `superuser` values. The old cache key didn't include `superuser`, so these two users shared the same cache entry and got wrong results!

**Non-member superuser (accessing via superuser privileges only):**
```
co-carecoordination-dev_mriese@dimagi.com_superuser=true_member=true_http://...
```
Note: `isAuthorized()` returns `true` for superusers even without explicit domain membership, so `member=true` appears here too. However, this scenario wasn't reproducible in testing.

**Non-member non-superuser (unauthorized):**
```
co-carecoordination-dev_regularuser_superuser=false_member=false_http://...
```

## Testing

### New Tests Added

**File: `formplayer/src/test/java/org/commcare/formplayer/tests/CaseClaimNavigationTests.java`**

1. `testCacheKey_DomainMemberVsSuperuser()` - Verifies domain member cache key includes correct flags
2. `testCacheKey_SuperuserNotMember()` - Verifies superuser cache key includes correct flags
3. `testCacheKey_NonMemberNonSuperuser()` - Verifies unauthorized user cache key includes correct flags
4. `testCacheKey_IncludesAuthorizationContext()` - Verifies all cache keys include auth context

### Running Tests
```bash
./gradlew test --tests CaseClaimNavigationTests.testCacheKey*
```

## Impact

### Before Fix
- Domain members with different privilege levels (superuser vs non-superuser) shared cached results
- Wrong cases intermittently returned based on who cached first:
- Regular members could see cases they shouldn't have access to (if superuser cached first)
- Superusers could see fewer cases than expected (if regular member cached first)
- Security/permission boundary violations possible

### After Fix
- Each authorization context gets its own cache entry
- Domain members with superuser privileges never share cache with non-superuser domain members
- Correct cases always returned regardless of cache state or request order
- Proper permission boundaries maintained

## Debugging

### Log Messages to Watch For

**Cache key generation:**
```
DEBUG CaseSearchHelper - Case search cache key includes auth context: domain=co-carecoordination-dev, user=mriese@dimagi.com, isSuperUser=true, isDomainMember=true
DEBUG CaseSearchHelper - Generated case search cache key: co-carecoordination-dev_mriese@dimagi.com_superuser=true_member=true_...
```

**Cache hit/miss:**
```
INFO CaseSearchHelper - Cache HIT for case search: url=http://..., key=...
INFO CaseSearchHelper - Cache MISS for case search: url=http://..., key=...
```

### Verifying the Fix

To verify the fix is working:
1. Enable DEBUG logging for `CaseSearchHelper`
2. Make a case search request as a domain member WITH superuser privileges
3. Check logs for cache key with `superuser=true_member=true`
4. Make the same request as a domain member WITHOUT superuser privileges
5. Check logs for cache key with `superuser=false_member=true`
6. Verify the cache keys are DIFFERENT (they now differ in the `superuser` component)
7. Verify that each user gets correct results without seeing the other's cached data

## Related Code

- `formplayer/src/main/java/org/commcare/formplayer/beans/auth/HqUserDetailsBean.java` - User authorization logic
- `formplayer/src/main/java/org/commcare/formplayer/util/RequestUtils.java` - Request context utilities
- `formplayer/src/main/java/org/commcare/formplayer/services/RestoreFactory.java` - User restore configuration

## Future Considerations

This fix ensures proper cache isolation based on authorization context. If additional factors affect case search results (e.g., location hierarchy, user roles), those should also be considered for inclusion in the cache key.
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,14 @@
import org.commcare.core.parse.CaseInstanceXmlTransactionParserFactory;
import org.commcare.core.parse.ParseUtils;
import org.commcare.formplayer.DbUtils;
import org.commcare.formplayer.beans.auth.HqUserDetailsBean;
import org.commcare.formplayer.database.models.FormplayerCaseIndexTable;
import org.commcare.formplayer.sandbox.CaseSearchSqlSandbox;
import org.commcare.formplayer.sandbox.UserSqlSandbox;
import org.commcare.formplayer.session.MenuSession;
import org.commcare.formplayer.sqlitedb.CaseSearchDB;
import org.commcare.formplayer.sqlitedb.SQLiteDB;
import org.commcare.formplayer.util.RequestUtils;
import org.commcare.formplayer.util.SerializationUtil;
import org.commcare.formplayer.web.client.WebClient;
import org.commcare.util.screen.ScreenUtils;
Expand Down Expand Up @@ -46,6 +48,7 @@
import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.TreeMap;

@CacheConfig(cacheNames = "case_search")
Expand Down Expand Up @@ -170,15 +173,17 @@ private boolean shouldParseIntoCaseSearchStorage(boolean useCaseTemplate) {

private TreeElement getCachedRoot(Cache cache, String cacheKey, String url, boolean skipCache) {
if (skipCache) {
log.info("Skipping cache check for case search results");
log.info(String.format("Skipping cache check for case search results. Key: %s", cacheKey));
} else {
TreeElement cachedRoot = cache.get(cacheKey, TreeElement.class);
if (cachedRoot != null) {
log.info(String.format("Using cached case search results for %s", url));
log.info(String.format("Cache HIT for case search: url=%s, key=%s", url, cacheKey));
// Deep copy to avoid concurrency issues
TreeElement copyOfRoot = SerializationUtil.deserialize(ExtUtil.serialize(cachedRoot),
TreeElement.class);
return copyOfRoot;
} else {
log.info(String.format("Cache MISS for case search: url=%s, key=%s", url, cacheKey));
}
}
return null;
Expand Down Expand Up @@ -229,6 +234,28 @@ private String getCacheKey(String url, Multimap<String, String> queryParams) thr
if (restoreFactory.getAsUsername() != null) {
builder.append("_").append(restoreFactory.getAsUsername());
}

// Include user authorization context in cache key to prevent superusers and domain members
// from sharing cached case search results, as they may have different access permissions
Optional<HqUserDetailsBean> userDetails = RequestUtils.getUserDetails();
if (userDetails.isPresent()) {
HqUserDetailsBean user = userDetails.get();
String domain = restoreFactory.getDomain();
String username = restoreFactory.getScrubbedUsername();

// Include superuser status
boolean isSuperUser = user.isSuperUser();
builder.append("_superuser=").append(isSuperUser);

// Include domain membership status (superusers can access any domain, members must be authorized)
boolean isDomainMember = user.isAuthorized(domain, username);
builder.append("_member=").append(isDomainMember);

log.debug(String.format(
"Case search cache key includes auth context: domain=%s, user=%s, isSuperUser=%s, isDomainMember=%s",
domain, username, isSuperUser, isDomainMember));
}

builder.append("_").append(uri);
Map<String, Collection<String>> sortedQueryParams = new TreeMap<>(queryParams.asMap());
for (String key : sortedQueryParams.keySet()) {
Expand All @@ -239,7 +266,9 @@ private String getCacheKey(String url, Multimap<String, String> queryParams) thr
builder.append("=").append(value);
}
}
return builder.toString();
String cacheKey = builder.toString();
log.debug(String.format("Generated case search cache key: %s", cacheKey));
return cacheKey;
}

public void clearCache() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.commcare.formplayer.services.CaseSearchHelper;
import org.commcare.formplayer.utils.MockRequestUtils;
import org.commcare.formplayer.utils.TestContext;
import org.commcare.formplayer.utils.WithHqUser;
import org.commcare.session.CommCareSession;
import org.commcare.session.SessionFrame;
import org.commcare.suite.model.StackFrameStep;
Expand Down Expand Up @@ -401,4 +402,89 @@ private ExternalDataInstanceSource getInstanceSourceFromSession(String sessionId
}
return null;
}

/**
* Test that superusers and domain members get different cache keys for case search.
* This prevents the bug where a superuser's case search results (with different permissions)
* are incorrectly returned to a domain member, or vice versa.
*/
@Test
@WithHqUser(domain = "caseclaimdomain", username = "caseclaimusername", domains = {"caseclaimdomain"}, isSuperUser = false)
public void testCacheKey_DomainMemberVsSuperuser() {
ImmutableMultimap<String, String> data = ImmutableMultimap.of(
"case_type", "patient",
"name", "John");

// Get cache key as domain member (not superuser)
String memberKey = ReflectionTestUtils.invokeMethod(
caseSearchHelper, "getCacheKey", "http://localhost:8000/a/caseclaimdomain/phone/search/", data);

assertNotNull(memberKey);
// Verify the key contains member=true and superuser=false
assertThat(memberKey).contains("superuser=false");
assertThat(memberKey).contains("member=true");
}

@Test
@WithHqUser(domain = "caseclaimdomain", username = "superuser@dimagi.com", domains = {}, isSuperUser = true)
public void testCacheKey_SuperuserNotMember() {
ImmutableMultimap<String, String> data = ImmutableMultimap.of(
"case_type", "patient",
"name", "John");

// Get cache key as superuser who is NOT a member of the domain
String superuserKey = ReflectionTestUtils.invokeMethod(
caseSearchHelper, "getCacheKey", "http://localhost:8000/a/caseclaimdomain/phone/search/", data);

assertNotNull(superuserKey);
// Verify the key contains member=false and superuser=true
assertThat(superuserKey).contains("superuser=true");
assertThat(superuserKey).contains("member=true"); // superuser.isAuthorized() returns true even without domain membership
}

@Test
@WithHqUser(domain = "caseclaimdomain", username = "regularuser", domains = {"otherdomain"}, isSuperUser = false)
public void testCacheKey_NonMemberNonSuperuser() {
ImmutableMultimap<String, String> data = ImmutableMultimap.of(
"case_type", "patient",
"name", "John");

// Get cache key as regular user who is NOT a member of caseclaimdomain
String nonMemberKey = ReflectionTestUtils.invokeMethod(
caseSearchHelper, "getCacheKey", "http://localhost:8000/a/caseclaimdomain/phone/search/", data);

assertNotNull(nonMemberKey);
// Verify the key contains member=false and superuser=false
assertThat(nonMemberKey).contains("superuser=false");
assertThat(nonMemberKey).contains("member=false");
}

/**
* This test verifies that the cache key generation properly distinguishes between
* different authorization contexts. The key difference is that the individual tests
* above with @WithHqUser annotations will produce different cache keys based on
* superuser status and domain membership, preventing the bug where wrong cases
* are returned due to cache collisions.
*/
@Test
@WithHqUser(domain = "caseclaimdomain", username = "member", domains = {"caseclaimdomain"}, isSuperUser = false)
public void testCacheKey_IncludesAuthorizationContext() {
ImmutableMultimap<String, String> data = ImmutableMultimap.of(
"case_type", "patient",
"name", "John");

String cacheKey = ReflectionTestUtils.invokeMethod(
caseSearchHelper, "getCacheKey", "http://localhost:8000/a/caseclaimdomain/phone/search/", data);

assertNotNull(cacheKey);
// Verify the cache key includes authorization context
assertThat(cacheKey).as("Cache key must include superuser status")
.contains("superuser=");
assertThat(cacheKey).as("Cache key must include domain membership status")
.contains("member=");

// This ensures that users with different authorization contexts (superuser vs member)
// will never share cached case search results, preventing the bug where wrong cases
// are returned to users.
}
}
Loading