Skip to content

Add CzertainlyAuthenticationCache#1435

Merged
ivosh merged 17 commits into
mainfrom
Authn-Cache
May 7, 2026
Merged

Add CzertainlyAuthenticationCache#1435
ivosh merged 17 commits into
mainfrom
Authn-Cache

Conversation

@LukasNajman
Copy link
Copy Markdown
Collaborator

Add CzertainlyAuthenticationCache with caching for authentication methods, eviction mechanisms, and tests

Copilot AI review requested due to automatic review settings April 29, 2026 11:48
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Apr 29, 2026

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Comment thread src/main/java/com/czertainly/core/security/authn/client/AuthenticationCache.java Dismissed
Comment thread src/main/java/com/czertainly/core/security/authn/client/AuthenticationCache.java Dismissed
Copy link
Copy Markdown

Copilot AI left a comment

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 introduces an authentication caching layer (CzertainlyAuthenticationCache) backed by Spring Cache + Caffeine, adds cache-aware authentication entrypoints on CzertainlyAuthenticationClient, and wires eviction into user/role/certificate mutations with accompanying tests.

Changes:

  • Add AuthenticationCache abstraction + CzertainlyAuthenticationCache implementation and Spring CacheConfig (Caffeine) to cache auth results for system users, user UUID, certificate thumbprints, and token jti.
  • Update authentication flow to compute certificate thumbprints in CzertainlyAuthenticationFilter and route to cache-enabled client methods; update OAuth2/JWT paths similarly.
  • Add/extend tests to cover caching behavior and eviction behavior across user/role/certificate operations.

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
src/test/java/com/czertainly/core/service/UserManagementServiceTest.java Replaces inline session table setup with SessionTableHelper call.
src/test/java/com/czertainly/core/service/UserManagementServiceCacheEvictionTest.java New tests verifying per-user cache eviction behavior on user mutations.
src/test/java/com/czertainly/core/service/RoleManagementServiceTest.java New tests verifying role mutations evict the entire auth cache.
src/test/java/com/czertainly/core/service/CertificateServiceTest.java Adds tests for auth-cache eviction behavior on certificate operations.
src/test/java/com/czertainly/core/security/authn/client/CzertainlyAuthenticationClientTest.java Extends client tests to validate cache hit/miss behavior.
src/test/java/com/czertainly/core/security/authn/client/CzertainlyAuthenticationCacheTest.java New unit tests for cache semantics and eviction behavior.
src/test/java/com/czertainly/core/security/authn/CzertainlyAuthenticationFilterTest.java New tests for cert-thumbprint routing and filter behavior.
src/test/java/com/czertainly/core/auth/oauth2/CzertainlyJwtAuthenticationConverterTest.java New tests for JWT claim extraction + token auth routing.
src/main/java/com/czertainly/core/util/AuthHelper.java Switches system/user proxy authentication to cache-enabled client methods.
src/main/java/com/czertainly/core/service/impl/UserManagementServiceImpl.java Adds targeted per-user cache eviction on user updates/role changes/disable/delete.
src/main/java/com/czertainly/core/service/impl/RoleManagementServiceImpl.java Evicts entire auth cache on role/permission mutations.
src/main/java/com/czertainly/core/service/impl/CertificateServiceImpl.java Adds cert/user auth-cache eviction on revoke and user↔cert association changes; adds lookup by user UUID.
src/main/java/com/czertainly/core/service/CertificateService.java Exposes findCertificateEntityByUserUuid.
src/main/java/com/czertainly/core/security/authn/client/CzertainlyAuthenticationClient.java Adds cache-aware helper methods for system user / UUID / cert / token authentication.
src/main/java/com/czertainly/core/security/authn/client/CzertainlyAuthenticationCache.java New cache implementation with token jti secondary index for per-user token eviction.
src/main/java/com/czertainly/core/security/authn/client/AuthenticationCache.java New interface defining cache operations + eviction API.
src/main/java/com/czertainly/core/security/authn/CzertainlyAuthenticationFilter.java Computes certificate thumbprint and routes to certificate auth via cache-aware client.
src/main/java/com/czertainly/core/config/CacheConfig.java Enables caching and configures a Caffeine-based CacheManager.
src/main/java/com/czertainly/core/auth/oauth2/OAuth2LoginFilter.java Routes token auth via cache-aware authenticateByToken.
src/main/java/com/czertainly/core/auth/oauth2/CzertainlyJwtAuthenticationConverter.java Routes token auth via cache-aware authenticateByToken.
pom.xml Adds Spring cache starter and Caffeine dependency.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/main/java/com/czertainly/core/service/impl/CertificateServiceImpl.java Outdated
@LukasNajman LukasNajman added this to ILM May 4, 2026
@github-project-automation github-project-automation Bot moved this to Planning in ILM May 4, 2026
@LukasNajman LukasNajman removed this from ILM May 4, 2026
@ivosh ivosh linked an issue May 4, 2026 that may be closed by this pull request
ivosh
ivosh previously requested changes May 5, 2026
Copy link
Copy Markdown
Collaborator

