refactor collect_session_recipients in advance of some behavioural changes

[uhoreg]

This is part of #3662, pulled out to into a separate PR. Recent changes in main made it pretty much impossible to merge this section of code from main into that PR, and Rich wanted to see the refactoring bits separate from the behavioural changes. So I've re-written the refactoring.

Pulls the match on sharing_strategy outside of the for loop, and moves any code that is specific to one strategy into the appropriate branch.

[codecov]

Codecov Report

Attention: Patch coverage is 81.81818% with 10 lines in your changes missing coverage. Please review.

Project coverage is 84.14%. Comparing base (2db031c) to head (7ce88fd).
Report is 34 commits behind head on main.

Files	Patch %	Lines
...c/session_manager/group_sessions/share_strategy.rs	81.81%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3884      +/-   ##
==========================================
+ Coverage   84.10%   84.14%   +0.03%     
==========================================
  Files         266      266              
  Lines       27724    27738      +14     
==========================================
+ Hits        23318    23339      +21     
+ Misses       4406     4399       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[richvdh]

This feels a bit odd because now recipient_devices.unsigned_of_verified_user etc are completely unused in the IdentityBasedStrategy case. We should probably use a different return type for split_recipients_withhelds_for_user_based_on_identity if we're going to do this.

---

I'd previously seen another PR from Valere making this change, and at the time dismissed it, because it felt like it was adding duplication (eg, we have to do the should_rotate logic twice) without a clear benefit. I appreciate I'm lacking the context from the later PR, so could you give me a one-liner on why you feel that duplicating that logic is a worthwhile tradeoff?

[richvdh]

consider inlining withheld_recipients

[uhoreg]

Indeed, this was done in #3662 (along with inlining recipients) so I've added that here now.

[uhoreg]

Initially, I was just trying to make #3662 easier to review, so I wasn't looking much into why the refactor was made, but IMHO, it isn't much duplication, and the code looks easier to follow. For example, in this version, it's clearer that unsigned_devices_of_verified_users and verified_users_with_new_identities are only used in the device-based strategy, rather than having to rely on comments to document that.

[uhoreg]

Actually, looking at #3662 again, it basically also adds the verified_users_with_new_identities check (and we should probably combine them). But I still think the refactor is useful because the check within the loop will still need to be duplicated (because the device-based check only runs if error_on_verified_user_problem is true, while the identity-based check runs all the time), and it will make it clear that the verified_users_with_new_identities check applies to both cases, whereas the unsigned_devices_of_verified_users check only applies to the device-based case.

[uhoreg]

And, having mostly re-implemented #3662 on top of this PR, another reason for the refactor is that identity-based collection needs to ensure that the user has cross-signing set up, so we already need to have a check for the collection strategy outside of the for loop, and it may be cleaner to just have the one check in the function, rather than one outside the for loop and one inside.

Actually, looking at #3662 again, it basically also adds the verified_users_with_new_identities check (and we should probably combine them). ...

Which leads to the somewhat awkward result that I need to move some things such as variable definitions back out to their original positions. So I'm somewhat torn as to whether it's better to actually move those things here, with the rationale that the moves make sense for what this PR does, or if it's better to leave those in place, with the rationale that it makes more sense for the end result, even though this PR gets more "trust me, it'll make sense later" thrown into it.

[richvdh]

The change looks good to me, so if you're happy that this is progress in the right general direction, suggest you go ahead and land it.

[uhoreg]

The change looks good to me, so if you're happy that this is progress in the right general direction, suggest you go ahead and land it.

I'm happy with this, but I don't have permissions to merge. Can you hit the button for me?

BTW, the next bit that builds on top of this PR is #3896 I have some compilation issues to fix and a changelog to write, but it should be ready for review soon.