Closed jeffy-mathew closed 1 day ago
buger
commented
3 days ago :broken_heart: The detected issue is not in one of the allowed statuses :broken_heart:
| Detected Status | Open | :x: |
|---|---|---|
| Allowed Statuses | In Dev,In Code Review,Ready for Testing,In Test,In Progress,In Review | :heavy_check_mark: |
Please ensure your jira story is in one of the allowed statuses
github-actions[bot]
commented
3 days ago API Changes
--- prev.txt 2024-10-25 14:26:59.613540649 +0000
+++ current.txt 2024-10-25 14:26:52.941483202 +0000
@@ -2416,9 +2416,9 @@
Username string `bson:"username" json:"username"`
// Password is the password to be used for upstream basic authentication.
Password string `bson:"password" json:"password"`
- // HeaderName is the custom header name to be used for upstream basic authentication.
+ // Header holds the configuration for custom header name to be used for upstream basic authentication.
// Defaults to `Authorization`.
- HeaderName string `bson:"header_name" json:"header_name"`
+ Header AuthSource `bson:"header" json:"header"`
}
UpstreamBasicAuth holds upstream basic authentication configuration.
@@ -5252,9 +5252,8 @@
type UpstreamBasicAuth struct {
// Enabled enables upstream basic authentication.
Enabled bool `bson:"enabled" json:"enabled"`
- // HeaderName is the custom header name to be used for upstream basic authentication.
- // Defaults to `Authorization`.
- HeaderName string `bson:"headerName" json:"headerName"`
+ // Header contains configurations for the header value.
+ Header *AuthSource `bson:"header,omitempty" json:"header,omitempty"`
// Username is the username to be used for upstream basic authentication.
Username string `bson:"username" json:"username"`
// Password is the password to be used for upstream basic authentication.
@@ -8184,6 +8183,87 @@
}
StreamsConfig represents a stream configuration.
+# Package: ./ee/middleware/upstreambasicauth
+
+package upstreambasicauth // import "github.com/TykTechnologies/tyk/ee/middleware/upstreambasicauth"
+
+
+CONSTANTS
+
+const (
+ // ExtensionTykStreaming is the OAS extension for Tyk streaming.
+ ExtensionTykStreaming = "x-tyk-streaming"
+ StreamGCInterval = 1 * time.Minute
+)
+
+TYPES
+
+type APISpec struct {
+ APIID string
+ Name string
+ IsOAS bool
+ OAS oas.OAS
+
+ UpstreamAuth apidef.UpstreamAuth
+}
+ APISpec is a subset of gateway.APISpec for the values the middleware
+ consumes.
+
+func NewAPISpec(id string, name string, isOasDef bool, oasDef oas.OAS, upstreamAuth apidef.UpstreamAuth) *APISpec
+ NewAPISpec creates a new APISpec object based on the required inputs.
+ The resulting object is a subset of `*gateway.APISpec`.
+
+type BaseMiddleware interface {
+ model.LoggerProvider
+}
+ BaseMiddleware is the subset of BaseMiddleware APIs that the middleware
+ uses.
+
+type Gateway interface {
+ model.ConfigProvider
+ model.ReplaceTykVariables
+}
+ Gateway is the subset of Gateway APIs that the middleware uses.
+
+type Middleware struct {
+ Spec *APISpec
+ Gw Gateway
+
+ // Has unexported fields.
+}
+ Middleware implements upstream basic auth middleware.
+
+func NewMiddleware(gw Gateway, mw BaseMiddleware, spec *APISpec) *Middleware
+ NewMiddleware returns a new instance of Middleware.
+
+func (m *Middleware) EnabledForSpec() bool
+ EnabledForSpec checks if streaming is enabled on the config.
+
+func (m *Middleware) Init()
+ Init initializes the middleware.
+
+func (m *Middleware) Logger() *logrus.Entry
+ Logger returns a logger with middleware filled out.
+
+func (m *Middleware) Name() string
+ Name returns the name for the middleware.
+
+func (m *Middleware) ProcessRequest(_ http.ResponseWriter, r *http.Request, _ interface{}) (error, int)
+ ProcessRequest will handle upstream basic auth.
+
+type Provider struct {
+ // Logger is the logger to be used.
+ Logger *logrus.Entry
+ // HeaderName is the header name to be used to fill upstream auth with.
+ HeaderName string
+ // AuthValue is the value of auth header.
+ AuthValue string
+}
+ Provider implements upstream auth provider.
+
+func (u Provider) Fill(r *http.Request)
+ Fill sets the request's HeaderName with AuthValue
+
# Package: ./gateway
package gateway // import "github.com/TykTechnologies/tyk/gateway"
@@ -11443,34 +11523,6 @@
Enums representing the various statuses for a VersionInfo Path match during
a proxy request
-type UpstreamBasicAuth struct {
- *BaseMiddleware
-}
- UpstreamBasicAuth is a middleware that will do basic authentication for
- upstream connections. UpstreamBasicAuth middleware is only supported in Tyk
- OAS API definitions.
-
-func (t *UpstreamBasicAuth) EnabledForSpec() bool
- EnabledForSpec returns true if the middleware is enabled based on API Spec.
-
-func (t *UpstreamBasicAuth) Name() string
- Name returns the name of middleware.
-
-func (t *UpstreamBasicAuth) ProcessRequest(_ http.ResponseWriter, r *http.Request, _ interface{}) (error, int)
- ProcessRequest will inject basic auth info into request context so that it
- can be used during reverse proxy.
-
-type UpstreamBasicAuthProvider struct {
- // HeaderName is the header name to be used to fill upstream auth with.
- HeaderName string
- // AuthValue is the value of auth header.
- AuthValue string
-}
- UpstreamBasicAuthProvider implements upstream auth provider.
-
-func (u UpstreamBasicAuthProvider) Fill(r *http.Request)
- Fill sets the request's HeaderName with AuthValue
-
type UpstreamOAuth struct {
*BaseMiddleware
}Here are some key observations to aid the review process:
| ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪ |
| 🧪 No relevant tests |
| 🔒 No security concerns identified |
| ⚡ Recommended focus areas for review Code Duplication The methods `IsEnabled` and `AuthKeyName` in the `AuthSource` struct seem to be duplicated across different files. Consider creating a shared package or file to define these methods in one place to adhere to the DRY principle. Error Handling The `Fill` and `ExtractTo` methods in `UpstreamBasicAuth` struct do not handle potential nil pointer dereferences when accessing properties of `api.Header`. It's important to add nil checks before accessing these properties to prevent runtime panics. Logging Level The use of `Info` level for logging potential sensitive operations such as overwriting headers in `Provider.Fill` method might not be appropriate. Consider using `Warning` or `Error` level to highlight the importance of this action. |
github-actions[bot]
commented
3 days ago Explore these optional code suggestions:
| Category | Suggestion | Score |
| Possible bug |
Prevent potential nil pointer dereference by checking if
___
**Consider checking if | 8 |
Add null check for
___
**Ensure that | 8 | |
| Enhancement |
Prevent setting empty authentication headers___ **Add a check to ensureAuthValue is not empty before setting the header, to avoid setting empty authentication headers.** [ee/middleware/upstreambasicauth/provider.go [25]](https://github.com/TykTechnologies/tyk/pull/6669/files#diff-16e7043ed331108c72617c42eb50870a76523593a477bebb197fae04c22943a4R25-R25) ```diff -r.Header.Set(u.HeaderName, u.AuthValue) +if u.AuthValue != "" { + r.Header.Set(u.HeaderName, u.AuthValue) +} ``` Suggestion importance[1-10]: 7Why: The suggestion enhances the code by ensuring that empty authentication headers are not set, which can prevent potential security issues or misconfigurations. | 7 |
Improve error handling by logging when
___
**Log an error message when | 6 |
sonarcloud[bot]
commented
3 days ago 
Quality Gate failedFailed conditions
0.0% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)
See analysis details on SonarCloud
Catch issues before they fail your Quality Gate with our IDE extension 
SonarLint
User description
Description
This PR moves upstream basic auth implementations to ee package
Related Issue
Parent: https://tyktech.atlassian.net/browse/TT-13359 Subtask: https://tyktech.atlassian.net/browse/TT-13389
Motivation and Context
How This Has Been Tested
Screenshots (if appropriate)
Types of changes
Checklist
PR Type
Enhancement, Other
Description
UpstreamBasicAuthto use a newAuthSourcestruct for better configuration management.Changes walkthrough 📝
8 files
api_definitions.go
Refactor UpstreamBasicAuth to use AuthSource structapidef/api_definitions.go
HeaderNametoHeaderinUpstreamBasicAuth.AuthSourcestruct for auth configurations.IsEnabledandAuthKeyNametoAuthSource.upstream.go
Update UpstreamBasicAuth to use AuthSource in OASapidef/oas/upstream.go
HeaderNametoHeaderinUpstreamBasicAuth.FillandExtractTomethods to handleAuthSource.middleware.go
Implement Upstream Basic Auth Middlewareee/middleware/upstreambasicauth/middleware.go
model.go
Define Middleware Model and APISpec Structuresee/middleware/upstreambasicauth/model.go
APISpecstruct for middleware configuration.provider.go
Implement Provider for Upstream Authenticationee/middleware/upstreambasicauth/provider.go
Providerfor upstream authentication.api_loader.go
Integrate Upstream Basic Auth Middleware in API Loadergateway/api_loader.go - Integrated new upstream basic auth middleware into API processing.
mw_upstream_basic_auth.go
Replace UpstreamBasicAuth with Noop for Non-EE Buildsgateway/mw_upstream_basic_auth.go
UpstreamBasicAuthwithnoopUpstreamBasicAuth.mw_upstream_basic_auth_ee.go
Add EE-Specific Upstream Basic Auth Middlewaregateway/mw_upstream_basic_auth_ee.go
1 files
mw_upstream_basic_auth_test.go
Update Test Build Constraints for EE Middlewaregateway/mw_upstream_basic_auth_test.go - Added build constraints for EE and dev environments.