Skip to content

Replace AbstractSecretKeyLoader with ISecretKeyProvider, Fixes AB#3297746 #2666

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 24 commits into from
Jul 4, 2025
Merged

Replace AbstractSecretKeyLoader with ISecretKeyProvider, Fixes AB#3297746 #2666

merged 24 commits into from
Jul 4, 2025

Conversation

p3dr0rv
Copy link
Contributor

@p3dr0rv p3dr0rv commented Jun 12, 2025 •
edited
Loading

In the next PR:

  • We are replacing AbstractSecretKeyLoader with ISecretKeyProvider.
  • We are also delegating the secret key generation to a new class to separate concerns and improve code readability.

All these changes lay the groundwork for the next PR, where the new implementation will be introduced and the feature flag will be used to switch between the two implementations.

AB#3297746

@github-actions GitHub Actions
Copy link

❌ Work item link check failed. Description does not contain AB#{ID}.

Click here to Learn more.

@p3dr0rv p3dr0rv marked this pull request as ready for review June 12, 2025 00:24
@Copilot Copilot AI review requested due to automatic review settings June 12, 2025 00:24
@p3dr0rv p3dr0rv requested a review from a team as a code owner June 12, 2025 00:24
Copilot

This comment was marked as outdated.

Sign in to view

@github-actions GitHub Actions
Copy link

✅ Work item link check complete. Description contains link AB#3297746 to an Azure Boards work item.

@github-actions github-actions bot changed the title Replace AbstractSecretKeyLoader with ISecretKeyLoader Replace AbstractSecretKeyLoader with ISecretKeyLoader, Fixes AB#3297746 Jun 12, 2025
@p3dr0rv p3dr0rv requested a review from Copilot June 12, 2025 03:48
Copilot

This comment was marked as outdated.

Sign in to view

@p3dr0rv p3dr0rv added the Skip-Consumers-Check Only include this if making a breaking change purposefully, and there is an MSAL/ADAL/Broker PR label Jun 20, 2025
mohitc1
mohitc1 approved these changes Jun 25, 2025
View reviewed changes
Copy link
Contributor

@mohitc1 mohitc1 left a comment

Choose a reason for hiding this comment

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

Left some suggestions

p3dr0rv added 4 commits July 2, 2025 18:10
@p3dr0rv
Merge branch 'dev' into pedroro/interface-for-secretkeyloader
f994746
@p3dr0rv
Refactor key loading mechanism to use ISecretKeyProvider interface, r…
5aea5bc 
…eplacing ISecretKeyLoader. Update related classes and tests to ensure compatibility with new key provider structure. Remove deprecated AndroidWrappedKeyLoaderFactory and adjust encryption manager implementations accordingly.
@p3dr0rv
Remove keySize and keyAlgorithm properties from ISecretKeyGenerator a…
2b56431 
…nd AES256SecretKeyGenerator. Update tests accordingly to reflect these changes.
@p3dr0rv
fix issues
cd2b17a
@p3dr0rv p3dr0rv changed the title Replace AbstractSecretKeyLoader with ISecretKeyLoader, Fixes AB#3297746 Replace AbstractSecretKeyLoader with ISecretKeyProvider, Fixes AB#3297746 Jul 3, 2025
@p3dr0rv p3dr0rv requested a review from a team as a code owner July 3, 2025 18:06
rpdome
rpdome approved these changes Jul 3, 2025
View reviewed changes
Copy link
Member

@rpdome rpdome left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Left a few suggestions. otherwise lgtm.

p3dr0rv added 5 commits July 3, 2025 12:57
@p3dr0rv
Refactor key management to implement ISecretKeyProvider interface, re…
c2c8273 
…placing AbstractAES256SecretKeyProvider. Update related classes and tests for compatibility. Remove deprecated classes.
@p3dr0rv
fix tests
37a9022
@p3dr0rv
Refactor key loader to key provider in encryption manager
4176ee0 
- Updated the AndroidAuthSdkStorageEncryptionManagerTest to replace instances of ISecretKeyLoader with ISecretKeyProvider.
- Refactored StorageEncryptionManager to use key providers instead of key loaders for encryption and decryption processes.
- Introduced MockAES256KeyProvider and MockAES256KeyProviderWithGetKeyError classes to facilitate testing with the new key provider interface.
- Adjusted StorageEncryptionManagerTest to utilize the new MockAES256KeyProvider for testing encryption and decryption functionalities.
- Ensured all references to key loaders in the codebase are replaced with key providers for consistency and clarity.
@p3dr0rv
Rename method to setIgnoreKeyProviderNotFoundError for consistency in…
0411dfb 
… AuthenticationSettings
@p3dr0rv
Rename key loader variable to key provider for consistency in Android…
1096d86 
…AuthSdkStorageEncryptionManager and update related test assertion.
@p3dr0rv p3dr0rv mentioned this pull request Jul 3, 2025
Merged
@p3dr0rv p3dr0rv requested a review from Copilot July 3, 2025 22:56
Copilot
Copilot AI reviewed Jul 3, 2025
View reviewed changes
Copy link
Contributor

@Copilot 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 replaces the legacy AbstractSecretKeyLoader abstraction with the new ISecretKeyProvider interface and delegates secret key generation to a dedicated AES256SecretKeyGenerator object for clearer separation of concerns.

  • Introduce ISecretKeyProvider and ISecretKeyGenerator interfaces
  • Implement AES256SecretKeyGenerator and refactor existing loaders/providers
  • Update all consumers and tests to use the new interfaces and generator

Reviewed Changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.

Show a summary per file
File Description
common4j/src/main/java/com/microsoft/identity/common/java/crypto/key/ISecretKeyProvider.kt New interface defining key providers
common4j/src/main/java/com/microsoft/identity/common/java/crypto/key/AES256SecretKeyGenerator.kt New singleton implementing key generation
common4j/src/main/java/com/microsoft/identity/common/java/crypto/StorageEncryptionManager.java Refactored to call ISecretKeyProvider methods
common4j/src/test/java/com/microsoft/identity/common/java/crypto/MockAES256KeyProvider*.java Updated mocks to implement ISecretKeyProvider
common/src/main/java/com/microsoft/identity/common/crypto/AndroidWrappedKeyProvider.java Refactored loader to provider

@p3dr0rv p3dr0rv merged commit 17f0b77 into dev Jul 4, 2025
19 of 20 checks passed
@p3dr0rv p3dr0rv deleted the pedroro/interface-for-secretkeyloader branch July 4, 2025 05:16
p3dr0rv added a commit to AzureAD/microsoft-authentication-library-for-android that referenced this pull request Jul 4, 2025
@p3dr0rv
Refactor ShadowAndroidSdkStorageEncryptionManager to use ISecretKeyPr…
8cc0c1a 
…ovider for key management, Fixes AB#3297746 (#2326)

Update necessary from: 

AzureAD/microsoft-authentication-library-common-for-android#2666


[AB#3297746](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3297746)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@rpdome rpdome rpdome approved these changes

@mohitc1 mohitc1 mohitc1 approved these changes

Assignees
No one assigned
Labels
Skip-Consumers-Check Only include this if making a breaking change purposefully, and there is an MSAL/ADAL/Broker PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@p3dr0rv @rpdome @mohitc1