feat: client version information - #8557
Conversation
d9ac2fb to
12d8f41
Compare
12d8f41 to
698d83b
Compare
Hocuri
left a comment
There was a problem hiding this comment.
Just some quick comments from quickly reading over it. Would like to finish #8544, afterwards I can take this over if still needed
when is IMAP METADATA actually read?
After starting io, before starting to fetch messages.
when are they ready?
Right now, core doesn't tell the UI that they are ready because the UI never needed any metadata. So, we will need a new event
This one is going to contain deltachat APK URLs, so it's fine if it is called deltachat. |
698d83b to
a636f3a
Compare
6beb655 to
f9b5873
Compare
idea is to have a |
|
i had a call with @Hocuri and incorporated all things we discussed there. if there are no more comments wrt json format and IMAP METADATA name, i will file an issue for the relays soon. otherwise, this PR can be re-reviewed, so it gets used by android then soon :) |
hpk42
left a comment
There was a problem hiding this comment.
overall looks good but the parse-error issue is a blocker i think, and freshness deserves refinement.
| /// and [`/shared/admin`](https://www.rfc-editor.org/rfc/rfc5464#section-6.2.2) | ||
| /// metadata. | ||
| #[expect(clippy::arithmetic_side_effects)] | ||
| pub(crate) async fn update_metadata(&mut self, context: &Context) -> Result<()> { |
There was a problem hiding this comment.
The full GETMETADATA runs only once per process and transport (but not per reconnection), afterwards only the TURN path refreshes. So a UI polling daily gets the same cached data and freshness comes from process restarts. Often on mobiles but rarer on desktop: IISIC even un-suspending a laptop would not refresh the metadata value. Maybe appversions could also be fetched in the TURN refresh way (which debounces re-fetching)? Maybe using the same timer even. Then UIs could poll however often they want, and core controls how often an actual re-fetch is done.
There was a problem hiding this comment.
makes sense, currently afk, will have a look at that later
Co-authored-by: holger krekel <holger@merlinux.eu>
Co-authored-by: holger krekel <holger@merlinux.eu>
Co-authored-by: holger krekel <holger@merlinux.eu>
Co-authored-by: holger krekel <holger@merlinux.eu>
| /// Security note: consumers need to verify themselves | ||
| /// that downloaded app files are valid before installing them. |
There was a problem hiding this comment.
Who exactly is responsible for verifying that the downloaded app files are valid? The UIs? I can't see any such logic in deltachat/deltachat-android#4582; right now, the relay could just point to some wrong download URL. They can't update the existing app on Android because the system checks the signature, but they can install an unrelated app (I don't think that many users would notice that they're asked to "install" an app rather than to "update" it).
So, I think we need either of these:
- A whitelist of URL prefixes here in core (containing
https://github.com/deltachat/deltachat-android/releases/downloadand URLs for the other UIs) - A whitelist of URL prefixes in the UIs
- Checking that the downloaded actually is an update, rather than some other app, on Android, decide later what to do about other UIs
If the UIs are responsible, then get_app_version() in api.rs also needs a big security warning. Or maybe the required URL prefix could even be passed to get_app_version(), so that the UI can't forget it?
There was a problem hiding this comment.
(I didn't actually notice this problem until hpk commented about it)
this PR is about reading update information via IMAP metadata from the relay.
it is up to the UI to call
get_app_version()at a reasonable time and frequency, see comment in the code. when called,get_app_version()iterates over all known profiles and relays and checks for version information, returning the newest for the given scope.we do not use an event, as that is tricky wrt changes - we do not know if other relays report later a newer version. we also do not cache anything, to prevent bad relays avoiding us to update permanently. also it is easier :)
outdated notes and questions
it is up to the clients to get the needed information, we could let core filter, but it seems easy enough the other way round, and may have debug advantages, one can iterate etc.EDIT: we now filter in core, this also makes the jsonrpc part easier, see review commentswhen is IMAP METADATA actually read? when are they ready? is that really the correct place? i am up to change that, but beware, this is not really my expertise, so someone else may need to take over :)
EDIT: see below, IMAP METADATA is read on connection, before fetching starts, usually fast enough
relay part is missing. once the format is settled and discussed shortly here, that should be done soon as well. but this is definitely not my expertise and needed to be done by someone else :)
key for IMAP METADATA is
/shared/vendor/deltachat/appversions- shall we continue usedeltachatfor compatibility or so?chatmailseems to be more correctEDIT: we stay with the current
relay counterpart issue: chatmail/relay#1037
cc @link2xt @Hocuri @hpk42