fix(sdk): Can't assert identity without session

[johannescpk]

Fixes #821

Part of #228

[codecov]

Codecov Report

Merging #832 (420ca26) into main (2d06538) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #832   +/-   ##
=======================================
  Coverage   78.20%   78.20%           
=======================================
  Files         101      101           
  Lines       14223    14223           
=======================================
  Hits        11123    11123           
  Misses       3100     3100           

Impacted Files	Coverage Δ
crates/matrix-sdk/src/client/builder.rs	71.57% <ø> (ø)
crates/matrix-sdk/src/http_client.rs	88.75% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d06538...420ca26. Read the comment docs.

[Hywan]

I understand the patch avoids to fall in the else branch with a session set to None, which will avoid to return an Err(HttpError::UserIdRequired) (from session). The new path could build a request without a user ID and with SendAccessToken::None instead.

All of this sounds sensible to me. I don't know if it's correct though, and it could be comfortable to get a second review to validate this new flow.

cc @poljar @gnunicorn

[gnunicorn]

This is an appservice only feature, and I don't fully understand when and how the assert_identity is used by the service, but I assume you have better knowledge about that and know about the potential problems here. That said, it'd be great if 1) you could update the code docs of ClientBuilder.assert_identity, as they say it would fail without a session (which it wouldn't anymore now) and 2) in a PR like this you'd give a bit more context, like stating that this is an appservice only feature and educate us more, why our current code is wrong :) (the issue linked also only describes the bug, not really why that behavior is considered a bug).

[johannescpk]

Thanks for the reviews and docs catch, updated accordingly.

Right, I somehow was thinking about identity assertion as independent feature - but forgot that it actually is tied to appservices only, in terms of spec.

It's considered a bug because using the appservice crate currently will error at runtime if you try to use it, regardless of what you try to do with it. That's because server_versions passes None as session - which is called as part of HttpClient::send - since a refactoring, and the else block always expects a session to be available. We unfortunately have no test that covers identity assertion proper for this scenario yet, which would've have surfaced it sooner.

The given PR was the only fix I found without large refactorings, because a lot depends on Store::session now returning Option<&Session> instead of Arc<OnceCell<Session>>.

[johannescpk]

In terms of correctness: It doesn't seem to make sense anyway to do identity assertion for endpoints that don't require authentication, since endpoints that don't require authentication don't have any knowledge about the user or session, and so wouldn't act differently either way. So maybe a refactoring including a test could take that into account.

[gnunicorn]

thanks for the clarifications!