Decrypt token: Remove exceptions + use new ValidationParameters #2729

iNinja · 2024-07-17T14:40:50Z

Decrypt token: Remove exceptions + use new ValidationParameters

You've read the Contributor Guide and Code of Conduct.
You've included unit or integration tests for your change, where applicable.
You've included inline docs for your change, where applicable.
If any gains or losses in performance are possible, you've included benchmarks for your changes. More info
There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Description

Added new TokenDecryptingResult
Added missing delegate for ValidateAlgorithm
Updated ValidationParameters to provide the delegates required for token decryption
Refactored DecryptToken method and dependencies to not throw exceptions and return result type.

Raising this as a draft PR to discuss the approach and validate the plan.
I will add tests before making it official.

Part of #2711.

…alidationParameters

… understand its use and potential for exception throwing if used wrong.

src/Microsoft.IdentityModel.JsonWebTokens/JwtTokenUtilities.cs

src/Microsoft.IdentityModel.JsonWebTokens/TokenDecryptingResult.cs

src/Microsoft.IdentityModel.Tokens/Delegates.cs

src/Microsoft.IdentityModel.Tokens/Validation/ValidationParameters.cs

# Conflicts: # src/Microsoft.IdentityModel.Tokens/Validation/ValidationFailureType.cs

src/Microsoft.IdentityModel.Tokens/Validation/ValidationParameters.cs

… not trigger the right scenario.

src/Microsoft.IdentityModel.JsonWebTokens/Results/TokenDecryptingResult.cs

src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.DecryptToken.cs

src/Microsoft.IdentityModel.JsonWebTokens/JwtTokenUtilities.DecryptTokenResult.cs

… exception. Updated iterating over list.

… null.

FuPingFranco

LGTM, you'll need to rebase as my changes went it on Friday

# Conflicts: # src/Microsoft.IdentityModel.Tokens/Validation/ValidationParameters.cs

src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.DecryptToken.cs

src/Microsoft.IdentityModel.JsonWebTokens/JwtTokenUtilities.DecryptTokenResult.cs

jennyf19 · 2024-07-29T01:23:48Z

src/Microsoft.IdentityModel.JsonWebTokens/JwtTokenUtilities.DecryptTokenResult.cs

+        /// <param name="decryptionParameters">The decryption parameters container.</param>
+        /// <param name="callContext">The call context used for logging.</param>
+        /// <returns>The decrypted, and if the 'zip' claim is set, decompressed string representation of the token.</returns>
+        internal static TokenDecryptionResult DecryptJwtToken(


nit: any possibility of splitting this into smaller helper methods?

jennyf19 · 2024-07-29T01:24:50Z

src/Microsoft.IdentityModel.JsonWebTokens/JwtTokenUtilities.DecryptTokenResult.cs

+            string zipAlgorithm = null;
+            foreach (SecurityKey key in decryptionParameters.Keys)
+            {
+                var cryptoProviderFactory = validationParameters.CryptoProviderFactory ?? key.CryptoProviderFactory;


can the decryption logic be extracted to a helper method?

jennyf19 · 2024-07-29T01:34:56Z

src/Microsoft.IdentityModel.JsonWebTokens/JwtTokenUtilities.DecryptTokenResult.cs

+                    (exceptionStrings ??= new StringBuilder()).AppendLine(ex.ToString());
+                }
+
+                if (key != null)


do we need to implement any key management or caching (not related to the purpose of keysAttempted but more around overall impact on performance.

src/Microsoft.IdentityModel.JsonWebTokens/JwtTokenUtilities.cs

src/Microsoft.IdentityModel.Tokens/Validation/TokenDecryptionResult.cs

test/Microsoft.IdentityModel.TestUtils/IdentityComparer.cs

jennyf19

A few nits, nothing blocking.

Refactored token decryption to not throw exceptions and use the new V…

1425c5b

…alidationParameters

iNinja requested review from GeoK, brentschmaltz, FuPingFranco and keegan-caruso July 17, 2024 14:41

iNinja added 2 commits July 17, 2024 15:45

Updated documentation.

67ad94c

Changing DecryptionToken from property to method to make it easier to…

6b8f348

… understand its use and potential for exception throwing if used wrong.

iNinja commented Jul 17, 2024

View reviewed changes

src/Microsoft.IdentityModel.JsonWebTokens/JwtTokenUtilities.cs Outdated Show resolved Hide resolved

src/Microsoft.IdentityModel.JsonWebTokens/TokenDecryptingResult.cs Outdated Show resolved Hide resolved

src/Microsoft.IdentityModel.Tokens/Delegates.cs Show resolved Hide resolved

brentschmaltz reviewed Jul 17, 2024

View reviewed changes

src/Microsoft.IdentityModel.Tokens/Validation/ValidationParameters.cs Outdated Show resolved Hide resolved

brentschmaltz reviewed Jul 17, 2024

View reviewed changes

src/Microsoft.IdentityModel.Tokens/Validation/ValidationParameters.cs Outdated Show resolved Hide resolved

iNinja added 4 commits July 22, 2024 16:56

Updated variable names to make them more descriptive and future proof.

21dc2f5

Merge branch 'dev' into iinglese/decrypt-token-validation-parameters

213186f

# Conflicts: # src/Microsoft.IdentityModel.Tokens/Validation/ValidationFailureType.cs

Fail if no keys are provided when decrypting. Added tests

da9e2cd

Renamed delegate to match agreed pattern

28130c8

iNinja marked this pull request as ready for review July 23, 2024 16:56

iNinja requested a review from a team as a code owner July 23, 2024 16:56

iNinja requested a review from brentschmaltz July 23, 2024 16:56

FuPingFranco reviewed Jul 23, 2024

View reviewed changes

src/Microsoft.IdentityModel.Tokens/Validation/ValidationParameters.cs Outdated Show resolved Hide resolved

iNinja added 3 commits July 23, 2024 18:53

Disable ECDsa test on unsupported platforms

0ea7977

Disable ECDsa test on unsupported platforms

588b882

Moved TokenDecryptingResults to its own folder. Removed test that did…

8528ffb

… not trigger the right scenario.