Skip to content

Conversation

mativm02
Copy link
Contributor

@mativm02 mativm02 commented Sep 23, 2025

User description

TT-15671
Summary Implement securityProcessingMode for compliant processing of OpenAPI security config
Type Story Story
Status Ready for Testing
Points N/A
Labels -

This commit introduces two new test cases in mw_auth_or_wrapper_test.go:

  1. TestVendorExtension_MixedANDOR_LegacyMode: Tests the behavior of the vendor extension in mixed AND/OR legacy mode, ensuring proper session management and API specification handling.
  2. TestEmptyOASCompliant: Validates the handling of empty OAS compliant requests, checking for correct responses when no API key or invalid keys are provided.

These tests enhance coverage for the new features related to vendor extensions and OAS compliance, ensuring robust functionality in various scenarios.


PR Type

Tests


Description

  • Add mixed AND/OR auth mode tests

  • Cover legacy vs compliant behaviors

  • Validate empty OAS security handling

  • Include HMAC, JWT, API key paths


Diagram Walkthrough

flowchart LR
  A["New test cases"] -- "Mixed AND/OR" --> B["Legacy mode"]
  A -- "Mixed AND/OR" --> C["Compliant mode"]
  A -- "Empty OAS" --> D["Legacy mode"]
  A -- "Empty OAS" --> E["Compliant mode"]
  B -- "JWT+HMAC, API key" --> F["Expect 400/401/403"]
  C -- "API key OK, JWT/HMAC checks" --> G["Expect 200/401/403"]
  D -- "HMAC required" --> H["200 with valid HMAC"]
  E -- "HMAC preferred; API key invalid" --> I["200 with HMAC, 400 otherwise"]
Loading

File Walkthrough

Relevant files
Tests
mw_auth_or_wrapper_test.go
Add mixed/compliant and empty OAS security tests                 

gateway/mw_auth_or_wrapper_test.go

  • Add tests for mixed AND/OR in legacy mode
  • Add tests for mixed AND/OR in compliant mode
  • Add tests for empty OAS in legacy and compliant modes
  • Exercise JWT, HMAC, API key, OAuth2, and custom auth combos
+1009/-0

This commit introduces two new test cases in `mw_auth_or_wrapper_test.go`:
1. `TestVendorExtension_MixedANDOR_LegacyMode`: Tests the behavior of the vendor extension in mixed AND/OR legacy mode, ensuring proper session management and API specification handling.
2. `TestEmptyOASCompliant`: Validates the handling of empty OAS compliant requests, checking for correct responses when no API key or invalid keys are provided.

These tests enhance coverage for the new features related to vendor extensions and OAS compliance, ensuring robust functionality in various scenarios.
@buger
Copy link
Member

buger commented Sep 23, 2025

Let's make that PR title a 💯 shall we? 💪

Your PR title and story title look slightly different. Just checking in to know if it was intentional!

Story Title Implement securityProcessingMode for compliant processing of OpenAPI security config
PR Title Add tests for mixed AND/OR legacy mode and empty OAS compliant scenarios

Check out this guide to learn more about PR best-practices.

@buger
Copy link
Member

buger commented Sep 23, 2025

This PR is too huge for one to review 💔

Additions 1009 🙅‍♀️
Expected ⬇️ 800

Consider breaking it down into multiple small PRs.

Check out this guide to learn more about PR best-practices.

Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Flaky Timing

HMAC signature tests rely on Date header formatted with local timezone (MST token). If test env timezone varies, signature validation may fail intermittently. Consider using time.FixedZone/UTC and consistent format.

date := time.Now().Format("Mon, 02 Jan 2006 15:04:05 MST")
headers := map[string]string{
	"Date": date,
}

signature := generateHMACSignature("GET", "/test-mixed-andor-legacy/", headers, hmacSecret, "hmac-sha1")
_ = signature

testCase := test.TestCase{
	Method: "GET",
	Path:   "/test-mixed-andor-legacy/",
	Headers: map[string]string{
		"Authorization": "Bearer " + jwtToken,
		"Date":          date,
	},
	Code: http.StatusBadRequest,
}

ts.Run(t, testCase)
Redundant Case

In legacy mixed AND/OR tests, two identical JWT-only cases both expect 400; deduplicate to keep the suite concise and reduce runtime.

	Method: "GET",
	Path:   "/test-mixed-andor-legacy/",
	Headers: map[string]string{
		"Authorization": "Bearer " + jwtToken,
	},
	Code: http.StatusBadRequest,
},
Unused Variable

The generated HMAC signature is computed but not sent in headers in the legacy mixed test, making the subtest not validate combined JWT+HMAC behavior as intended.

signature := generateHMACSignature("GET", "/test-mixed-andor-legacy/", headers, hmacSecret, "hmac-sha1")
_ = signature

testCase := test.TestCase{
	Method: "GET",
	Path:   "/test-mixed-andor-legacy/",
	Headers: map[string]string{
		"Authorization": "Bearer " + jwtToken,
		"Date":          date,
	},
	Code: http.StatusBadRequest,

Copy link
Contributor

API Changes

no api changes detected

Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Align HMAC header format

The HMAC auth source for compliant complex test is set to header name Authorization
with HTTP Signature format, but the test sends a non-signature value HMAC
some-signature. This mismatch may produce misleading results; either construct a
proper HTTP Signature header or use the configured header name/value format
consistently.

gateway/mw_auth_or_wrapper_test.go [3197-3203]

 {
   Method: "GET",
   Path:   "/test-complex-compliant/",
   Headers: map[string]string{
-    "Authorization": "HMAC some-signature",
+    "Authorization": fmt.Sprintf(`Signature keyId="%s",algorithm="hmac-sha1",signature="invalid"`, "wrong"),
   },
   Code: http.StatusBadRequest,
 },
Suggestion importance[1-10]: 7

__

Why: The existing code sends an HMAC value that doesn’t match the configured HTTP Signature format; recommending a proper Signature header is accurate and improves test validity without changing semantics.

Medium
Remove unused HMAC signature

The generated HMAC signature is unused, making the test ineffective for validating
combined JWT+HMAC behavior. Either construct and send the HMAC header or remove the
dead code to avoid false confidence.

gateway/mw_auth_or_wrapper_test.go [2736-2737]

-signature := generateHMACSignature("GET", "/test-mixed-andor-legacy/", headers, hmacSecret, "hmac-sha1")
-_ = signature
+// Remove unused signature generation since HMAC header isn't sent in this test.
+// signature := generateHMACSignature("GET", "/test-mixed-andor-legacy/", headers, hmacSecret, "hmac-sha1")
Suggestion importance[1-10]: 5

__

Why: The snippet matches the new hunk and the signature is indeed unused, so removing it avoids dead code; impact is minor and doesn't change test outcomes.

Low
Possible issue
Send HMAC header with JWT

In legacy mode the test later computes an HMAC signature but does not send it;
expecting 400 may mask auth evaluation. To assert mixed AND/OR correctly, include
the computed HMAC header (using the chosen header name) alongside the JWT so the
engine evaluates the AND group.

gateway/mw_auth_or_wrapper_test.go [2736-2747]

-tykExtension := &oas.XTykAPIGateway{
-  ...
-  Server: oas.Server{
-    ...
-    Authentication: &oas.Authentication{
-      Enabled:                true,
-      SecurityProcessingMode: oas.SecurityProcessingModeLegacy,
-
-      Security: [][]string{
-        {"jwt", "hmac"},
-        {"apikey"},
-      },
-      SecuritySchemes: oas.SecuritySchemes{
-        "jwt": &oas.JWT{
-          Enabled:           true,
-          Source:            base64.StdEncoding.EncodeToString([]byte(jwtRSAPubKey)),
-          SigningMethod:     "rsa",
-          IdentityBaseField: "user_id",
-          PolicyFieldName:   "policy_id",
-          DefaultPolicies:   []string{pID},
-          AuthSources: oas.AuthSources{
-            Header: &oas.AuthSource{
-              Enabled: true,
-              Name:    "Authorization",
-            },
-          },
-        },
-        "apikey": &oas.Token{
-          Enabled: func() *bool { b := true; return &b }(),
-          AuthSources: oas.AuthSources{
-            Header: &oas.AuthSource{
-              Enabled: true,
-              Name:    "X-API-Key",
-            },
-          },
-        },
-        "hmac": &oas.HMAC{
-          Enabled: true,
-          AuthSources: oas.AuthSources{
-            Header: &oas.AuthSource{
-              Enabled: true,
-              Name:    "Authorization",
-            },
-          },
-        },
-      },
-    },
+// Use computed signature in header to exercise {"jwt","hmac"} group.
+date := time.Now().Format("Mon, 02 Jan 2006 15:04:05 MST")
+headers := map[string]string{"Date": date}
+signature := generateHMACSignature("GET", "/test-mixed-andor-legacy/", headers, hmacSecret, "hmac-sha1")
+authHeader := fmt.Sprintf(`Signature keyId="%s",algorithm="hmac-sha1",headers="(request-target) date",signature="%s"`, hmacKey, signature)
+testCase := test.TestCase{
+  Method: "GET",
+  Path:   "/test-mixed-andor-legacy/",
+  Headers: map[string]string{
+    "Authorization": "Bearer " + jwtToken + ", " + authHeader,
+    "Date":          date,
   },
-  ...
+  Code: http.StatusBadRequest,
 }
Suggestion importance[1-10]: 4

__

Why: It correctly identifies that the computed HMAC isn’t sent and proposes using it, but the combined header format shown is likely invalid for two auth schemes and the expected status remains unchanged, reducing reliability.

Low

Copy link

probelabs bot commented Sep 23, 2025

🔍 Code Analysis Results

This is an excellent pull request that adds crucial test coverage for complex authentication scenarios within the Tyk Gateway. Here is a detailed analysis of the changes.

1. Change Impact Analysis

What this PR accomplishes

This pull request significantly enhances the test suite for the Tyk Gateway's authentication middleware. It introduces a comprehensive set of tests for APIs defined using OpenAPI Specifications (OAS) that utilize the x-tyk-api-gateway vendor extension.

The primary goal is to validate the behavior of the securityProcessingMode setting, which governs how the gateway interprets and enforces security requirements. The PR adds tests for two key modes:

  • legacy mode: Where security rules defined in the x-tyk-api-gateway extension take precedence.
  • compliant mode: Where the standard OAS security requirements are prioritized, providing better interoperability and adherence to the OpenAPI standard.

The new tests cover complex logical combinations of security schemes (e.g., (JWT AND HMAC) OR APIKey) and important edge cases, such as when the top-level OAS security definition is empty.

Key Technical Changes

The PR introduces 1009 lines of new test code within a single file, gateway/mw_auth_or_wrapper_test.go. The changes consist of six new test functions:

  • TestVendorExtension_MixedANDOR_LegacyMode
  • TestVendorExtension_MixedANDOR_CompliantMode
  • TestVendorExtension_ComplexCombination_LegacyMode
  • TestVendorExtension_ComplexCombination_CompliantMode
  • TestVendorExtension_EmptyOAS_LegacyMode
  • TestVendorExtension_EmptyOAS_CompliantMode

Each test function meticulously sets up a specific scenario by:

  1. Programmatically building an API definition (APISpec) with a corresponding OAS document.
  2. Configuring the securityProcessingMode and defining multiple security schemes (like API Key, JWT, HMAC, OAuth2) within the x-tyk-api-gateway extension.
  3. Creating necessary user policies and sessions (e.g., API keys, HMAC secrets).
  4. Executing a series of HTTP requests with various authentication headers.
  5. Asserting that the gateway returns the correct HTTP status codes (200 OK, 400 Bad Request, 401 Unauthorized, 403 Forbidden), thereby validating the enforcement logic.

Affected System Components

  • Authentication Middleware (mw_auth_or_wrapper): This is the component directly under test. The new tests verify its logic for handling different security configurations, ensuring it behaves as expected and preventing future regressions.
  • OAS Parser: The tests implicitly validate the gateway's ability to parse and interpret both the standard security section of an OAS file and the custom logic defined within the x-tyk-api-gateway extension.
  • API Loader: The tests rely on the BuildAndLoadAPI function, ensuring that programmatically defined APIs with complex security rules are correctly loaded and activated within the gateway.

Since this PR only adds test code, it has no direct impact on the production runtime but greatly improves the project's quality and maintainability.

2. Architecture Visualization

The changes in this PR test the conditional logic within the authentication middleware, which branches based on the securityProcessingMode. The following diagram illustrates this architectural flow.

graph TD
    A[Incoming Request] --> B{API uses OAS?};
    B -- No --> C[Standard Tyk Auth];
    B -- Yes --> D{"Read securityProcessingMode"};
    D -- "compliant" --> E[Compliant Mode];
    D -- "legacy (or not set)" --> F[Legacy Mode];

    subgraph Compliant Mode
        E --> G{"Is global security defined in OAS?"};
        G -- Yes --> H["Enforce security schemes from<br>standard OAS security section"];
        G -- No --> I["Fallback: Enforce security schemes from<br>x-tyk-api-gateway extension"];
    end

    subgraph Legacy Mode
        F --> J["Enforce security schemes from<br>x-tyk-api-gateway extension"];
    end

    H --> K{Request Authorized?};
    I --> K;
    J --> K;
    C --> K;

    K -- Yes --> L[Proxy to Upstream];
    K -- No --> M["Reject Request (401/403)"];
Loading

Diagram Explanation:

  1. When a request arrives, the gateway checks if the targeted API is defined by an OAS document.
  2. If it is, the securityProcessingMode field from the x-tyk-api-gateway extension is inspected.
  3. In compliant mode, the gateway first attempts to enforce the rules in the standard, top-level security section of the OAS document. The new tests confirm that if this section is empty or missing, the gateway correctly falls back to using the security rules defined within the x-tyk-api-gateway extension.
  4. In legacy mode, the gateway ignores the standard OAS security section and exclusively uses the rules defined within the x-tyk-api-gateway extension.
  5. Based on the evaluated rules, the request is either authorized and proxied to the upstream service or rejected.

The tests added in this PR are critical for validating both the Compliant Mode and Legacy Mode paths in this flow, ensuring the logic is robust and predictable for API developers.


Powered by Visor from Probelabs

Last updated: 2025-09-23T17:09:49.307Z | Triggered by: opened | Commit: 92f93dd

Copy link

probelabs bot commented Sep 23, 2025

🔍 Code Analysis Results

✅ Security Check Passed

No security issues found – changes LGTM.

Performance Issues (1)

Severity Location Issue
🟡 Warning gateway/mw_auth_or_wrapper.go:61-77
The `AuthORWrapper` middleware clones the HTTP request inside a loop for each security requirement it attempts to validate. In scenarios with multiple security schemes, if a frequently used credential is for a scheme ordered late in the list, this will result in unnecessary request cloning and processing for every request, increasing latency and resource consumption.
💡 SuggestionThe performance of this authentication logic is highly sensitive to the order of security schemes defined in the OpenAPI specification. To mitigate the overhead, advise users in the documentation to order their security requirements from most- to least-frequently used. This ensures that the most common authentication paths succeed on the first attempt, avoiding unnecessary work.

Quality Issues (2)

Severity Location Issue
🟢 Info gateway/mw_auth_or_wrapper_test.go:2577
The error returned from `ts.Gw.GlobalSessionManager.UpdateSession` is ignored. In a test environment, failing to check errors during the setup phase can lead to flaky tests that fail for reasons unrelated to the code being tested, making debugging more difficult.
💡 SuggestionCheck the error returned by `UpdateSession` and fail the test immediately if an error occurs. This ensures that the test setup is valid before proceeding with assertions. Replace `_ = ...` with `err := ...` and add `require.NoError(t, err)` or a similar check.
🟡 Warning gateway/mw_auth_or_wrapper_test.go:2535-3543
The added test functions exhibit significant code duplication, particularly in the setup of API specifications, policies, and user sessions. Large blocks of boilerplate for creating `APISpec`, `oas.OAS`, and `oas.XTykAPIGateway` are repeated across multiple tests (`TestVendorExtension_MixedANDOR_LegacyMode`, `TestVendorExtension_MixedANDOR_CompliantMode`, etc.), making the test suite difficult to read, maintain, and extend.
💡 SuggestionRefactor the common test setup logic into reusable helper functions. For instance, create a builder or factory function that accepts parameters for key configuration details (like `securityProcessingMode`, authentication types, etc.) and returns a fully configured `APISpec`. This will significantly reduce duplication, improve readability, and make the tests more focused on the specific behavior they are validating.

Style Issues (3)

Severity Location Issue
🟢 Info gateway/mw_auth_or_wrapper_test.go:2571
The pattern `func() *bool { b := true; return &b }()` is used to create a pointer to a boolean literal. While functionally correct, this idiom is verbose and reduces code readability when repeated.
💡 SuggestionTo improve clarity and reduce boilerplate, consider adding a small helper function at the package level, such as `func boolPtr(b bool) *bool { return &b }`. The call site would then become a much cleaner `Enabled: boolPtr(true)`. This pattern is repeated on lines 2750, 2970, and 3150.
🟡 Warning gateway/mw_auth_or_wrapper_test.go:2612-2620
This test case is a duplicate of the one defined at lines 2592-2600. Both cases test the exact same scenario: a request with only a JWT 'Authorization' header, expecting a 400 Bad Request.
💡 SuggestionRemove the redundant test case to keep the test suite concise and easier to maintain. Duplicate tests do not add to coverage and can increase maintenance overhead.
🟡 Warning gateway/mw_auth_or_wrapper_test.go:2632
The `signature` variable is calculated for HMAC authentication but is immediately discarded with `_ = signature`. The sub-test is named "JWT and HMAC together in legacy mode", but the actual request in the test case at line 2635 does not include the generated HMAC signature, making the test's name and intent misleading.
💡 SuggestionTo align the test's implementation with its description, either add the generated HMAC signature to the request headers to properly test the combined authentication scenario, or, if the intent is to test a missing HMAC signature, rename the test and add comments to clarify the purpose. The unused variable should be removed in either case.

Dependency Issues (1)

Severity Location Issue
🔴 Critical apidef/oas/authentication.go:90
The introduction of the `securityProcessingMode` field in the OAS API definition schema constitutes a breaking change for downstream projects, but no corresponding updates are referenced. This change impacts `tyk-operator`, `tyk-charts`, `portal`, and `tyk-sink`, which all rely on the API definition schema. Without updates, these tools will either fail to process the new field or strip it, leading to misconfigurations.
💡 SuggestionThis pull request should be blocked until pull requests for the affected downstream repositories (`tyk-operator`, `tyk-charts`, `portal`, `tyk-sink`) are created and linked. This ensures the new feature is supported across the entire Tyk ecosystem upon release.

✅ Connectivity Check Passed

No connectivity issues found – changes LGTM.


Powered by Visor from Probelabs

Last updated: 2025-09-23T17:09:50.101Z | Triggered by: opened | Commit: 92f93dd

Copy link

@mativm02 mativm02 changed the title Add tests for mixed AND/OR legacy mode and empty OAS compliant scenarios [TT-15671] Add tests for mixed AND/OR legacy mode and empty OAS compliant scenarios Sep 24, 2025
@mativm02 mativm02 closed this Sep 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants