Skip to content

Conversation

@Shane32
Copy link
Owner

@Shane32 Shane32 commented May 7, 2025

Summary by CodeRabbit

  • New Features

    • Added the ability to test JWT authentication with and without event handlers enabled.
    • Introduced a new test to verify custom claim injection during token validation.
  • Tests

    • Enhanced existing authentication tests to run with configurable JWT event handling.
    • Improved WebSocket authentication tests to assert event handler invocation.
    • Maintained backward compatibility by running tests in both modes.
  • Bug Fixes

    • Improved handling of token validation failures by triggering authentication failure events, allowing graceful recovery in some cases.

@Shane32 Shane32 self-assigned this May 7, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented May 7, 2025

📝 Walkthrough

"""

Walkthrough

The test class for JWT WebSocket authentication was updated to support conditional enabling of JWT bearer event handlers during tests. Test methods were refactored to run with and without event handling. New flags and event hooks were added for verifying event invocation and custom claim injection. A new test demonstrates custom claim validation. The authentication service was enhanced to trigger the AuthenticationFailed event when token validation fails without exceptions, allowing event-driven handling of such failures.

Changes

File(s) Change Summary
src/Tests/JwtBearer/JwtWebSocketAuthenticationServiceTests.cs Refactored test methods to accept a boolean parameter for enabling JWT events; added flags and event hooks for event verification; added a test for custom claim injection and validation; updated WebSocket test method to assert event invocation as needed; improved RSA key handling.
src/GraphQL.AspNetCore3.JwtBearer/JwtWebSocketAuthenticationService.cs Added handling for invalid token validation results by triggering the AuthenticationFailed event when JWT events are enabled, allowing event-driven override of authentication failure without exceptions.

Sequence Diagram(s)

sequenceDiagram
    participant TestMethod
    participant TestServer
    participant JwtBearerEvents
    participant WebSocketClient
    participant GraphQLResolver

    TestMethod->>TestServer: Create (with/without JwtBearerEvents)
    TestMethod->>WebSocketClient: Connect and authenticate
    WebSocketClient->>TestServer: Send JWT token
    TestServer->>JwtBearerEvents: (If enabled) Trigger OnMessageReceived
    TestServer->>JwtBearerEvents: (If enabled) Trigger OnTokenValidated/OnAuthenticationFailed
    TestServer->>GraphQLResolver: Invoke resolver (optionally validates custom claim)
    GraphQLResolver-->>TestMethod: Return result
    TestMethod->>TestMethod: Assert event flags and authentication result
Loading

Possibly related PRs

  • Shane32/GraphQL.AspNetCore3#86: Adds support for raising JwtBearer events in the authentication service and configuration extensions, which is directly tested and verified in this PR.
  • Shane32/GraphQL.AspNetCore3#82: Introduces the initial JWT Bearer authentication package and related infrastructure, which is extended and more thoroughly tested by the changes in this PR.
    """

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c3241a8 and a5d2503.

