Added new classifier methods on jweService - #4495
Merged
Merged
Conversation
anushasunkada
requested review from
ThaminduDilshan,
hwupathum,
rajithacharith and
thiva-k
as code owners
August 2, 2026 12:45
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change exposes JWE algorithm capabilities through ChangesCrypto capability reporting and provider support
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/pkg/thunderidengine/config/config.go`:
- Around line 83-85: Update the SDK documentation under docs/content/sdks/ to
document the renamed DefaultRuntimeCryptoProviderConfig and
DefaultConfigCryptoProviderConfig types, the
WithDefaultRuntimeCryptoProviderConfigs and
WithDefaultConfigCryptoProviderConfig options, and migration guidance. Cover the
affected declarations in backend/pkg/thunderidengine/config/config.go at lines
83-85 and 149-151 and the option usage in backend/pkg/thunderidengine/engine.go
at lines 359-366; no code changes are required at these sites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 55fe0277-3af6-45fc-9b4c-9d5993b78b2f
⛔ Files ignored due to path filters (1)
backend/tests/mocks/jose/jwemock/JWEServiceInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (36)
backend/AGENTS.mdbackend/cmd/server/servicemanager.gobackend/internal/connection/init_test.gobackend/internal/entitytype/init_test.gobackend/internal/idp/init_test.gobackend/internal/idp/utils_test.gobackend/internal/inboundclient/init.gobackend/internal/inboundclient/service.gobackend/internal/inboundclient/service_test.gobackend/internal/notification/client/custom_client_test.gobackend/internal/notification/client/factory_test.gobackend/internal/notification/client/twilio_client_test.gobackend/internal/notification/client/vonage_client_test.gobackend/internal/notification/file_based_store_test.gobackend/internal/notification/init_test.gobackend/internal/notification/mgt_service_test.gobackend/internal/notification/notification_sender_service_test.gobackend/internal/notification/otp_service_test.gobackend/internal/notification/store_test.gobackend/internal/notification/utils_test.gobackend/internal/oauth/init.gobackend/internal/oauth/oauth2/discovery/discovery_test.gobackend/internal/oauth/oauth2/discovery/init.gobackend/internal/oauth/oauth2/discovery/service.gobackend/internal/system/config/config.gobackend/internal/system/export/init_test.gobackend/internal/system/export/service_test.gobackend/internal/system/importer/service_test.gobackend/internal/system/jose/init_test.gobackend/internal/system/jose/jwe/model.gobackend/internal/system/jose/jwe/service.gobackend/internal/system/jose/jwe/service_test.gobackend/internal/system/kmprovider/defaultkm/pki/service_test.gobackend/pkg/thunderidengine/config/config.gobackend/pkg/thunderidengine/engine.gobackend/pkg/thunderidengine/engine_test.go
Comment on lines
+83
to
+85
| // DefaultRuntimeCryptoProviderConfig holds the key configuration details for the default | ||
| // runtime crypto provider. | ||
| type DefaultRuntimeCryptoProviderConfig struct { |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🔴 Documentation Required
This PR introduces user-facing changes that are not covered by documentation updates under docs/.
Please update the relevant documentation before merging.
Missing documentation:
- Embedded engine crypto configuration API: Document the renamed
DefaultRuntimeCryptoProviderConfigandDefaultConfigCryptoProviderConfigtypes, plus the newWithDefaultRuntimeCryptoProviderConfigsandWithDefaultConfigCryptoProviderConfigoptions. Add migration guidance underdocs/content/sdks/.
As per path instructions, SDK-impacting changes without matching documentation updates require one consolidated PR-level comment.
📍 Affects 2 files
backend/pkg/thunderidengine/config/config.go#L83-L85(this comment)backend/pkg/thunderidengine/config/config.go#L149-L151backend/pkg/thunderidengine/engine.go#L359-L366
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/pkg/thunderidengine/config/config.go` around lines 83 - 85, Update
the SDK documentation under docs/content/sdks/ to document the renamed
DefaultRuntimeCryptoProviderConfig and DefaultConfigCryptoProviderConfig types,
the WithDefaultRuntimeCryptoProviderConfigs and
WithDefaultConfigCryptoProviderConfig options, and migration guidance. Cover the
affected declarations in backend/pkg/thunderidengine/config/config.go at lines
83-85 and 149-151 and the option usage in backend/pkg/thunderidengine/engine.go
at lines 359-366; no code changes are required at these sites.
Source: Path instructions
senthalan
reviewed
Aug 2, 2026
Comment on lines
+141
to
+143
| Encryption engineconfig.DefaultConfigCryptoProviderConfig `yaml:"encryption" json:"encryption"` | ||
| PasswordHashing PasswordHashingConfig `yaml:"password_hashing" json:"password_hashing"` | ||
| Keys []engineconfig.DefaultRuntimeCryptoProviderConfig `yaml:"keys" json:"keys"` |
Member
There was a problem hiding this comment.
I think, better to rename the keys for this configuration too. Shall we take this as a separate PR
Member
Author
There was a problem hiding this comment.
In thunderID we always use only one key manager, so renaming it as default in config.go will not be correct.
senthalan
reviewed
Aug 2, 2026
Comment on lines
+234
to
+240
| func (js *jweService) SupportedContentEncryptionAlgorithms() []string { | ||
| result := make([]string, len(supportedContentEncAlgorithms)) | ||
| for i, enc := range supportedContentEncAlgorithms { | ||
| result[i] = string(enc) | ||
| } | ||
| return result | ||
| } |
Member
There was a problem hiding this comment.
It should get the all the GetSupportedEncryptionAlgorithms from the cryptoProvider and filter out based on the knownKeyEncAlgorithms
anushasunkada
force-pushed
the
local_main
branch
2 times, most recently
from
August 4, 2026 06:20
45011dd to
ba9595d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/internal/system/kmprovider/defaultkm/runtime_crypto_provider.go`:
- Line 380: Update NewRuntimeCryptoService and GetSupportedEncryptionAlgorithms
so AES-GCM is advertised only when the runtime service has a non-nil cfgService,
matching the existing Encrypt and Decrypt checks; add the necessary nil-safe
construction behavior and coverage for NewRuntimeCryptoService(nil, ...) and its
advertised AES-GCM path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: badff87e-08ec-4e9e-a57f-71b88e9c7755
⛔ Files ignored due to path filters (1)
backend/tests/mocks/jose/jwemock/JWEServiceInterface_mock.gois excluded by!**/*_mock.go
📒 Files selected for processing (15)
backend/AGENTS.mdbackend/cmd/server/servicemanager.gobackend/internal/inboundclient/init.gobackend/internal/inboundclient/service.gobackend/internal/inboundclient/service_test.gobackend/internal/oauth/init.gobackend/internal/oauth/oauth2/discovery/discovery_test.gobackend/internal/oauth/oauth2/discovery/init.gobackend/internal/oauth/oauth2/discovery/service.gobackend/internal/system/i18n/core/defaults.gobackend/internal/system/jose/jwe/model.gobackend/internal/system/jose/jwe/service.gobackend/internal/system/jose/jwe/service_test.gobackend/internal/system/kmprovider/defaultkm/runtime_crypto_provider.gobackend/internal/system/kmprovider/defaultkm/runtime_crypto_provider_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- backend/internal/oauth/init.go
- backend/cmd/server/servicemanager.go
- backend/internal/oauth/oauth2/discovery/init.go
- backend/internal/oauth/oauth2/discovery/service.go
- backend/internal/system/jose/jwe/service_test.go
- backend/internal/inboundclient/init.go
- backend/AGENTS.md
- backend/internal/system/jose/jwe/service.go
- backend/internal/inboundclient/service.go
- backend/internal/system/jose/jwe/model.go
- backend/internal/oauth/oauth2/discovery/discovery_test.go
- backend/internal/inboundclient/service_test.go
| // GetSupportedEncryptionAlgorithms returns the list of algorithms supported by Encrypt and Decrypt. | ||
| func (s *runtimeCryptoService) GetSupportedEncryptionAlgorithms() []string { | ||
| return []string{ | ||
| string(cryptolib.AlgorithmAESGCM), |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 'runtimeCryptoService|cfgService|GetSupportedEncryptionAlgorithms' \
backend/internal/system/kmprovider/defaultkm --glob '*.go'Repository: thunder-id/thunderid
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- NewRuntimeCryptoService usages ---\n'
rg -n 'NewRuntimeCryptoService\b' --glob '*.go' --glob '*.go' | head -n 200
printf '\n--- Test constructors / nil cfgService relevant cases ---\n'
rg -n -C 4 'NewRuntimeCryptoService\(.*nil|cfgSvc:|cfgService:.*nil|\&runtimeCryptoService\{|GetSupportedEncryptionAlgorithms|EncryptAESGCM|DecryptAESGCM|AlgorithmAESGCM' backend/internal/system/kmprovider/defaultkm --glob '*.go' | sed -n '1,260p'
printf '\n--- AES-GCM supported encryption test count ---\n'
rg -n 'Test[A-Za-z0-9_]*(.*[Aa]es[Gg][Cc][Mm]|[Aa]LGORITHM[Aa][Ee][Ss][Gg][Cc][Mm]|AESGCM)|AlgorithmAESGCM' backend/internal/system/kmprovider/defaultkm --glob '*_test.go'Repository: thunder-id/thunderid
Length of output: 249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- NewRuntimeCryptoService usages ---'
rg -n 'NewRuntimeCryptoService\b' --glob '*.go' | head -n 200
printf '%s\n' ''
printf '%s\n' '--- Constructor nil tests/cases and AES capability references ---'
rg -n -C 4 'NewRuntimeCryptoService\(.*nil|cfgService:.*nil|cfgSvc:.*nil|\&runtimeCryptoService\{|GetSupportedEncryptionAlgorithms|AESGCM|AlgorithmAESGCM' backend/internal/system/kmprovider/defaultkm --glob '*.go' | sed -n '1,260p'
printf '%s\n' ''
printf '%s\n' '--- AES-GCM test names ---'
rg -n 'Test[A-Za-z0-9_]*(.*AE?SG?G?CM|AESGCM|AlgorithmAESGCM).*\{|AESGCM|AlgorithmAESGCM' backend/internal/system/kmprovider/defaultkm --glob '*_test.go'Repository: thunder-id/thunderid
Length of output: 431
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- NewRuntimeCryptoService usages ---"
rg -n 'NewRuntimeCryptoService\b' --glob '*.go' | head -n 200 || true
echo
echo "--- Constructor null/nil cases and supported AES-GCM references in defaultkm ---"
rg -n -C 4 'NewRuntimeCryptoService\(.*nil|cfgService:\s*nil|cfgSvc:\s*nil|\&runtimeCryptoService\s*\{|GetSupportedEncryptionAlgorithms|AESGCM|AlgorithmAESGCM' \
backend/internal/system/kmprovider/defaultkm --glob '*.go' | sed -n '1,260p' || true
echo
echo "--- AES-GCM test names/references in tests ---"
rg -n 'Test[A-Za-z0-9_]*(.*AE?SG?G?CM|AESGCM|AlgorithmAESGCM).*\{|AESGCM|AlgorithmAESGCM' \
backend/internal/system/kmprovider/defaultkm --glob '*_test.go' || trueRepository: thunder-id/thunderid
Length of output: 361
Keep AES-GCM capability in sync with construction state.
NewRuntimeCryptoService accepts cfgSvc without requiring a non-nil value, Encrypt/Decrypt reject AES-GCM when runtimeCryptoService.cfgService is nil, and GetSupportedEncryptionAlgorithms still advertises AES-GCM unconditionally. Add construction checks/null constructors and coverage for NewRuntimeCryptoService(nil, ...) with its advertised AES-GCM path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/internal/system/kmprovider/defaultkm/runtime_crypto_provider.go` at
line 380, Update NewRuntimeCryptoService and GetSupportedEncryptionAlgorithms so
AES-GCM is advertised only when the runtime service has a non-nil cfgService,
matching the existing Encrypt and Decrypt checks; add the necessary nil-safe
construction behavior and coverage for NewRuntimeCryptoService(nil, ...) and its
advertised AES-GCM path.
senthalan
reviewed
Aug 4, 2026
Comment on lines
+4
to
+20
| // Copyright 2025 The ThunderID Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| /* | ||
| * Copyright (c) 2025, WSO2 LLC. (https://www.wso2.com). | ||
| * | ||
| * WSO2 LLC. licenses this file to you under the Apache License, | ||
| * Version 2.0 (the "License"); you may not use this file except | ||
| * in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ |
Member
There was a problem hiding this comment.
shall we revert this change, we have updated the license header
anushasunkada
force-pushed
the
local_main
branch
from
August 4, 2026 09:06
ba9595d to
88fb507
Compare
senthalan
previously approved these changes
Aug 4, 2026
senthalan
added
Type/Improvement
trigger-pr-builder
Signed-off-by: anushasunkada <anushasunkada@gmail.com>
anushasunkada
force-pushed
the
local_main
branch
from
August 4, 2026 09:16
88fb507 to
1dd2751
Compare
ThaminduDilshan
approved these changes
Aug 4, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Approach
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
New Features
Tests
Chores