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

App update notification #1608

Open
wants to merge 30 commits into
base: dev
from

Conversation

Projects
None yet
3 participants
@krtkush
Contributor

krtkush commented Aug 16, 2018

Added feature described in #1520 and #1531.

Currently it is a very basic version. Will further build on this feature in the future.

@TobiGr

Hey @krtkush!
Thanks for working on this.
Unfortunately, I think there are some details, which we need to discuss first. Apart from that, I've pointed some things out in the code which should be improved. But I recommend you to wait with fixes until we have finished our discussion.

My biggest concern about these changes are the additional build flavors. When someone installs NewPipe from F-Droid, he/she will always be forced to use this source to update the application.
Additionally, with this new config every user needs to uninstall the current version of the app.
Not only for this reason, but also as update notifications can be disturbing from time to time, I suggest to add a setting which handles the update options:

  • Enable / Disable search for new updates
  • Preferred download mirror (applies when F-Droid and GitHub versions are the same, see below).

To identify whether the app is from F-Droid or GitHub we need to check the app's signature. A guide how this can be achieved can be found here.

IMHO we should come back to one release apk using F-Droid's reproducible builds feature.

@TobiGr TobiGr referenced this pull request Aug 16, 2018

Open

Auto upgrade Newpipe #1531

@theScrabi

This comment has been minimized.

Show comment
Hide comment
@theScrabi

theScrabi Aug 17, 2018

Member

My biggest concern about these changes are the additional build flavors. When someone installs NewPipe from F-Droid, he/she will always be forced to use this source to update the application.

Due to different signatures you already have to reisntall the app if you want to use the fdroid version.

Additionally, with this new config every user needs to uninstall the current version of the app.

How comes? As long as the package name and signing key does not change nothing needs to be re installed.

Enable / Disable search for new updates

I agree. Although it should be enabled by default.

Member

theScrabi commented Aug 17, 2018

My biggest concern about these changes are the additional build flavors. When someone installs NewPipe from F-Droid, he/she will always be forced to use this source to update the application.

Due to different signatures you already have to reisntall the app if you want to use the fdroid version.

Additionally, with this new config every user needs to uninstall the current version of the app.

How comes? As long as the package name and signing key does not change nothing needs to be re installed.

Enable / Disable search for new updates

I agree. Although it should be enabled by default.

@TobiGr

This comment has been minimized.

Show comment
Hide comment
@TobiGr

TobiGr Aug 17, 2018

Member

Due to different signatures you already have to reisntall the app if you want to use the fdroid version.

That's true, but I'd like to get back to one version.

How comes? As long as the package name and signing key does not change nothing needs to be re installed.

flavorDimensions "apkSource"
    productFlavors {
        github {
            dimension "apkSource"
            applicationIdSuffix ".github"

        }

        fdroid {
            dimension "apkSource"
            applicationIdSuffix ".fdroid"
        }
}

This means, we now have four different builds:
githubRelease
githubDebug
fdroidRelease
fdroidDebug

None of them matches our release build because these new have suffixes. This means an upgrade is not possible.

Member

TobiGr commented Aug 17, 2018

Due to different signatures you already have to reisntall the app if you want to use the fdroid version.

That's true, but I'd like to get back to one version.

How comes? As long as the package name and signing key does not change nothing needs to be re installed.

flavorDimensions "apkSource"
    productFlavors {
        github {
            dimension "apkSource"
            applicationIdSuffix ".github"

        }

        fdroid {
            dimension "apkSource"
            applicationIdSuffix ".fdroid"
        }
}

This means, we now have four different builds:
githubRelease
githubDebug
fdroidRelease
fdroidDebug

None of them matches our release build because these new have suffixes. This means an upgrade is not possible.

@theScrabi

This comment has been minimized.

Show comment
Hide comment
@theScrabi

theScrabi Aug 17, 2018

Member

But do we need the suffix?

I am with you though, I think trying to make the reproducible build work might not be a bad idea.

Member

theScrabi commented Aug 17, 2018

But do we need the suffix?

I am with you though, I think trying to make the reproducible build work might not be a bad idea.

@TobiGr

This comment has been minimized.

Show comment
Hide comment
@TobiGr

TobiGr Aug 17, 2018

Member

The current implementation uses the new build flavors to decide whether to check for an update. As mentioned above, there are other ways to do this (e.g. the signature). So if this is implemented in a different way, we won't need a suffix.

Member

TobiGr commented Aug 17, 2018

The current implementation uses the new build flavors to decide whether to check for an update. As mentioned above, there are other ways to do this (e.g. the signature). So if this is implemented in a different way, we won't need a suffix.

@krtkush

This comment has been minimized.

Show comment
Hide comment
@krtkush

krtkush Aug 20, 2018

Contributor

Hey @TobiGr, @theScrabi

I agree - A single version will be a better approach; I'll look into replacing the flavor with signature check.

Contributor

krtkush commented Aug 20, 2018

Hey @TobiGr, @theScrabi

I agree - A single version will be a better approach; I'll look into replacing the flavor with signature check.

@theScrabi

This comment has been minimized.

Show comment
Hide comment
@theScrabi

theScrabi Sep 11, 2018

Member

@krtkush @TobiGr so here are some updates. After the youtube amageddon that made us release v0.14.1 a few days ago, I had a conversation with the fdroid people about update speed.

It seems to be a complicated topic which seem to not get a widespread acceptance within their team. Therefore we should try and even deliver updates ourself even if its a version deploid by fdroid.

This on one hand makes things easier for us since we don't have to deploy two separate versions, and on the other hand we need to make NewPipe a reproducable build within fdroid.

@krtkush could you please try to fix the conflicts, and @TobiGr would you please review it after the changes are done.

I am willing to put this feature into the next version if it's possible :)

Member

theScrabi commented Sep 11, 2018

@krtkush @TobiGr so here are some updates. After the youtube amageddon that made us release v0.14.1 a few days ago, I had a conversation with the fdroid people about update speed.

It seems to be a complicated topic which seem to not get a widespread acceptance within their team. Therefore we should try and even deliver updates ourself even if its a version deploid by fdroid.

This on one hand makes things easier for us since we don't have to deploy two separate versions, and on the other hand we need to make NewPipe a reproducable build within fdroid.

@krtkush could you please try to fix the conflicts, and @TobiGr would you please review it after the changes are done.

I am willing to put this feature into the next version if it's possible :)

@krtkush

This comment has been minimized.

Show comment
Hide comment
@krtkush

krtkush Sep 11, 2018

Contributor

@theScrabi
Sure! I'll work on it this weekend.

Just to be clear, we'll be going with the signature check method as suggested by @TobiGr, right? And not with different builds.

Contributor

krtkush commented Sep 11, 2018

@theScrabi
Sure! I'll work on it this weekend.

Just to be clear, we'll be going with the signature check method as suggested by @TobiGr, right? And not with different builds.

@theScrabi

This comment has been minimized.

Show comment
Hide comment
@theScrabi

theScrabi Sep 11, 2018

Member

Yes the signature check method.

Member

theScrabi commented Sep 11, 2018

Yes the signature check method.

@krtkush

This comment has been minimized.

Show comment
Hide comment
@krtkush

krtkush Sep 15, 2018

Contributor

@TobiGr @theScrabi Does NewPipe use different KeyStores for the github and f-droid versions respectively?

I'll need the developer certificate signature of the KeyStore(s), as instructed in the linked article. Could either of you provide that?

Contributor

krtkush commented Sep 15, 2018

@TobiGr @theScrabi Does NewPipe use different KeyStores for the github and f-droid versions respectively?

I'll need the developer certificate signature of the KeyStore(s), as instructed in the linked article. Could either of you provide that?

@TobiGr

This comment has been minimized.

Show comment
Hide comment
@TobiGr

TobiGr Oct 4, 2018

Member

@krtkush Sorry for the silence. We are all quite busy. If I am correct, you can find the fingerprint on @theScrabi's website: https://schabi.org If you need any further information about the signing key, please ask him :)

Member

TobiGr commented Oct 4, 2018

@krtkush Sorry for the silence. We are all quite busy. If I am correct, you can find the fingerprint on @theScrabi's website: https://schabi.org If you need any further information about the signing key, please ask him :)

@krtkush

This comment has been minimized.

Show comment
Hide comment
@krtkush

krtkush Oct 6, 2018

Contributor

Thanks @TobiGr!

@theScrabi Can you confirm both the keys? On the website I only see one - SHA1: B0:2E:90:7C:1C:D6:FC:57:C3:35:F0:88:D0:8F:50:5F:94:E4:D2:15

Contributor

krtkush commented Oct 6, 2018

Thanks @TobiGr!

@theScrabi Can you confirm both the keys? On the website I only see one - SHA1: B0:2E:90:7C:1C:D6:FC:57:C3:35:F0:88:D0:8F:50:5F:94:E4:D2:15

@theScrabi

This comment has been minimized.

Show comment
Hide comment
@theScrabi

theScrabi Oct 6, 2018

Member

The second key is the one of the fdroid version. You can see the cert by downloading the apk, and extracting it. They key is:
SHA1: 83:10:87:55:C1:3C:C6:D5:5D:46:86:53:C8:2F:CA:9E:25:46:8A:C9

