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

Client cert #12408

Merged
merged 10 commits into from
Jan 25, 2024
Merged

Client cert #12408

merged 10 commits into from
Jan 25, 2024

Conversation

tobiasKaminsky
Copy link
Member

@tobiasKaminsky tobiasKaminsky commented Jan 24, 2024

Replaces #11314

  • Tests written, or not not needed

Elv1zz and others added 7 commits January 24, 2024 08:28
Using the new `AdvancedX509KeyManager` class from the `nextcloud-android-library` to add support for servers that require a TLS client certificate to connect.

Signed-off-by: Elv1zz <elv1zz.git@gmail.com>
Since the official nextcloud android-library does not have the new `AdvancedX509KeyManager`, the automated tests cannot build th
e app. So for that I refer to my fork of the android-library for now. This commit shall be reverted before merge.

Signed-off-by: Elv1zz <elv1zz.git@gmail.com>
All interaction with the nextcloud server is handled by the `NextCloudWebViewClient`, so TLS client certificate handling should be done by that class. Since `AuthenticatorActivity` only extends `NextCloudWebViewClient` with some additional methods, it is enough to have the certificate handling in one place.

Signed-off-by: Elv1zz <elv1zz.git@gmail.com>
Isntead of having to find the hostname and port from an URL (which might be more tricky than expected), we now can simply pass down the URL and `AdvancedX509KeyManager` will take care of finding the port from the URL.

Signed-off-by: Elv1zz <elv1zz.git@gmail.com>
Improving code quality a bit by avoiding magic numbers.

Signed-off-by: Elv1zz <elv1zz.git@gmail.com>
The method `onReceivedHttpError` did have 3 exit points (`return`), but Codacy only allows us 2, so error handling for `request?.url` and `view?.context` was combined. Seems debatable, what's more readable, but the rules are the rules.

Signed-off-by: Elv1zz <elv1zz.git@gmail.com>
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
@tobiasKaminsky
Copy link
Member Author

@Elv1zz I have opened it here again, to have working CI and rebased it.
Many thanks for your great PR.

Can you test this once again?
I will also test it soon.

After that I will merge it.

@tobiasKaminsky
Copy link
Member Author

/rebase

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
Signed-off-by: github-actions <github-actions@github.com>
Copy link

Codacy

Lint

TypemasterPR
Warnings6969
Errors33

SpotBugs

CategoryBaseNew
Bad practice2626
Correctness7373
Dodgy code360360
Experimental22
Internationalization77
Malicious code vulnerability22
Multithreaded correctness66
Performance5454
Security1818
Total548548

Copy link

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/12408.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

Copy link

blue-Light-Screenshot test failed, but no output was generated. Maybe a preliminary stage failed.

@tobiasKaminsky tobiasKaminsky merged commit aa780ca into master Jan 25, 2024
3 of 4 checks passed
@delete-merged-branch delete-merged-branch bot deleted the clientCert branch January 25, 2024 13:10
@Elv1zz
Copy link
Contributor

Elv1zz commented Jan 25, 2024

@Elv1zz I have opened it here again, to have working CI and rebased it. Many thanks for your great PR.

:)

Can you test this once again? I will also test it soon.

I tested it for about 24 hours now and everything seems to work fine.

There is one thing, which does not seem to work completely, but it might be a new thing still under development in the app itself: There is a new notification showing the "upload preparation" progress, which is never going beyond about 20%. Either this is not implemented completely, yet, or it does not fully work with client certificates. But in general photo upload in the background still works.

After that I will merge it.

Yay, already merged 🎉 Looking forward to see this feature in the next release.

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

Successfully merging this pull request may close these issues.

3 participants