Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NAM: Implement direct media requests fallback #576

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

KitsuneRal
Copy link
Member

(From the commit message:)
This is behind a configuration switch because direct unauthenticated requests are at odds with the Matrix model where clients stick to their homeservers and only homeservers talk to each other. The implementation is also quite inefficient atm, causing a .well-known resolution roundtrip every single time a direct media request is made.

@KitsuneRal KitsuneRal added the enhancement A feature or change request for the library label Oct 10, 2022
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

64.9% 64.9% Coverage
0.0% 0.0% Duplication

@KitsuneRal
Copy link
Member Author

Upon some more testing - this really needs a cache/registry of homeservers, otherwise each direct request becomes a sequence of three (not two, as would be optimal): .well-known resolution, requesting login flows (there's no way in Connection to perform a homeserver resolution without getting login flows for now), and the request for media itself - and besides, creation and destruction of a Connection object for that. In fact, it would be great to disassociate resolving a homeserver from Connection since the latter is intended to be a long-lived user session but has to be created early in the login process just for the sake of keeping the information about the target homeserver. ConnectionData might be a good candidate for repurposing in that direction. That change is rather significant for the API though so probably better left for 0.8.

@KitsuneRal KitsuneRal force-pushed the kitsune/direct-media-requests branch from 74d9446 to 2d10238 Compare December 20, 2022 13:43
This is behind a configuration switch because direct unauthenticated
requests are at odds with the Matrix model where clients stick to their
homeservers and only homeservers talk to each other. The implementation
is also quite inefficient atm, causing a .well-known resolution
roundtrip every single time a direct media request is made.
@KitsuneRal KitsuneRal force-pushed the kitsune/direct-media-requests branch from 2d10238 to 0eafc1d Compare May 28, 2023 05:43
Also: simplify the expression in FINISH_TEST.
Going through QSettings doesn't work quite well on Windows; setting it
in Quotest and immediately testing through another QSettings instance
inside NetworkAccessManager (in another thread) returns the previous
value. In fact, there's no general guarantee for that, as almost all
QSettings member functions are reentrant but not thread-safe. The flag
is only initialised from QSettings once at the first NAM creation, and its
changes are only propagated to QSettings but never read again.
Implication: changes in QSettings files when the client is running will
only be reflected at the next run (or may be overwritten altogether),
so just don't change settings files while the client is running, ok?
@KitsuneRal KitsuneRal force-pushed the kitsune/direct-media-requests branch from 0eafc1d to e337379 Compare May 28, 2023 08:13
std::atomic_flag(bool) is not a standard constructor, and MSVC doesn't
have it, for one.
@KitsuneRal KitsuneRal requested a review from TobiasFella May 28, 2023 12:35
Copy link
Member

@TobiasFella TobiasFella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable, i guess

@KitsuneRal KitsuneRal self-assigned this May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature or change request for the library
Projects
Status: 0.10(?) - To Do
Development

Successfully merging this pull request may close these issues.

2 participants