The issue is, I don't know where you can find the fingerprint for the verification. The only thing I can tell you is, that fdroid is signing the apk with their own key. Therefore you might find this fingerprint on their website.

Member

theScrabi commented Oct 6, 2018

The second key is the one of the fdroid version. You can see the cert by downloading the apk, and extracting it. They key is:
SHA1: 83:10:87:55:C1:3C:C6:D5:5D:46:86:53:C8:2F:CA:9E:25:46:8A:C9

The issue is, I don't know where you can find the fingerprint for the verification. The only thing I can tell you is, that fdroid is signing the apk with their own key. Therefore you might find this fingerprint on their website.

@krtkush

This comment has been minimized.

Show comment
Hide comment
@krtkush

krtkush Oct 13, 2018

Contributor

You're right. I'm having trouble finding the fingerprint.

However, this approach should work. The accepted answer rebuilds the SHA1 key at runtime. Hence, we can confirm that if the retrieved SHA1 key is equal to B0:2E:90:7C:1C:D6:FC:57:C3:35:F0:88:D0:8F:50:5F:94:E4:D2:15 then it is a GitHub build and therefore we can enable the version update check.

Contributor

krtkush commented Oct 13, 2018

You're right. I'm having trouble finding the fingerprint.

However, this approach should work. The accepted answer rebuilds the SHA1 key at runtime. Hence, we can confirm that if the retrieved SHA1 key is equal to B0:2E:90:7C:1C:D6:FC:57:C3:35:F0:88:D0:8F:50:5F:94:E4:D2:15 then it is a GitHub build and therefore we can enable the version update check.

@theScrabi

This comment has been minimized.

Show comment
Hide comment
@theScrabi

theScrabi Oct 16, 2018

Member

:D nice solution.

Member

theScrabi commented Oct 16, 2018

:D nice solution.

@krtkush

This comment has been minimized.

Show comment
Hide comment
@krtkush

krtkush Oct 16, 2018

Contributor

Thanks!

All done from my side.

Contributor

krtkush commented Oct 16, 2018

Thanks!

All done from my side.

krtkush added some commits Oct 18, 2018

@krtkush

This comment has been minimized.

Show comment
Hide comment
@krtkush

krtkush Oct 18, 2018

Contributor

I made a couple of more changes. Everything seems to be working and I think the feature is ready for a production release; we can add more functionality to it later.

Contributor

krtkush commented Oct 18, 2018

I made a couple of more changes. Everything seems to be working and I think the feature is ready for a production release; we can add more functionality to it later.

@theScrabi

This comment has been minimized.

Show comment
Hide comment
@theScrabi

theScrabi Oct 18, 2018

Member

:D nice i wall qu it soon

Member

theScrabi commented Oct 18, 2018

:D nice i wall qu it soon

@TobiGr

I just skipped the code. A more detailed review will follow within the next week.

Show outdated Hide outdated app/src/main/java/org/schabi/newpipe/CheckForNewAppVersionTask.java Outdated
MessageDigest md = MessageDigest.getInstance("SHA1");
byte[] publicKey = md.digest(c.getEncoded());
hexString = byte2HexFormatted(publicKey);
} catch (NoSuchAlgorithmException e1) {

This comment has been minimized.

@TobiGr

TobiGr Oct 22, 2018

Member

Aah..

That's just a habit of mine. I'll change it to the suggested one.

I see 😆

@TobiGr

TobiGr Oct 22, 2018

Member

Aah..

That's just a habit of mine. I'll change it to the suggested one.

I see 😆

Show resolved Hide resolved app/src/main/res/values/strings.xml Outdated
@TobiGr

This comment has been minimized.

Show comment
Hide comment
@TobiGr

TobiGr Oct 22, 2018

Member

:D nice i wall qu it soon

Or even better @theScrabi can do this. I didn't see your comment. GitHub had some problems today 😄

Member

TobiGr commented Oct 22, 2018

:D nice i wall qu it soon

Or even better @theScrabi can do this. I didn't see your comment. GitHub had some problems today 😄

@theScrabi

This comment has been minimized.

Show comment
Hide comment
@theScrabi

theScrabi Oct 22, 2018

Member

I don't find time recently, but I'll try my best. After next release this should be highest prio.

Member

theScrabi commented Oct 22, 2018

I don't find time recently, but I'll try my best. After next release this should be highest prio.

TobiGr and others added some commits Oct 22, 2018

Code review changes.
Co-Authored-By: krtkush <kartikey92@gmail.com>
@krtkush

This comment has been minimized.

Show comment
Hide comment
@krtkush

krtkush Oct 22, 2018

Contributor

Yeah GitHub was down today for a fair bit of time.

Contributor

krtkush commented Oct 22, 2018

Yeah GitHub was down today for a fair bit of time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment