WIP - Add installer downloader and in-app upgrades backend #7583
Conversation
dlon
changed the title
dlon
force-pushed
the
installer-downloader
branch
from
8bdac28 to
bf3eeb4
Compare
mullvad-update/src/app.rs
Outdated
| } | ||
|
|
||
| impl<SigProgress, AppProgress> HttpAppDownloader<SigProgress, AppProgress> { | ||
| const MAX_SIGNATURE_SIZE: usize = 1 * 1024; |
| println!("cargo:rerun-if-changed=loader.manifest"); | ||
| //res.set_manifest_file("loader.manifest"); |
There was a problem hiding this comment.
Is there some manual process here where you need to update the loader.manifest file if it changes. Could that be documented?
There was a problem hiding this comment.
No, I've just commented it out for testing.
There was a problem hiding this comment.
To clarify: I'm not sure whether we should elevate as the loader starts or just before installing. In the former case, we need this, since it triggers an elevation prompt.
mullvad-update/src/app.rs
Outdated
| signature_url: String, | ||
| app_url: String, | ||
| app_size: usize, | ||
| signature_progress_updater: SigProgress, | ||
| app_progress_updater: AppProgress, |
There was a problem hiding this comment.
Nit: Why flatten the parameters here? You could just have parameters: AppDownloaderParameters<SigProgress, AppProgress>.
| #[derive(Debug)] | ||
| pub enum DownloadError { | ||
| FetchSignature(anyhow::Error), | ||
| FetchApp(anyhow::Error), | ||
| Verification(anyhow::Error), | ||
| } |
There was a problem hiding this comment.
These error variants are never matched on, and the enum doesn't implement the Error trait, so is there really any point to it? Why not just use anyhow?
mullvad-update/src/api.rs
Outdated
| #[derive(Debug, Clone)] | ||
| pub struct Version { | ||
| /// Version | ||
| pub version: String, | ||
| /// URLs to use for downloading the app installer | ||
| pub urls: Vec<String>, | ||
| /// Size of installer, in bytes | ||
| pub size: usize, | ||
| /// URLs pointing to app PGP signatures | ||
| pub signature_urls: Vec<String>, | ||
| } | ||
|
|
There was a problem hiding this comment.
This differs a bit from the specification if the REST format, which I assume is intentional since it isn't active yet and the format is not final.
Just to help me understand better, is there not supposed to be any information in this struct about e.g. architectures, changelogs and signature types, or is that something left for the future?
There was a problem hiding this comment.
My intention was to keep this public type separate from the data that's deserialized from JSON. The output of this module is "version information that's guaranteed to have been signed by Mullvad, isn't expired, etc., for the current architeture", with all those details hidden.
But yes, it's also not complete.
There was a problem hiding this comment.
I get this warning when running cargo check
warning: profiles for the non root package will be ignored, specify profiles at the workspace root:
package: C:\Users\Sebastian\Documents\Programming\mullvad\installer-downloader\Cargo.toml
workspace: C:\Users\Sebastian\Documents\Programming\mullvad\Cargo.toml
Is the opt level specified in installer-downloader being overwritten by the workspace level?
It isn't being correctly applied. This is noted in the toml file. I think it'll be replaced by a build script. |
| add_file_server_mock(&mut server, "/my_file", file_data); | ||
|
|
||
| // Interrupt after exactly half the file has been downloaded | ||
| let mut limited_buffer = vec![0u8; file_data.len() / 2].into_boxed_slice(); |
There was a problem hiding this comment.
.into_boxed_slice() and the following into_vec() aren't needed. The test compiles and passes when I remove them.
There was a problem hiding this comment.
Does it work without removing vec!? I was sure that it would automatically resize the buffer in that case. I'll try it, but somehow I trust it more knowing it's a fixed size.
| /// Create endpoints that serve a file at `url_path` using range requests | ||
| fn add_file_server_mock(server: &mut mockito::Server, url_path: &str, data: &'static [u8]) { | ||
| // Respond to head requests with file size | ||
| server | ||
| .mock("HEAD", url_path) | ||
| .with_header(CONTENT_LENGTH, &data.len().to_string()) | ||
| .create(); | ||
|
|
||
| // Respond to range requests with file | ||
| server | ||
| .mock("GET", url_path) | ||
| .with_body_from_request(|request| { | ||
| let range = request.header(RANGE); | ||
| let range = range[0].to_str().expect("expected str"); | ||
| let (begin, end) = parse_http_range(range).expect("invalid range"); | ||
|
|
||
| data[begin..=end].to_vec() | ||
| }) | ||
| .create(); | ||
| } |
There was a problem hiding this comment.
Cool mocking crate! Seems like something we could use more
| if active_download.is_some() { | ||
| continue; | ||
| } | ||
| let Some(version_info) = version_info.clone() else { | ||
| continue; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Perhaps we should log some warning here?
I think it should because it makes it easier to regenerate them if the SVGs are edited. |
dlon
force-pushed
the
installer-downloader
branch
from
ec26088 to
15fb2cc
Compare
mullvad-update/src/deserializer.rs
Outdated
| pub signed: Response, | ||
| } | ||
|
|
||
| /// Helper class that leaves the signed data untouched |
There was a problem hiding this comment.
:man-gesturing-no:
| /// Helper class that leaves the signed data untouched | |
| /// Helper type that leaves the signed data untouched |
| /// Beta preface text | ||
| pub const BETA_PREFACE_DESC: &str = "Want to try the new Beta version? "; | ||
| /// Beta link text | ||
| pub const BETA_LINK_TEXT: &str = "Click here!"; |
There was a problem hiding this comment.
Is the text really up to date with the design? Shouldn't it say "Want to try out new features? Try our new beta!"
| impl AppWindow { | ||
| /// Set up UI elements, position them, and register window message handlers | ||
| /// Note that some additional setup happens in [Self::on_init] | ||
| pub fn layout(mut self) -> Result<Rc<RefCell<AppWindow>>, nwg::NwgError> { |
There was a problem hiding this comment.
The text is much larger than in the design for me
Small refactoring that replaces a custom trait with a `From` implementation.
Co-authored-by: Sebastian Holmin <[email protected]>
This also replaces the PGP verifier with a SHA256 checksum verifier
`format` was also updated to support signing
dlon
force-pushed
the
installer-downloader
branch
from
11a15f8 to
180ea7d
Compare
This brings down the binary size of installer-downloader from 2.3 M to 1.4 M with size optimizations enabled
This PR aims to introduce a minimal installer as well as most of the functionality for in-app upgrades (except UI). This is still a work in progress.
The main things that are missing:
The planned native module for the Electron frontend is not in scope for this PR.
Close DES-1685
Close DES-1684
Close DES-1641
Close DES-1640
Close DES-1571
This change is