V9.0.5/digest algorithm support #118
Conversation
WalkthroughThe pull request updates the digest authentication implementation by replacing the legacy Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant H as DigestAuthHandler
participant M as DigestAuthMiddleware
participant O as AuthOptions
participant B as AuthHeaderBuilder
participant F as DigestHashFactory
C->>H: Request resource
H->>M: Initiate authentication challenge
M->>O: Retrieve DigestAlgorithm configuration
O-->>M: Return DigestAlgorithm (e.g., Sha256)
M->>B: Build WWW-Authenticate header using DigestAlgorithm
B->>F: Compute digest hash based on provided algorithm
F-->>B: Return computed hash
B-->>M: Deliver authentication header
M-->>H: Forward header for challenge response
H-->>C: Send challenge response
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (15)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthorizationHeaderBuilder.cs:76
- [nitpick] The obsolete 'Algorithm' property coexists with the new 'DigestAlgorithm' property, which may lead to confusion. Consider removing or clearly documenting this obsolete property to clarify its role for backward compatibility.
public UnkeyedCryptoAlgorithm Algorithm { get; }
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthorizationHeaderBuilder.cs (2)
293-294: Minor optimization suggestion: avoid recreating the hash instance.
You could store theHashobject in a local variable rather than callingDigestHashFactory.CreateCrypto(DigestAlgorithm)multiple times to marginally improve performance.For example:
- var hashFields = ... - : string.Create(CultureInfo.InvariantCulture, $"{method}:{Data[DigestFields.DigestUri]}:{DigestHashFactory.CreateCrypto(DigestAlgorithm).ComputeHash(entityBody, ...).ToHexadecimalString()}"); - return DigestHashFactory.CreateCrypto(DigestAlgorithm).ComputeHash(hashFields, ...).ToHexadecimalString(); + var factory = DigestHashFactory.CreateCrypto(DigestAlgorithm); + var computedEntity = factory.ComputeHash(entityBody, o => o.Encoding = Encoding.UTF8).ToHexadecimalString(); + var hashFields = !hasIntegrityProtection + ? string.Create(CultureInfo.InvariantCulture, $"{method}:{Data[DigestFields.DigestUri]}") + : string.Create(CultureInfo.InvariantCulture, $"{method}:{Data[DigestFields.DigestUri]}:{computedEntity}"); + return factory.ComputeHash(hashFields, o => o.Encoding = Encoding.UTF8).ToHexadecimalString();
311-311: Same optimization applies toComputeResponse.
Reusing a singleHashinstance avoids multiple factory lookups per call.- return DigestHashFactory.CreateCrypto(DigestAlgorithm).ComputeHash( + var factory = DigestHashFactory.CreateCrypto(DigestAlgorithm); + return factory.ComputeHash( string.Create(...), o => ... ).ToHexadecimalString();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthenticationHandler.cs(1 hunks)src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthenticationMiddleware.cs(2 hunks)src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthenticationOptions.cs(4 hunks)src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthorizationHeaderBuilder.cs(8 hunks)src/Cuemon.AspNetCore.Authentication/Digest/DigestCryptoAlgorithm.cs(1 hunks)src/Cuemon.AspNetCore.Authentication/Digest/DigestHashFactory.cs(1 hunks)test/Cuemon.AspNetCore.Authentication.Tests/Digest/DigestAccessAuthenticationHandlerTest.cs(9 hunks)test/Cuemon.AspNetCore.Authentication.Tests/Digest/DigestAccessAuthenticationMiddlewareTest.cs(3 hunks)test/Cuemon.AspNetCore.Authentication.Tests/DigestAuthenticationOptionsTest.cs(1 hunks)test/Cuemon.Extensions.AspNetCore.Authentication.Tests/AuthorizationResponseHandlerTest.cs(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
src/Cuemon.AspNetCore.Authentication/Digest/DigestCryptoAlgorithm.cs (1)
src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthorizationHeaderBuilder.cs (1)
DigestCryptoAlgorithm(214-223)
src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthenticationHandler.cs (2)
src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthenticationMiddleware.cs (5)
DigestAuthorizationHeader(135-138)DigestAuthenticationMiddleware(19-160)DigestAuthenticationMiddleware(26-28)DigestAuthenticationMiddleware(35-37)ParseAlgorithm(140-159)src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthorizationHeader.cs (4)
DigestAuthorizationHeader(14-217)DigestAuthorizationHeader(27-31)DigestAuthorizationHeader(38-40)DigestAuthorizationHeader(55-67)
test/Cuemon.AspNetCore.Authentication.Tests/DigestAuthenticationOptionsTest.cs (1)
src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthorizationHeaderBuilder.cs (1)
DigestCryptoAlgorithm(214-223)
src/Cuemon.AspNetCore.Authentication/Digest/DigestHashFactory.cs (1)
src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthorizationHeaderBuilder.cs (1)
DigestCryptoAlgorithm(214-223)
src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthenticationOptions.cs (1)
src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthorizationHeaderBuilder.cs (2)
DigestCryptoAlgorithm(214-223)Obsolete(29-48)
test/Cuemon.Extensions.AspNetCore.Authentication.Tests/AuthorizationResponseHandlerTest.cs (1)
src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthorizationHeaderBuilder.cs (1)
DigestCryptoAlgorithm(214-223)
src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthorizationHeaderBuilder.cs (3)
src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthenticationMiddleware.cs (1)
ParseAlgorithm(140-159)src/Cuemon.AspNetCore.Authentication/AuthorizationHeaderBuilder.cs (1)
ValidateData(99-107)src/Cuemon.AspNetCore.Authentication/Digest/DigestHashFactory.cs (1)
DigestHashFactory(10-34)
test/Cuemon.AspNetCore.Authentication.Tests/Digest/DigestAccessAuthenticationHandlerTest.cs (1)
src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthorizationHeaderBuilder.cs (1)
DigestCryptoAlgorithm(214-223)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: 🐧 Prepare Linux
- GitHub Check: 🪟 Prepare Windows
🔇 Additional comments (37)
src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthenticationHandler.cs (1)
69-69: Properly migrated to use DigestAlgorithm instead of AlgorithmThe change correctly updates the WWWAuthenticate header construction to use the new
Options.DigestAlgorithminstead of the deprecatedAlgorithmproperty. This aligns with the overall refactoring to use the more specificDigestCryptoAlgorithmtype throughout the codebase.src/Cuemon.AspNetCore.Authentication/Digest/DigestCryptoAlgorithm.cs (1)
6-37: Well-structured enumeration for digest algorithmsThe newly added
DigestCryptoAlgorithmenum is well-designed with:
- Clear documentation for each algorithm variant
- Appropriate naming that follows C# conventions
- Support for both standard and session variants of each algorithm
- Explicit integer values for each enum member
This provides a more type-safe approach compared to the previous implementation and clearly communicates the supported digest algorithms.
One observation: The enum values start from -2 rather than 0, which is slightly unusual but likely intentional for backward compatibility or other technical reasons.
test/Cuemon.AspNetCore.Authentication.Tests/Digest/DigestAccessAuthenticationMiddlewareTest.cs (3)
120-120: Correctly updated to use DigestAlgorithm propertyThe test has been properly updated to use the new
DigestAlgorithmproperty when instantiating theDigestAuthorizationHeaderBuilder, aligning with the changes in the main implementation.
196-196: Correctly updated to use DigestAlgorithm propertyThe test has been properly updated to use the new
DigestAlgorithmproperty when instantiating theDigestAuthorizationHeaderBuilder, aligning with the changes in the main implementation.
271-271: Correctly updated to use DigestAlgorithm propertyThe test has been properly updated to use the new
DigestAlgorithmproperty when instantiating theDigestAuthorizationHeaderBuilder, aligning with the changes in the main implementation.test/Cuemon.AspNetCore.Authentication.Tests/DigestAuthenticationOptionsTest.cs (1)
182-182: Correctly updated default value assertionThe test has been properly updated to assert the default value of the new
DigestAlgorithmproperty isDigestCryptoAlgorithm.Sha256, replacing the previous assertion that checked theAlgorithmproperty.src/Cuemon.AspNetCore.Authentication/Digest/DigestHashFactory.cs (1)
1-35: Well-designed factory implementation with good algorithm coverageThe new
DigestHashFactoryclass provides a clean factory pattern implementation for creating hash instances based on digest algorithms. The mapping betweenDigestCryptoAlgorithmvalues and the appropriate hash implementations is straightforward and well-handled:
- Properly maps both regular and session variants to their corresponding hash implementations
- Uses proper exception handling for unsupported algorithms
- Maintains good default value handling
src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthenticationOptions.cs (5)
30-32: LGTM! Documentation updated properly for the new DigestAlgorithm propertyThe documentation in the constructor has been correctly updated to reference the new
DigestAlgorithmproperty.
61-61: Properly initializes the new DigestAlgorithm propertyThe constructor initializes the new property with the appropriate default value of
DigestCryptoAlgorithm.Sha256.
106-107: Good use of obsolete attribute for the old Algorithm propertyThe obsolete attribute correctly informs developers that this property will be removed in a future version and provides guidance on using the new
DigestAlgorithmproperty instead.
109-112: LGTM! New DigestAlgorithm property is well-documentedThe property documentation clearly specifies its purpose and default value.
148-148: Documentation updated to reference the new enum typeThe remarks for
UseServerSideHa1Storagehave been updated to referenceDigestCryptoAlgorithminstead of the old enum type, maintaining documentation consistency.test/Cuemon.Extensions.AspNetCore.Authentication.Tests/AuthorizationResponseHandlerTest.cs (4)
549-549: Test properly updated to use the new DigestAlgorithm propertyThe test code has been correctly updated to initialize the
DigestAuthorizationHeaderBuilderwithoptions.DigestAlgorithminstead of the deprecatedoptions.Algorithm.
597-597: Test configuration updated to use the new enum typeThe test configuration now uses
DigestCryptoAlgorithm.Sha512Slash256instead ofUnkeyedCryptoAlgorithm.Sha512, correctly aligning with the API changes.
626-626: Header builder initialization updated to use the new DigestAlgorithm propertyThe initialization of
DigestAuthorizationHeaderBuildercorrectly uses the newoptions.DigestAlgorithmproperty.
705-705: LGTM! Consistent use of the new DigestAlgorithm property across testsThis test case also correctly uses the new
options.DigestAlgorithmproperty, ensuring consistency across all test methods.src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthenticationMiddleware.cs (2)
63-63: WWW-Authenticate header generation updated to use the new DigestAlgorithm propertyThe middleware now correctly uses
Options.DigestAlgorithminstead of the deprecatedOptions.Algorithmwhen generating the WWW-Authenticate header.
140-159: ParseAlgorithm method updated to handle the new DigestCryptoAlgorithm enumThe method signature has been updated to accept
DigestCryptoAlgorithminstead ofUnkeyedCryptoAlgorithm, and the switch statement now handles all possible values of the new enum, including session variants with appropriate string representations.This implementation:
- Properly maps algorithm enums to their string representations
- Handles session variants with the "-sess" suffix
- Maintains compatibility with HTTP Digest Authentication specifications
- Provides a sensible default for unrecognized values
src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthorizationHeaderBuilder.cs (10)
2-2: Import usage looks appropriate.
The newly addedSystem.ComponentModelimport is needed forInvalidEnumArgumentExceptionin the updated constructor.
17-22: Default constructor sets a secure default.
Having the parameterless constructor default toDigestCryptoAlgorithm.Sha256is a good choice for a secure baseline algorithm.
29-49: Obsolete constructor implementation is consistent.
Deprecating theUnkeyedCryptoAlgorithmapproach and mapping to the newDigestCryptoAlgorithmensures backward compatibility while providing clear guidance to migrate.
50-54: New constructor aligns well with the updated pattern.
AcceptingDigestCryptoAlgorithmdirectly is more intuitive and strongly typed.
75-76: Marking the old property as obsolete is appropriate.
TheAlgorithmproperty is clearly deprecated in favor ofDigestAlgorithm.
78-82: Introducing a dedicatedDigestAlgorithmproperty is a clean improvement.
Promoting this to a first-class property makes the code more readable and maintainable going forward.
203-203: Parsing intoDigestAlgorithmensures consistent usage.
You may want to confirm thatheader.Algorithmis never null or empty before callingParseAlgorithm, but otherwise looks good.Would you like to add a null-check for
header.Algorithm?
214-223:ParseAlgorithmimplementation covers all expected enum variants.
Throwing aNotSupportedExceptionfor unknown inputs is a safe approach.
244-244: Doc comments reflect the new property accurately.
They clearly convey usage ofDigestAlgorithmin the computed hash value.
249-274:ComputeHash1logic correctly trimsSHA-512-256results and handles session variants.
This ensures you only keep 256 bits from the 512-bit hash. Implementation is correct.test/Cuemon.AspNetCore.Authentication.Tests/Digest/DigestAccessAuthenticationHandlerTest.cs (9)
28-35: Switching from[Fact]to[Theory]allows robust multi-algorithm testing.
This approach improves coverage by validating multiple digest algorithms in a single test method.
58-58: Use ofo.DigestAlgorithm = algorithm;is consistent with the new pattern.
Replacing the obsolete property ensures correct usage of the updated approach.
82-82: Passingoptions.DigestAlgorithminto the builder is now aligned with options.
This harmonizes the test setup with production code usage.
111-118: Parameterized tests for QOP Authentication Integrity are comprehensive.
Implementing[Theory]with multipleDigestCryptoAlgorithmvalues ensures broad coverage of algorithm variants.
141-141: AssigningDigestAlgorithmin test ensures an accurate reference.
Demonstrates consistent usage across different test scenarios.
165-165: Same expression ofDigestAlgorithmaligns with other test cases.
Ensures you are testing the correct property in a uniform manner.
206-206: Explicitly settingDigestAlgorithmtoSha512Slash256confirms coverage for that variant.
This is beneficial for verifying server-side HA1 storage with a more complex algorithm.
212-212: Test data for HA1 is correct forSha512Slash256.
The precomputed value effectively simulates a server-side stored HA1 hash.
270-270: Assertion verifies the correct algorithm is in effect.
This ensures the final environment is aligned with the intendedDigestAlgorithm.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #118 +/- ##
==========================================
+ Coverage 80.13% 80.17% +0.04%
==========================================
Files 593 596 +3
Lines 18328 18513 +185
Branches 1884 1902 +18
==========================================
+ Hits 14687 14843 +156
- Misses 3573 3599 +26
- Partials 68 71 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This pull request focuses on refactoring the
DigestAuthenticationfeature in theCuemon.AspNetCore.Authenticationpackage. The main changes include replacing theUnkeyedCryptoAlgorithmwith a newDigestCryptoAlgorithm, updating the relevant methods and properties accordingly, and marking some members as obsolete.Refactoring Digest Authentication:
src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthenticationHandler.cs: Updated theHandleChallengeAsyncmethod to useOptions.DigestAlgorithminstead ofOptions.Algorithm.src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthenticationMiddleware.cs: Replaced theParseAlgorithmmethod and related usages to work withDigestCryptoAlgorithminstead ofUnkeyedCryptoAlgorithm. [1] [2]src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthenticationOptions.cs: Introduced a newDigestAlgorithmproperty to replace theAlgorithmproperty, which is now marked as obsolete. [1] [2] [3]Updating Digest Authorization Header Builder:
src/Cuemon.AspNetCore.Authentication/Digest/DigestAuthorizationHeaderBuilder.cs: Added a new constructor and property forDigestCryptoAlgorithm, and marked the oldUnkeyedCryptoAlgorithmconstructor and property as obsolete. Updated methods to useDigestCryptoAlgorithmfor computing hash values. [1] [2] [3] [4] [5] [6] [7] [8]Summary by CodeRabbit
New Features
Refactor
Tests