Skip to content

Fix the cert4android user decision dialog not being shown#694

Merged
sunkup merged 5 commits into
devfrom
692-broken-certificate-handling
Sep 10, 2025
Merged

Fix the cert4android user decision dialog not being shown#694
sunkup merged 5 commits into
devfrom
692-broken-certificate-handling

Conversation

@sunkup

@sunkup sunkup commented Sep 8, 2025

Copy link
Copy Markdown
Member

Purpose

Fix the cert4android user decision dialog not being shown when needed.

Short description

Instead of passing a client engine directly to AppHttpClient we now pass a createEngine function which allows us to configure the engine at the same time. By default we create the minimally configured OkHttp engine and it gets replaced in the mock server which passes a function which creates a mock engine.

I tested (please double check), that:

  • user agent is still showing
  • redirects are handled by CalendarFetcher
  • the timeout increase is applied

Note also that accepting a certificate from within the cert4android user decision dialog does not yet work. Probably due to the Nav3 rewrite. That is tracked in #697

Checklist

  • The PR has a proper title, description and label.
  • I have self-reviewed the PR.
  • I have added documentation to complex functions and functions that can be used by other modules.
  • I have added reasonable tests or consciously decided to not add tests.

@sunkup sunkup self-assigned this Sep 8, 2025
@sunkup sunkup added the bug label Sep 8, 2025
@sunkup sunkup linked an issue Sep 8, 2025 that may be closed by this pull request
@sunkup
sunkup force-pushed the 692-broken-certificate-handling branch from d22691d to 7bfaec4 Compare September 9, 2025 11:56
@sunkup
sunkup requested a review from Copilot September 9, 2025 12:04

This comment was marked as outdated.

@sunkup
sunkup requested a review from Copilot September 9, 2025 12:20
@sunkup sunkup changed the title Fix broken certificate handling Fix the cert4android user decision dialog not being shown Sep 9, 2025

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes certificate handling by changing the AppHttpClient architecture to properly initialize the certificate manager when the HTTP client engine is created.

  • Replaced direct HttpClientEngine parameter with a factory function that creates engines with proper certificate configuration
  • Updated MockServer to work with the new factory pattern while maintaining test isolation
  • Simplified the engine configuration by moving SSL setup to the factory function

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
AppHttpClient.kt Refactored to use engine factory function instead of direct engine, enabling proper certificate manager integration
ValidatorTest.kt Updated test to use new factory function signature for mock HTTP client creation
MockServer.kt Modified to provide engine factory function instead of direct engine instance for testing

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread app/src/main/java/at/bitfire/icsdroid/AppHttpClient.kt
Comment thread app/src/main/java/at/bitfire/icsdroid/AppHttpClient.kt
@sunkup
sunkup marked this pull request as ready for review September 9, 2025 12:29
@sunkup
sunkup requested a review from ArnyminerZ September 9, 2025 12:29

@ArnyminerZ ArnyminerZ left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, feel free to merge.

@sunkup
sunkup merged commit 4a9fb4b into dev Sep 10, 2025
8 checks passed
@sunkup
sunkup deleted the 692-broken-certificate-handling branch September 10, 2025 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cert4android user decision dialog does not show

3 participants