📒 Files selected for processing (1)
  • src/Tests/JwtBearer/JwtWebSocketAuthenticationServiceTests.cs (7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: test (windows-latest)
  • GitHub Check: test (ubuntu-latest)
🔇 Additional comments (12)
src/Tests/JwtBearer/JwtWebSocketAuthenticationServiceTests.cs (12)

20-20: Good performance optimization by making RSA parameters static.

The change to make _rsaParameters static helps avoid resource exhaustion on .NET Framework, which can only handle a limited number of RSA keys simultaneously. This optimization prevents potential test failures when running multiple tests that would otherwise each create their own RSA key.


25-32: Event tracking flags and JwtBearerEvents setup looks good.

The addition of event tracking flags and the JwtBearerEvents instance allows for thorough testing of JWT bearer authentication events. This is well-structured for verifying that events are firing as expected.


37-40: Field action for testing custom claims is well implemented.

The modification to include a test field action in the resolver is a good approach to verify custom claim injection and validation. This allows for flexible testing of claim-related scenarios.


43-57: JwtBearerEvents initialization is comprehensive.

The initialization of event handlers covers all three key JWT bearer events: message received, token validated, and authentication failed. The implementation correctly updates the tracking flags when events are triggered.


60-71: Good approach using Theory instead of Fact for parametrized testing.

Converting the test to a Theory with InlineData for both true and false values of enableJwtEvents allows testing with and without JWT event handling in a single test method, reducing code duplication.


73-100: Custom claim validation test is well structured.

This test thoroughly demonstrates adding a custom claim during token validation and verifying it in the GraphQL resolver, showing how JWT bearer events can be used for claim transformation.


318-328: Comprehensive event verification based on test configuration.

The assertions correctly verify that events were triggered as expected when enabled, or not triggered when disabled. The conditional checks match the expected behavior based on the authentication context.


353-362: Event verification for successful authentication is comprehensive.

For successful authentication, the assertions confirm that when events are enabled, both message received and token validated events fired, but not authentication failed. When events are disabled, it ensures no events fired.


381-382: JwtBearerEvents configuration is correctly applied.

The event handlers are properly assigned to the JWT bearer options, enabling the authentication middleware to invoke the configured handlers during the authentication process.


462-466: Static constructor for RSA key initialization reduces resource usage.

The static constructor initializes the RSA parameters once for all test instances, which is a good optimization for .NET Framework environments with limited RSA key capacity.


473-479: Improved key handling for negative test scenarios.

The refactored CreateSignedToken method now properly supports generating tokens with different keys without overwriting the static key, which provides better isolation for negative test cases.


285-287: TestWebSocketAsync method parameters provide granular control over expected event behavior.

The additional parameters allow for precise specification of expected event states, making the tests more targeted and reducing the likelihood of false positives when testing different scenarios.

✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

♻️ Duplicate comments (1)
src/Tests/JwtBearer/JwtWebSocketAuthenticationServiceTests.cs (1)

285-288: Insert flag reset here (see comment above)

Same recommendation as earlier—reset the booleans before establishing the WebSocket so that expectations reflect only this phase of the test.

🧹 Nitpick comments (2)
src/Tests/JwtBearer/JwtWebSocketAuthenticationServiceTests.cs (2)

37-41: Guard against null test actions for readability

_testFieldAction?.Invoke(ctx); is safe, but because it sits before the return statement the intent (optional extra assertions) can be lost to future readers.
Consider adding a small comment or extracting to a named local function, e.g. RunTestFieldAction(ctx);, to make this purpose crystal-clear.
No functional impact—pure readability.


43-57: Minor: include OnChallenge / OnForbidden if you need complete coverage

You track three core events, but JwtBearerEvents exposes two more that are often useful when debugging GraphQL authorization issues (OnChallenge, OnForbidden).
If you ever need to assert that a challenge was generated (401) versus an authorization failure (403), wiring those here will save you another plumbing round-trip later on.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f7b74e2 and a79237b.

📒 Files selected for processing (1)
  • src/Tests/JwtBearer/JwtWebSocketAuthenticationServiceTests.cs (5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: test (windows-latest)
  • GitHub Check: test (ubuntu-latest)

@coveralls
Copy link

coveralls commented May 7, 2025

Pull Request Test Coverage Report for Build 14895387759

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall first build on eventtests at 91.74%

Totals Coverage Status
Change from base Build 14893013165: 91.7%
Covered Lines: 2062
Relevant Lines: 2199

💛 - Coveralls

@Shane32 Shane32 changed the title Add tests for JWT bearer events Add tests for JWT bearer events; fix event when expired May 7, 2025
@github-actions
Copy link

github-actions bot commented May 7, 2025

Coverage Report

Totals Coverage
Statements: 93.77% ( 2062 / 2199 )
Methods: 80.88% ( 313 / 387 )

@Shane32 Shane32 merged commit e04baf0 into master May 7, 2025
6 checks passed
@Shane32 Shane32 deleted the eventtests branch May 7, 2025 23:51
@Shane32 Shane32 added this to the 7.1.0 milestone Jul 29, 2025
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.

3 participants