@ivosh ivosh left a comment

Choose a reason for hiding this comment

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

Good job with the authentication cache! And kudos for fixing the pre-existing problem with malformed certificate header.

I have a couple of generic comments which do not fit into individual lines of the source code changes:

  • Address the SonarQube issues
  • Prepare a follow-up PR for documentation changes against https://github.com/OmniTrustILM/documentation
  • Use the same approach with TokenJtiIndex for certificate fingerprints. Currently there are multiple call sites which need to retrieve the old cert and pass it to evictByUserUuid(userUuid, fingerprint). This is brittle and adds additional responsibility to the callers. The caching mechanism shall keep this information by itself because it has all the information available already. And this approach is already used by TokenJtiIndex. Call it UserCertificateIndex if you wish.
  • There is no real integration test. I expect at least a couple of them, exercising full roundtrip, such as: populate real cache -> mutate user -> verify loader re-invoked.
  • Have you considered wiring up cache eviction also on enableUser?
  • The cache parameters (TTL, size) must definitely come from the application properties (application.yml).
  • Wire up cache statistics into Micrometer stats.

Comment thread src/main/java/com/czertainly/core/security/authn/client/AuthenticationCache.java Outdated
Comment thread src/main/java/com/czertainly/core/security/authn/client/AuthenticationCache.java Outdated
Comment thread src/test/java/com/czertainly/core/service/CertificateServiceTest.java Outdated
@LukasNajman
Copy link
Copy Markdown
Collaborator Author

LukasNajman commented May 6, 2026

  • Address the SonarQube issues - One issue remains, fails possitive, won't fix it. But can't accepted in SQ, I don't have rights for that.
  • Prepare a follow-up PR for documentation changes against https://github.com/OmniTrustILM/documentation - Document the authentication cache documentation#293
  • Use the same approach with TokenJtiIndex for certificate fingerprints. Currently there are multiple call sites which need to retrieve the old cert and pass it to evictByUserUuid(userUuid, fingerprint). This is brittle and adds additional responsibility to the callers. The caching mechanism shall keep this information by itself because it has all the information available already. And this approach is already used by TokenJtiIndex. Call it UserCertificateIndex if you wish. - Implemented
  • There is no real integration test. I expect at least a couple of them, exercising full roundtrip, such as: populate real cache -> mutate user -> verify loader re-invoked. - added a bunch
  • Have you considered wiring up cache eviction also on enableUser? - Yes, if the used was disabled, it will not be in the cache, so there is no need of evicting it.
  • The cache parameters (TTL, size) must definitely come from the application properties (application.yml). - Added to application.yml and also to helm chart values
  • Wire up cache statistics into Micrometer stats. - wired

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented May 7, 2026

@ivosh ivosh self-requested a review May 7, 2026 08:08
@ivosh ivosh dismissed their stale review May 7, 2026 08:08

Review already performed.

Copy link
Copy Markdown
Collaborator

@ivosh ivosh left a comment

Choose a reason for hiding this comment

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

Ready for merge.

@ivosh ivosh merged commit 3804836 into main May 7, 2026
10 checks passed
@ivosh ivosh deleted the Authn-Cache branch May 7, 2026 08:10
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.

Authentication decision cache

4 participants