feat: Add Enterprise-Grade SPNEGO Authentication Middleware #1368
Conversation
jarod2011
requested review from
ReneWerner87,
efectn,
gaby and
sixcolors
and removed request for
a team
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new SPNEGO Kerberos authentication middleware for Fiber v2 and v3, plus configuration, identity helpers, adapters, keytab utilities, mock keytab builders, bilingual READMEs, examples, and comprehensive tests for middleware and utilities. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Fiber as Fiber App (v2/v3)
participant MW as SPNEGO Middleware
participant Lookup as KeytabLookupFunc
participant SPNEGO as SPNEGO KRB5 Handler
participant Handler as Protected Route
Client->>Fiber: HTTP request /protected/resource
Fiber->>MW: Invoke middleware
MW->>Lookup: Obtain keytab
alt Lookup fails
MW-->>Fiber: 500 Internal Server Error
Fiber-->>Client: 500 + error body
else Lookup succeeds
MW->>SPNEGO: ServeHTTP(adapted ctx, http.Request)
alt No/invalid Authorization
SPNEGO-->>Client: 401 + WWW-Authenticate: Negotiate
else Valid ticket
SPNEGO->>MW: Identity in http.Request.Context
MW->>MW: Set identity to Fiber Locals
MW->>Fiber: Next()
Fiber->>Handler: Call route handler
Handler-->>Client: 200 OK (app response)
end
end
sequenceDiagram
participant Route as Route Handler
participant Ctx as Fiber Context
Note over Route,Ctx: Downstream identity access
Route->>Ctx: GetAuthenticatedIdentityFromContext(c)
alt Found
Route-->>Client: 200 "Hello, <user>!"
else Not found
Route-->>Client: 401 Unauthorized
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related issues
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @jarod2011, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I'm introducing a new SPNEGO authentication middleware for Fiber, which provides Kerberos-based single sign-on (SSO) capabilities. This middleware is designed to be enterprise-grade, supporting dynamic keytab loading and adhering to relevant RFCs, making it compatible with both Fiber v2 and v3.
Highlights
- SPNEGO Authentication Middleware: I've implemented a new SPNEGO authentication middleware for Fiber applications.
- Fiber Version Compatibility: The middleware supports both Fiber v2 and Fiber v3 versions, ensuring broad compatibility.
- Kerberos-based Single Sign-On (SSO): This feature enables Kerberos-based Single Sign-On (SSO) capabilities for Fiber applications.
- Dynamic Keytab Loading: A flexible keytab lookup system has been designed, allowing dynamic retrieval of keytabs from various sources like files, databases, or remote services.
- RFC Compliance: The implementation ensures full compliance with relevant RFCs, specifically RFC 4559, RFC 4120, and RFC 7235, for robust and standard-compliant authentication.
- Comprehensive Test Coverage: The new middleware comes with comprehensive test coverage, exceeding 90%, to ensure reliability and proper handling of edge cases and errors.
- Detailed Documentation: Complete documentation, including usage examples for both Fiber v2 and v3, has been provided in the README to facilitate easy adoption.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a new SPNEGO authentication middleware for Fiber, with support for both v2 and v3. The implementation is well-structured, separating configuration from the version-specific middleware logic. However, there are several critical issues that need to be addressed. The response writer wrapper (wrapCtx) in both v2 and v3 implementations is flawed, which will prevent the SPNEGO authentication challenge from working correctly. Additionally, there are uncompilable helper functions in the test files, and the provided runnable example will panic due to missing files. The documentation in the README also contains incorrect code snippets. Addressing these issues will significantly improve the quality and usability of this new middleware.
spnego/v2/spnego.go
Outdated
| // wrapCtx wraps a Fiber context pointer to implement the http.ResponseWriter interface. | ||
| // This adapter allows the Fiber context to be used with standard HTTP handlers | ||
| // that expect an http.ResponseWriter, bridging the gap between Fiber's context | ||
| // model and the standard library's HTTP interfaces. | ||
|
|
||
| type wrapCtx struct { | ||
| *fiber.Ctx | ||
| } | ||
|
|
||
| // Header returns the request headers from the wrapped Fiber context. | ||
| // This method implements the http.ResponseWriter interface. | ||
| func (w wrapCtx) Header() http.Header { | ||
| return w.Ctx.GetReqHeaders() | ||
| } | ||
|
|
||
| // WriteHeader sets the HTTP status code on the wrapped Fiber context. | ||
| // This method implements the http.ResponseWriter interface. | ||
| func (w wrapCtx) WriteHeader(statusCode int) { | ||
| w.Ctx.Status(statusCode) | ||
| } |
There was a problem hiding this comment.
The wrapCtx implementation of http.ResponseWriter is incorrect. The Header() method returns request headers (w.Ctx.GetReqHeaders()) instead of providing access to response headers. This is a critical bug that will prevent the SPNEGO authentication challenge from working. The gokrb5 library will attempt to set the WWW-Authenticate header on the request headers, which will be ignored, and the client will never receive the required challenge.
You need to provide a proper http.ResponseWriter implementation that correctly proxies header modifications and status code writes to the Fiber context's response.
spnego/v2/spnego_test.go
Outdated
| func newBadKeytabTempFile(principal string, realm string, et ...int32) (filename string, clean func(), err error) { | ||
| filename = fmt.Sprintf("./tmp_%d.keytab", time.Now().Unix()) | ||
| clean = func() { | ||
| os.Remove(filename) | ||
| } | ||
| var kt keytab.Keytab | ||
| for _, e := range et { | ||
| kt.AddEntry(principal, realm, "abcdefg", time.Now(), 2, e) | ||
| } |
There was a problem hiding this comment.
This test helper function will not compile. var kt keytab.Keytab declares a value, but the AddEntry method is defined on the pointer type *keytab.Keytab.
To fix this, you should initialize kt using keytab.New(), which returns a *keytab.Keytab, similar to how it's done in the newKeytabTempFile function.
func newBadKeytabTempFile(principal string, realm string, et ...int32) (filename string, clean func(), err error) {
filename = fmt.Sprintf("./tmp_%d.keytab", time.Now().Unix())
clean = func() {
os.Remove(filename)
}
kt := keytab.New()
// Set a bad version number to cause a load failure
kt.Vno = 0
for _, e := range et {
kt.AddEntry(principal, realm, "abcdefg", time.Now(), 2, e)
}
spnego/v3/spnego.go
Outdated
| // wrapCtx wraps a Fiber context to implement the http.ResponseWriter interface. | ||
| // This adapter allows the Fiber context to be used with standard HTTP handlers | ||
| // that expect an http.ResponseWriter, bridging the gap between Fiber's context | ||
| // model and the standard library's HTTP interfaces. | ||
|
|
||
| type wrapCtx struct { | ||
| fiber.Ctx | ||
| } | ||
|
|
||
| // Header returns the request headers from the wrapped Fiber context. | ||
| // This method implements the http.ResponseWriter interface. | ||
| func (w wrapCtx) Header() http.Header { | ||
| return w.Ctx.GetReqHeaders() | ||
| } | ||
|
|
||
| // WriteHeader sets the HTTP status code on the wrapped Fiber context. | ||
| // This method implements the http.ResponseWriter interface. | ||
| func (w wrapCtx) WriteHeader(statusCode int) { | ||
| w.Ctx.Status(statusCode) | ||
| } |
There was a problem hiding this comment.
The wrapCtx implementation of http.ResponseWriter is incorrect. The Header() method returns request headers (w.Ctx.GetReqHeaders()) instead of providing access to response headers. This is a critical bug that will prevent the SPNEGO authentication challenge from working. The gokrb5 library will attempt to set the WWW-Authenticate header on the request headers, which will be ignored, and the client will never receive the required challenge.
You need to provide a proper http.ResponseWriter implementation that correctly proxies header modifications and status code writes to the Fiber context's response.
spnego/v3/spnego_test.go
Outdated
| func newBadKeytabTempFile(principal string, realm string, et ...int32) (filename string, clean func(), err error) { | ||
| filename = fmt.Sprintf("./tmp_%d.keytab", time.Now().Unix()) | ||
| clean = func() { | ||
| os.Remove(filename) | ||
| } | ||
| var kt keytab.Keytab | ||
| for _, e := range et { | ||
| kt.AddEntry(principal, realm, "abcdefg", time.Now(), 2, e) | ||
| } |
There was a problem hiding this comment.
This test helper function will not compile. var kt keytab.Keytab declares a value, but the AddEntry method is defined on the pointer type *keytab.Keytab.
To fix this, you should initialize kt using keytab.New(), which returns a *keytab.Keytab, similar to how it's done in the newKeytabTempFile function.
func newBadKeytabTempFile(principal string, realm string, et ...int32) (filename string, clean func(), err error) {
filename = fmt.Sprintf("./tmp_%d.keytab", time.Now().Unix())
clean = func() {
os.Remove(filename)
}
kt := keytab.New()
// Set a bad version number to cause a load failure
kt.Vno = 0
for _, e := range et {
kt.AddEntry(principal, realm, "abcdefg", time.Now(), 2, e)
}| func ExampleNewSpnegoKrb5AuthenticateMiddleware() { | ||
| app := fiber.New() | ||
| keytabLookup, err := config.NewKeytabFileLookupFunc("/keytabFile/one.keytab", "/keytabFile/two.keyta") | ||
| if err != nil { | ||
| panic(fmt.Errorf("create keytab lookup function failed: %w", err)) | ||
| } | ||
| authMiddleware, err := v3.NewSpnegoKrb5AuthenticateMiddleware(&config.Config{ | ||
| KeytabLookup: keytabLookup, | ||
| }) | ||
| if err != nil { | ||
| panic(fmt.Errorf("create spnego middleware failed: %w", err)) | ||
| } |
There was a problem hiding this comment.
This example function will cause a panic when run with go test because it attempts to load keytab files from hardcoded paths that do not exist. Example functions should be self-contained and runnable.
Consider creating a mock KeytabLookupFunc or using temporary files for the keytab data to make this example pass testing and be more useful for users.
Additionally, app.Listen("sso.example.local:3000") may fail on systems where this hostname is not resolved. Using localhost:3000 or :3000 would be more robust for an example.
spnego/README.md
Outdated
| cfg := &spnego.Config{ | ||
| // Use a function to look up keytab from files | ||
| KeytabLookup: func() (*keytab.Keytab, error) { | ||
| // Implement your keytab lookup logic here | ||
| // This could be from files, database, or other sources | ||
| kt, err := spnego.NewKeytabFileLookupFunc("/path/to/keytab/file.keytab") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return kt() | ||
| }, | ||
| // Optional: Set a custom logger | ||
| Log: flog.DefaultLogger().Logger().(*log.Logger), | ||
| } | ||
|
|
||
| // Create the middleware | ||
| authMiddleware, err := v3.NewSpnegoKrb5AuthenticateMiddleware(cfg) | ||
| if err != nil { | ||
| flog.Fatalf("Failed to create middleware: %v", err) | ||
| } | ||
|
|
||
| // Apply the middleware to protected routes | ||
| app.Use("/protected", authMiddleware) | ||
|
|
||
| // Access authenticated identity | ||
| app.Get("/protected/resource", func(c fiber.Ctx) error { | ||
| identity, ok := v3.GetAuthenticatedIdentityFromContext(c) | ||
| if !ok { | ||
| return c.Status(fiber.StatusUnauthorized).SendString("Unauthorized") | ||
| } | ||
| return c.SendString(fmt.Sprintf("Hello, %s!", identity.UserName())) | ||
| }) | ||
|
|
||
| app.Listen(":3000") | ||
| } | ||
| ``` | ||
|
|
||
| ### For Fiber v2 | ||
|
|
||
| ```go | ||
| package main | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "log" | ||
| "os" | ||
|
|
||
| "github.com/jcmturner/gokrb5/v8/keytab" | ||
| "github.com/gofiber/fiber/v2" | ||
| "github.com/gofiber/contrib/spnego/v2" | ||
| ) | ||
|
|
||
| func main() { | ||
| app := fiber.New() | ||
|
|
||
| // Create a configuration with a keytab lookup function | ||
| cfg := &spnego.Config{ | ||
| // Use a function to look up keytab from files | ||
| KeytabLookup: func() (*keytab.Keytab, error) { | ||
| // Implement your keytab lookup logic here | ||
| // This could be from files, database, or other sources | ||
| kt, err := spnego.NewKeytabFileLookupFunc("/path/to/keytab/file.keytab") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return kt() |
There was a problem hiding this comment.
The code examples for both Fiber v3 and v2 are incorrect and will not compile as-is. They reference types and functions from an unimported spnego package alias (e.g., spnego.Config, spnego.NewKeytabFileLookupFunc).
To fix this, you need to:
- Add
"github.com/gofiber/contrib/spnego/config"to the import block. - Use
config.Configinstead ofspnego.Config. - Use
config.NewKeytabFileLookupFuncinstead ofspnego.NewKeytabFileLookupFunc.
The doc.go file in this same PR contains a correct example that you can use as a reference.
spnego/README.zh-CN.md
Outdated
| cfg := &v3.Config{ | ||
| // 使用函数从文件查找keytab | ||
| KeytabLookup: func() (*keytab.Keytab, error) { | ||
| // 在此实现您的keytab查找逻辑 | ||
| // 可以从文件、数据库或其他来源获取 | ||
| kt, err := v3.NewKeytabFileLookupFunc("/path/to/keytab/file.keytab") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return kt() | ||
| }, | ||
| // 可选：设置自定义日志器 | ||
| Log: flog.DefaultLogger().Logger().(*log.Logger), | ||
| } | ||
|
|
||
| // 创建中间件 | ||
| authMiddleware, err := v3.NewSpnegoKrb5AuthenticateMiddleware(cfg) | ||
| if err != nil { | ||
| flog.Fatalf("创建中间件失败: %v", err) | ||
| } | ||
|
|
||
| // 将中间件应用于受保护的路由 | ||
| app.Use("/protected", authMiddleware) | ||
|
|
||
| // 访问认证身份 | ||
| app.Get("/protected/resource", func(c fiber.Ctx) error { | ||
| identity, ok := v3.GetAuthenticatedIdentityFromContext(c) | ||
| if !ok { | ||
| return c.Status(fiber.StatusUnauthorized).SendString("未授权") | ||
| } | ||
| return c.SendString(fmt.Sprintf("你好, %s!", identity.UserName())) | ||
| }) | ||
|
|
||
| app.Listen(":3000") | ||
| } | ||
| ``` | ||
|
|
||
| ### 对于Fiber v2 | ||
|
|
||
| ```go | ||
| package main | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "log" | ||
| "os" | ||
|
|
||
| "github.com/jcmturner/gokrb5/v8/keytab" | ||
| "github.com/gofiber/fiber/v2" | ||
| "github.com/gofiber/contrib/spnego/v2" | ||
| ) | ||
|
|
||
| func main() { | ||
| app := fiber.New() | ||
|
|
||
| // 创建带有keytab查找函数的配置 | ||
| cfg := &v2.Config{ | ||
| // 使用函数从文件查找keytab | ||
| KeytabLookup: func() (*keytab.Keytab, error) { | ||
| // 在此实现您的keytab查找逻辑 | ||
| // 可以从文件、数据库或其他来源获取 | ||
| kt, err := v2.NewKeytabFileLookupFunc("/path/to/keytab/file.keytab") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return kt() |
There was a problem hiding this comment.
The code examples for both Fiber v3 and v2 are incorrect. They refer to v3.Config and v2.Config, but the Config struct is defined in the config package. The code should import github.com/gofiber/contrib/spnego/config and use config.Config and config.NewKeytabFileLookupFunc.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (4)
spnego/example.go (2)
36-46: Consider graceful shutdown approach.Using
time.Afterfor server shutdown timing is not ideal for production examples, even though this is acceptable for demonstration purposes.Consider showing a more realistic shutdown pattern:
log.Info("Server is running on :3000") - go func() { - <-time.After(time.Second * 1) - fmt.Println("use curl -kv --negotiate http://sso.example.local:3000/protected/resource") - fmt.Println("if response is 401, execute `klist` to check use kerberos session") - <-time.After(time.Second * 2) - fmt.Println("close server") - if err = app.Shutdown(); err != nil { - panic(fmt.Errorf("shutdown server failed: %w", err)) - } - }() + fmt.Println("use curl -kv --negotiate http://sso.example.local:3000/protected/resource") + fmt.Println("if response is 401, execute `klist` to check use kerberos session")
47-49: Verify server binding address.The example binds to
sso.example.local:3000which requires DNS resolution. Consider usinglocalhost:3000or0.0.0.0:3000for broader compatibility in examples.- if err := app.Listen("sso.example.local:3000"); err != nil { + if err := app.Listen(":3000"); err != nil {spnego/v2/spnego.go (1)
34-36: Consider using a more robust logger initializationThe comment indicates difficulty obtaining Fiber's internal logger. Consider providing a configuration option for users to pass their preferred logger instance.
This would allow users to maintain consistent logging across their application rather than having a separate logger instance for the middleware.
spnego/v3/spnego.go (1)
1-104: Consider extracting common SPNEGO logicBoth v2 and v3 implementations share significant logic. While API differences between Fiber versions require separate implementations, common patterns could be extracted.
Consider creating a shared internal package for:
- Common error wrapping patterns
- Identity management utilities
- SPNEGO handler creation logic
This would reduce code duplication and make maintenance easier while still allowing version-specific adaptations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spnego/go.modis excluded by!**/*.modspnego/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (9)
spnego/README.md(1 hunks)spnego/README.zh-CN.md(1 hunks)spnego/config/config.go(1 hunks)spnego/doc.go(1 hunks)spnego/example.go(1 hunks)spnego/v2/spnego.go(1 hunks)spnego/v2/spnego_test.go(1 hunks)spnego/v3/spnego.go(1 hunks)spnego/v3/spnego_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: ReneWerner87
PR: gofiber/recipes#0
File: :0-0
Timestamp: 2024-11-26T20:05:15.793Z
Learning: For future contributions to the `gofiber/recipes` repository, ensure that the tasks outlined in `.github/CONTRIBUTING.md` are incorporated, including creating a new directory without a "fiber" prefix, adding a `README.md` with Docusaurus metadata, and updating the overview by running `make generate`.
Learnt from: ReneWerner87
PR: gofiber/contrib#0
File: :0-0
Timestamp: 2024-10-16T10:04:06.328Z
Learning: The i18n functionality in the gofiber/contrib repository is being refactored from middleware to a global container to improve robustness and performance. The global container will be initialized once before setting up routes and will manage the i18n bundle and localizer map.
Learnt from: ReneWerner87
PR: gofiber/fiber#0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Feature request #3224 has been created to add support for square bracket notation and comma-separated values in multipart form data in Fiber, while maintaining binary data transfer capabilities. This would bring parity with the existing form-urlencoded functionality.
Learnt from: ReneWerner87
PR: gofiber/contrib#0
File: :0-0
Timestamp: 2024-07-03T11:59:00.303Z
Learning: The i18n functionality in the gofiber/contrib repository is being refactored from middleware to a global container to improve robustness and performance. The global container will be initialized once before setting up routes and will manage the i18n bundle and localizer map.
📚 Learning: in the `clean-code` example at `clean-code/app/server/server.go` using the go fiber framework, it's ...
Learnt from: norri
PR: gofiber/recipes#2701
File: clean-code/app/server/server.go:14-16
Timestamp: 2024-11-23T19:50:41.765Z
Learning: In the `clean-code` example at `clean-code/app/server/server.go` using the Go Fiber framework, it's acceptable to omit production-level features like context usage and graceful shutdown handling to keep the example simple.
Applied to files:
spnego/doc.gospnego/example.gospnego/README.md
📚 Learning: the storage package in gofiber/storage repository can be used independently of the fiber web framewo...
Learnt from: ReneWerner87
PR: gofiber/storage#0
File: :0-0
Timestamp: 2025-02-12T11:24:31.153Z
Learning: The storage package in gofiber/storage repository can be used independently of the Fiber web framework.
Applied to files:
spnego/doc.go
📚 Learning: for future contributions to the `gofiber/recipes` repository, ensure that the tasks outlined in `.gi...
Learnt from: ReneWerner87
PR: gofiber/recipes#0
File: :0-0
Timestamp: 2024-11-26T20:05:15.793Z
Learning: For future contributions to the `gofiber/recipes` repository, ensure that the tasks outlined in `.github/CONTRIBUTING.md` are incorporated, including creating a new directory without a "fiber" prefix, adding a `README.md` with Docusaurus metadata, and updating the overview by running `make generate`.
Applied to files:
spnego/README.zh-CN.mdspnego/README.md
📚 Learning: the i18n functionality in the gofiber/contrib repository is being refactored from middleware to a gl...
Learnt from: ReneWerner87
PR: gofiber/contrib#0
File: :0-0
Timestamp: 2024-10-16T10:04:06.328Z
Learning: The i18n functionality in the gofiber/contrib repository is being refactored from middleware to a global container to improve robustness and performance. The global container will be initialized once before setting up routes and will manage the i18n bundle and localizer map.
Applied to files:
spnego/README.zh-CN.md
📚 Learning: the `encryptcookie_test.go` file contains unit tests that validate key lengths for both `encryptcook...
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:22-25
Timestamp: 2024-07-02T13:29:56.992Z
Learning: The `encryptcookie_test.go` file contains unit tests that validate key lengths for both `EncryptCookie` and `DecryptCookie` functions, ensuring that invalid key lengths raise appropriate errors.
Applied to files:
spnego/v3/spnego_test.gospnego/v2/spnego_test.go
📚 Learning: parallel tests for `session.save` already exist in the `middleware/session/session_test.go` file, sp...
Learnt from: sixcolors
PR: gofiber/fiber#3051
File: middleware/session/session.go:215-216
Timestamp: 2024-06-30T00:38:06.580Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Applied to files:
spnego/v3/spnego_test.gospnego/v2/spnego_test.go
📚 Learning: in go test files, prefer using the `require` methods from the `testify` package for assertions inste...
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
spnego/v3/spnego_test.gospnego/v2/spnego_test.go
📚 Learning: in the `test_session_next` function in `middleware/session/middleware_test.go`, the variable `donext...
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/middleware_test.go:400-407
Timestamp: 2024-09-25T17:09:03.756Z
Learning: In the `Test_Session_Next` function in `middleware/session/middleware_test.go`, the variable `doNext` is properly synchronized with the `muNext` mutex for both read and write access.
Applied to files:
spnego/v3/spnego_test.gospnego/v2/spnego_test.go
📚 Learning: unit tests for key length enforcement in both `encryptcookie` and `decryptcookie` functions have bee...
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.
Applied to files:
spnego/v3/spnego_test.gospnego/v2/spnego_test.go
📚 Learning: the project uses the testify/assert package for assertions in unit tests....
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
spnego/v3/spnego_test.gospnego/v2/spnego_test.go
📚 Learning: unit tests in this project use testify require....
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Applied to files:
spnego/v3/spnego_test.gospnego/v2/spnego_test.go
📚 Learning: in the fiber framework tests, using `ctx.response.header.cookie` may not be suitable for parsing coo...
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Applied to files:
spnego/v3/spnego_test.gospnego/v2/spnego_test.go
📚 Learning: in the fiber framework, breaking changes are acceptable when moving from version 2 to version 3, inc...
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
spnego/README.mdspnego/v3/spnego.gospnego/v2/spnego.go
📚 Learning: in the `middleware/adaptor/adaptor.go` file of the fiber framework, when updating context handling, ...
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
spnego/v3/spnego.gospnego/v2/spnego.go
🧬 Code Graph Analysis (1)
spnego/v3/spnego.go (2)
spnego/v2/spnego.go (2)
NewSpnegoKrb5AuthenticateMiddleware(25-62)GetAuthenticatedIdentityFromContext(81-84)spnego/config/config.go (5)
Config(37-42)ErrConfigInvalidOfKeytabLookupFunctionRequired(12-12)ErrLookupKeytabFailed(15-15)ErrConvertRequestFailed(18-18)ContextKeyOfIdentity(27-27)
🔇 Additional comments (5)
spnego/config/config.go (2)
11-24: LGTM! Well-defined error variables.The error variables are clearly named and provide specific context for different failure scenarios. The grouping of related errors using
var()block is appropriate.
29-33: Excellent design for extensibility.The
KeytabLookupFunctype provides a clean abstraction that enables dynamic keytab retrieval from various sources. The documentation clearly explains the extensibility benefits.spnego/doc.go (1)
7-51: LGTM! Comprehensive package documentation.The documentation provides excellent coverage of version compatibility, usage examples, and API overview. The example code demonstrates proper error handling and middleware usage patterns.
spnego/v2/spnego_test.go (1)
145-162: LGTM!The test properly validates the context-based identity storage and retrieval functionality.
spnego/v2/spnego.go (1)
97-99: Verify response header retrieval in Header() implementationThe
http.ResponseWriter.Header()method must return response headers, but the current code returns request headers viaw.Ctx.GetReqHeaders().
- Confirm whether Fiber v2’s
Ctxinterface offers a response-header method (for example,Response().Header()).- If it does, replace
GetReqHeaders()with the correct call so thatHeader()returns the response headers as required byhttp.ResponseWriter.
spnego/config/config.go
Outdated
| package config | ||
|
|
There was a problem hiding this comment.
Duplicate comment line detected.
Line 2 duplicates line 1's package comment. Remove the duplicate.
// Package spnego provides SPNEGO (Simple and Protected GSSAPI Negotiation Mechanism)
-// Package spnego provides SPNEGO (Simple and Protected GSSAPI Negotiation Mechanism)
// authentication middleware for Fiber applications. It enables Kerberos authenticationCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In spnego/config/config.go at lines 1 to 2, there is a duplicate package comment
line. Remove the second line to eliminate the duplicate comment and keep only
one package declaration line.
spnego/config/config.go
Outdated
| var mergeKeytab keytab.Keytab | ||
| for _, keytabFile := range keytabFiles { | ||
| kt, err := keytab.Load(keytabFile) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("%w: file %s load failed: %w", ErrLoadKeytabFileFailed, keytabFile, err) | ||
| } | ||
| mergeKeytab.Entries = append(mergeKeytab.Entries, kt.Entries...) | ||
| } | ||
| return func() (*keytab.Keytab, error) { | ||
| return &mergeKeytab, nil | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential memory issue with keytab merging.
The current implementation creates a single merged keytab that contains all entries from multiple files. This could lead to memory issues with large keytab files and doesn't handle potential duplicate entries.
Consider these improvements:
func NewKeytabFileLookupFunc(keytabFiles ...string) (KeytabLookupFunc, error) {
if len(keytabFiles) == 0 {
return nil, ErrConfigInvalidOfAtLeastOneKeytabFileRequired
}
- var mergeKeytab keytab.Keytab
- for _, keytabFile := range keytabFiles {
- kt, err := keytab.Load(keytabFile)
- if err != nil {
- return nil, fmt.Errorf("%w: file %s load failed: %w", ErrLoadKeytabFileFailed, keytabFile, err)
- }
- mergeKeytab.Entries = append(mergeKeytab.Entries, kt.Entries...)
- }
return func() (*keytab.Keytab, error) {
- return &mergeKeytab, nil
+ var mergeKeytab keytab.Keytab
+ for _, keytabFile := range keytabFiles {
+ kt, err := keytab.Load(keytabFile)
+ if err != nil {
+ return nil, fmt.Errorf("%w: file %s load failed: %w", ErrLoadKeytabFileFailed, keytabFile, err)
+ }
+ mergeKeytab.Entries = append(mergeKeytab.Entries, kt.Entries...)
+ }
+ return &mergeKeytab, nil
}, nil
}This approach:
- Loads keytabs on-demand rather than pre-loading
- Provides fresher keytab data if files are updated
- Reduces memory footprint during initialization
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var mergeKeytab keytab.Keytab | |
| for _, keytabFile := range keytabFiles { | |
| kt, err := keytab.Load(keytabFile) | |
| if err != nil { | |
| return nil, fmt.Errorf("%w: file %s load failed: %w", ErrLoadKeytabFileFailed, keytabFile, err) | |
| } | |
| mergeKeytab.Entries = append(mergeKeytab.Entries, kt.Entries...) | |
| } | |
| return func() (*keytab.Keytab, error) { | |
| return &mergeKeytab, nil | |
| }, nil | |
| } | |
| func NewKeytabFileLookupFunc(keytabFiles ...string) (KeytabLookupFunc, error) { | |
| if len(keytabFiles) == 0 { | |
| return nil, ErrConfigInvalidOfAtLeastOneKeytabFileRequired | |
| } | |
| return func() (*keytab.Keytab, error) { | |
| var mergeKeytab keytab.Keytab | |
| for _, keytabFile := range keytabFiles { | |
| kt, err := keytab.Load(keytabFile) | |
| if err != nil { | |
| return nil, fmt.Errorf("%w: file %s load failed: %w", ErrLoadKeytabFileFailed, keytabFile, err) | |
| } | |
| mergeKeytab.Entries = append(mergeKeytab.Entries, kt.Entries...) | |
| } | |
| return &mergeKeytab, nil | |
| }, nil | |
| } |
🤖 Prompt for AI Agents
In spnego/config/config.go around lines 50 to 61, the current code merges all
keytab entries into a single in-memory keytab, which can cause high memory usage
and does not handle duplicates. To fix this, refactor the function to load each
keytab file on-demand instead of preloading and merging all entries upfront.
Implement a mechanism that returns a function which loads and merges keytab
entries fresh each time it is called, ensuring updated data and lower memory
footprint during initialization.
spnego/example.go
Outdated
|
|
||
| func ExampleNewSpnegoKrb5AuthenticateMiddleware() { | ||
| app := fiber.New() | ||
| keytabLookup, err := config.NewKeytabFileLookupFunc("/keytabFile/one.keytab", "/keytabFile/two.keyta") |
There was a problem hiding this comment.
Fix typo in keytab file extension.
The second keytab file has a typo: .keyta should be .keytab.
- keytabLookup, err := config.NewKeytabFileLookupFunc("/keytabFile/one.keytab", "/keytabFile/two.keyta")
+ keytabLookup, err := config.NewKeytabFileLookupFunc("/keytabFile/one.keytab", "/keytabFile/two.keytab")🤖 Prompt for AI Agents
In spnego/example.go at line 15, there is a typo in the second keytab file
extension where ".keyta" should be corrected to ".keytab". Update the string
"/keytabFile/two.keyta" to "/keytabFile/two.keytab" to fix the file extension.
spnego/README.md
Outdated
| cfg := &spnego.Config{ | ||
| // Use a function to look up keytab from files | ||
| KeytabLookup: func() (*keytab.Keytab, error) { | ||
| // Implement your keytab lookup logic here | ||
| // This could be from files, database, or other sources | ||
| kt, err := spnego.NewKeytabFileLookupFunc("/path/to/keytab/file.keytab") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return kt() | ||
| }, | ||
| // Optional: Set a custom logger | ||
| Log: flog.DefaultLogger().Logger().(*log.Logger), | ||
| } | ||
|
|
There was a problem hiding this comment.
Inconsistent API usage in Fiber v3 example.
The example shows inconsistent usage of the Config struct and API. It references spnego.Config and spnego.NewKeytabFileLookupFunc but should use the config package.
// Create a configuration with a keytab lookup function
- cfg := &spnego.Config{
+ cfg := &config.Config{
// Use a function to look up keytab from files
KeytabLookup: func() (*keytab.Keytab, error) {
// Implement your keytab lookup logic here
// This could be from files, database, or other sources
- kt, err := spnego.NewKeytabFileLookupFunc("/path/to/keytab/file.keytab")
+ kt, err := config.NewKeytabFileLookupFunc("/path/to/keytab/file.keytab")
if err != nil {
return nil, err
}
return kt()
},Also add the missing import:
import (
flog "github.com/gofiber/fiber/v3/log"
"fmt"
+ "log"
"github.com/jcmturner/gokrb5/v8/keytab"
"github.com/gofiber/fiber/v3"
+ "github.com/gofiber/contrib/spnego/config"
"github.com/gofiber/contrib/spnego/v3"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cfg := &spnego.Config{ | |
| // Use a function to look up keytab from files | |
| KeytabLookup: func() (*keytab.Keytab, error) { | |
| // Implement your keytab lookup logic here | |
| // This could be from files, database, or other sources | |
| kt, err := spnego.NewKeytabFileLookupFunc("/path/to/keytab/file.keytab") | |
| if err != nil { | |
| return nil, err | |
| } | |
| return kt() | |
| }, | |
| // Optional: Set a custom logger | |
| Log: flog.DefaultLogger().Logger().(*log.Logger), | |
| } | |
| import ( | |
| flog "github.com/gofiber/fiber/v3/log" | |
| "fmt" | |
| "log" | |
| "github.com/jcmturner/gokrb5/v8/keytab" | |
| "github.com/gofiber/fiber/v3" | |
| "github.com/gofiber/contrib/spnego/config" | |
| "github.com/gofiber/contrib/spnego/v3" | |
| ) | |
| // Create a configuration with a keytab lookup function | |
| cfg := &config.Config{ | |
| // Use a function to look up keytab from files | |
| KeytabLookup: func() (*keytab.Keytab, error) { | |
| // Implement your keytab lookup logic here | |
| // This could be from files, database, or other sources | |
| kt, err := config.NewKeytabFileLookupFunc("/path/to/keytab/file.keytab") | |
| if err != nil { | |
| return nil, err | |
| } | |
| return kt() | |
| }, | |
| // Optional: Set a custom logger | |
| Log: flog.DefaultLogger().Logger().(*log.Logger), | |
| } |
🤖 Prompt for AI Agents
In spnego/README.md lines 52 to 66, the example incorrectly uses spnego.Config
and spnego.NewKeytabFileLookupFunc instead of the config package. Update the
code to use config.Config and config.NewKeytabFileLookupFunc to align with the
correct API. Additionally, add the missing import statement for the config
package to ensure the example compiles and runs correctly.
spnego/v2/spnego_test.go
Outdated
| if _, err = kt.Write(file); err != nil { | ||
| return filename, clean, fmt.Errorf("write file failed: %w", err) | ||
| } | ||
| file.Close() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check error from file.Close()
File close operations can fail, especially when writes are buffered. Not checking the error could mask write failures.
- file.Close()
- return filename, clean, nil
+ if err := file.Close(); err != nil {
+ return filename, clean, fmt.Errorf("close file failed: %w", err)
+ }
+ return filename, clean, nilAlso applies to: 141-141
🤖 Prompt for AI Agents
In spnego/v2/spnego_test.go at lines 121 and 141, the file.Close() calls do not
check for errors, which can mask write failures. Update these lines to capture
the error returned by file.Close() and handle it appropriately, such as by
returning the error or logging it, to ensure any issues during file close are
not ignored.
| } | ||
| // Set default logger if not provided | ||
| if cfg.Log == nil { | ||
| cfg.Log = flog.DefaultLogger().Logger().(*log.Logger) |
There was a problem hiding this comment.
Unsafe type assertion could cause panic
The type assertion flog.DefaultLogger().Logger().(*log.Logger) will panic if the underlying logger is not a *log.Logger.
- cfg.Log = flog.DefaultLogger().Logger().(*log.Logger)
+ if logger, ok := flog.DefaultLogger().Logger().(*log.Logger); ok {
+ cfg.Log = logger
+ } else {
+ cfg.Log = log.New(os.Stderr, "", log.LstdFlags|log.Lshortfile|log.Lmicroseconds)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cfg.Log = flog.DefaultLogger().Logger().(*log.Logger) | |
| // before: | |
| - cfg.Log = flog.DefaultLogger().Logger().(*log.Logger) | |
| // after: | |
| if logger, ok := flog.DefaultLogger().Logger().(*log.Logger); ok { | |
| cfg.Log = logger | |
| } else { | |
| cfg.Log = log.New(os.Stderr, "", log.LstdFlags|log.Lshortfile|log.Lmicroseconds) | |
| } |
🤖 Prompt for AI Agents
In spnego/v3/spnego.go at line 35, the direct type assertion to *log.Logger is
unsafe and may cause a panic if the underlying logger is not of that type.
Replace the type assertion with a type check using the comma-ok idiom to safely
verify the type before assigning it to cfg.Log. Handle the case where the
assertion fails gracefully, such as by logging an error or using a fallback
logger.
spnego/v3/spnego.go
Outdated
| func (w wrapCtx) Header() http.Header { | ||
| return w.Ctx.GetReqHeaders() | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Verify Header() implementation for response headers
Similar to the v2 implementation, the Header() method returns request headers, but http.ResponseWriter expects response headers.
🏁 Script executed:
#!/bin/bash
# Check if Fiber v3 has methods for getting response headers
ast-grep --pattern 'func ($_ $_) GetRespHeaders() $_'
# Search for response header methods in Fiber v3 context
rg -A 5 "func.*\(.*Ctx\).*Response" --glob "**/*.go"Length of output: 110
Update Header() to return response headers
The current implementation returns request headers via GetReqHeaders(), but for http.ResponseWriter you need to expose response headers. Fiber v3 doesn’t provide a GetRespHeaders() helper—use w.Ctx.Response().Header (a fasthttp.ResponseHeader) and convert it to an http.Header.
• File: spnego/v3/spnego.go, lines 96–98
• Replace the request‐header call with a conversion of the response header.
Suggested diff:
func (w wrapCtx) Header() http.Header {
- return w.Ctx.GetReqHeaders()
+ headers := http.Header{}
+ w.Ctx.Response().Header.VisitAll(func(k, v []byte) {
+ headers.Add(string(k), string(v))
+ })
+ return headers
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (w wrapCtx) Header() http.Header { | |
| return w.Ctx.GetReqHeaders() | |
| } | |
| func (w wrapCtx) Header() http.Header { | |
| headers := http.Header{} | |
| w.Ctx.Response().Header.VisitAll(func(k, v []byte) { | |
| headers.Add(string(k), string(v)) | |
| }) | |
| return headers | |
| } |
🤖 Prompt for AI Agents
In spnego/v3/spnego.go at lines 96 to 98, the Header() method incorrectly
returns request headers using GetReqHeaders(). Instead, update it to return
response headers by accessing w.Ctx.Response().Header, then convert this
fasthttp.ResponseHeader to an http.Header type before returning it. This ensures
the method correctly exposes response headers as required by
http.ResponseWriter.
|
|
||
| ```bash | ||
| # For Fiber v3 | ||
| go get github.com/gofiber/contrib/spnego/v3 |
There was a problem hiding this comment.
v2: github.com/gofiber/contrib/v2/spnego
v3: github.com/gofiber/contrib/v3/spnego
You can arrange it so that the packet itself can also have major versions in the future.
future : github.com/gofiber/contrib/v3/spnego/v2
Of course, the go.mod would have to be in the subfolders for that.
There was a problem hiding this comment.
@jarod2011 We are currently working on this change and plan to use the prefix.
Can you please adjust it accordingly so that it fits into the scheme?
There was a problem hiding this comment.
Hi @ReneWerner87 ,
Thanks for your feedback!
To make sure I understand correctly and to align with the new scheme: should I split this PR into two?
One for the Fiber v3 compatibility, which would be merged into the v3-beta branch.
And another one for the Fiber v2 compatibility, which would be merged into the v2 branch.
Please let me know if this is the right approach. I'm happy to adjust accordingly.
Thanks!
|
@jarod2011 |
Move configuration and utility functions from config package to main package, optimize code organization Add new error types and utility functions, improve test coverage Simplify documentation content, remove redundant information
- Update English README document structure, add badges and document ID - Synchronize update of Chinese translation version - Unify keytab lookup implementation in example code
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
spnego/README.zh-CN.md (1)
190-191: Fix stray fenced code block (markdownlint MD040).A dangling triple/quadruple backtick breaks rendering and linting. Remove it or provide a language.
-``` - -```` +<!-- removed stray fences -->
♻️ Duplicate comments (8)
spnego/doc.go (1)
1-3: Duplicate doc line previously flagged appears resolved.The earlier duplicate line reported in past reviews is no longer present. Good cleanup.
spnego/README.md (3)
44-91: Fix v3 example: incorrect API/package usage and unused imports.Examples won’t compile as-is. Past reviews noted the same; apply consistent
configusage and clean imports.import ( - "fmt" - "time" - - "github.com/gofiber/contrib/spnego" - "github.com/gofiber/contrib/spnego/utils" - v3 "github.com/gofiber/contrib/spnego/v3" - "github.com/gofiber/fiber/v3" - "github.com/gofiber/fiber/v3/log" + "fmt" + "log" + + "github.com/gofiber/fiber/v3" + "github.com/gofiber/contrib/spnego" + "github.com/gofiber/contrib/spnego/config" + v3 "github.com/gofiber/contrib/spnego/v3" ) @@ - // For testing, you can create a mock keytab file using utils.NewMockKeytab - // In production, use a real keytab file - keytabLookup, err := spnego.NewKeytabFileLookupFunc("/path/to/keytab/file.keytab") + // Use a keytab file (replace with your path). For tests, see utils.NewMockKeytab. + keytabLookup, err := config.NewKeytabFileLookupFunc("/path/to/keytab/file.keytab") @@ - authMiddleware, err := v3.NewSpnegoKrb5AuthenticateMiddleware(spnego.Config{ - KeytabLookup: keytabLookup, - }) + authMiddleware, err := v3.NewSpnegoKrb5AuthenticateMiddleware(config.Config{ + KeytabLookup: keytabLookup, + })Note: If the repository adopts the
github.com/gofiber/contrib/v3/spnegolayout, update the three imports accordingly:contrib/v3/spnego,contrib/v3/spnego/config, and keepfiber/v3.
95-144: Fix v2 example: same API/package issues as v3 snippet.Mirror the corrections for v2 and ensure
logandosare actually used.import ( "fmt" "log" "os" - "github.com/gofiber/contrib/spnego" - "github.com/gofiber/contrib/spnego/utils" - v2 "github.com/gofiber/contrib/spnego/v2" "github.com/gofiber/fiber/v2" + "github.com/gofiber/contrib/spnego" + "github.com/gofiber/contrib/spnego/config" + v2 "github.com/gofiber/contrib/spnego/v2" ) @@ - // For testing, you can create a mock keytab file using utils.NewMockKeytab - // In production, use a real keytab file - keytabLookup, err := spnego.NewKeytabFileLookupFunc("/path/to/keytab/file.keytab") + // Use a keytab file (replace with your path). For tests, see utils.NewMockKeytab. + keytabLookup, err := config.NewKeytabFileLookupFunc("/path/to/keytab/file.keytab") @@ - authMiddleware, err := v2.NewSpnegoKrb5AuthenticateMiddleware(spnego.Config{ + authMiddleware, err := v2.NewSpnegoKrb5AuthenticateMiddleware(config.Config{ KeytabLookup: keytabLookup, // Optional: Set a custom logger Log: log.New(os.Stderr, "", log.LstdFlags|log.Lshortfile|log.Lmicroseconds), })
166-179: API reference: pointNewKeytabFileLookupFuncto the config package and correct return types.Reference belongs to the
configpackage and returnsconfig.KeytabLookupFunc.-### `NewKeytabFileLookupFunc(keytabFiles ...string) (KeytabLookupFunc, error)` +### `config.NewKeytabFileLookupFunc(keytabFiles ...string) (config.KeytabLookupFunc, error)` -Creates a new KeytabLookupFunc that loads keytab files. +Creates a new KeytabLookupFunc that loads and merges keytab files.spnego/README.zh-CN.md (1)
189-189: Don’t reference an unexported/internal context key in docs.The variable
spnego.contextKeyOfIdentityis internal and not part of the public API. Directing users to it is misleading; the supported API isspnego.GetAuthenticatedIdentityFromContext(andSetAuthenticatedIdentityToContext).-- 已认证的身份使用`spnego.contextKeyOfIdentity`存储在Fiber上下文中 +- 已认证的身份通过 `spnego.GetAuthenticatedIdentityFromContext(ctx)` 从 Fiber 上下文中获取。 + 中间件会在认证成功时将身份放入上下文；请使用公开的辅助函数进行读写。spnego/v3/spnego.go (2)
55-57: Verify ResponseWriter adapter writes response headers, not request headers.The SPNEGO library sets WWW-Authenticate on the ResponseWriter. If utils.NewWrapFiberContext returns a Header() backed by request headers, the challenge will never reach clients. Ensure Header() maps to ctx.Response().Header. Add/adjust tests (see v3 test suggestion) to catch this.
#!/bin/bash # Verify Header() implementation of the WrapFiberContext adapter rg -nP -C3 'type\s+WrapFiberContext.*\n|\bHeader\(\)\s+http\.Header' spnego/utils | sed -n '1,200p' rg -nP -C3 'GetReqHeaders|Response\(\)\.Header' spnego/utilsExpected: Header() should read/write via response headers (ctx.Response().Header); no use of GetReqHeaders for ResponseWriter.
--- `32-34`: **Avoid unsafe type assertion when defaulting the logger.** Directly asserting to *log.Logger can panic if Fiber’s default logger changes type. Use comma-ok and a sane fallback. ```diff - if cfg.Log == nil { - cfg.Log = flog.DefaultLogger().Logger().(*log.Logger) - } + if cfg.Log == nil { + if logger, ok := flog.DefaultLogger().Logger().(*log.Logger); ok { + cfg.Log = logger + } else { + cfg.Log = log.New(os.Stderr, "", log.LstdFlags|log.Lshortfile|log.Lmicroseconds) + } + }Also add missing import if needed:
import ( "fmt" "log" + "os" "net/http"spnego/v2/spnego.go (1)
56-58: Same ResponseWriter adapter concern for v2.Confirm that utils.NewWrapFiberContext(*fiber.Ctx) implements http.ResponseWriter over response headers and status, not request headers—otherwise WWW-Authenticate/Negotiate won’t be emitted.
#!/bin/bash # Check the v2 adapter as well (shared utils) rg -nP -C3 'WrapFiberContext.*Header\(\)\s+http\.Header' spnego/utils rg -nP -C3 'GetReqHeaders|Response\(\)\.Header' spnego/utils
🧹 Nitpick comments (28)
spnego/doc.go (1)
1-4: Tighten package doc; make it a single, punctuated sentence.The first sentence should fully describe the package and end with a period. Suggest collapsing the two lines into one sentence for godoc clarity.
-// Package spnego provides SPNEGO (Simple and Protected GSSAPI Negotiation Mechanism) -// authentication middleware for Fiber applications. -// It enables Kerberos authentication for HTTP requests. +// Package spnego provides SPNEGO (Simple and Protected GSSAPI Negotiation Mechanism) authentication middleware for Fiber applications, enabling Kerberos-based SSO for HTTP requests.spnego/error.go (2)
5-18: Polish error message phrasing for clarity/consistency.Minor grammar tweaks improve readability and align with Go error style (“failed to …”, “invalid config: …”).
-var ErrConfigInvalidOfKeytabLookupFunctionRequired = errors.New("config invalid: keytab lookup function is required") +var ErrConfigInvalidOfKeytabLookupFunctionRequired = errors.New("invalid config: keytab lookup function is required") -var ErrLookupKeytabFailed = errors.New("keytab lookup failed") +var ErrLookupKeytabFailed = errors.New("failed to look up keytab") -var ErrConvertRequestFailed = errors.New("convert request failed") +var ErrConvertRequestFailed = errors.New("failed to convert request") -var ErrConfigInvalidOfAtLeastOneKeytabFileRequired = errors.New("config invalid: at least one keytab file required") +var ErrConfigInvalidOfAtLeastOneKeytabFileRequired = errors.New("invalid config: at least one keytab file is required") -var ErrLoadKeytabFileFailed = errors.New("load keytab failed") +var ErrLoadKeytabFileFailed = errors.New("failed to load keytab")
5-18: Consolidate or Rename Duplicated Error Variables Across PackagesI’ve confirmed that the same error variables are declared both in
spnego/error.go(lines 6, 9, 12, 15, 18) and inspnego/config/config.go(lines 12, 15, 18, 20). Since they live in separate packages (spnegovs.spnego/config), you won’t get a compile-time conflict, but this duplication can lead to maintenance overhead and subtle inconsistencies.• spnego/error.go:
- ErrConfigInvalidOfKeytabLookupFunctionRequired
- ErrLookupKeytabFailed
- ErrConvertRequestFailed
- ErrConfigInvalidOfAtLeastOneKeytabFileRequired
- ErrLoadKeytabFileFailed
• spnego/config/config.go:
- ErrConfigInvalidOfKeytabLookupFunctionRequired
- ErrLookupKeytabFailed
- ErrConvertRequestFailed
- ErrConfigInvalidOfAtLeastOneKeytabFileRequired
Consider one of the following to reduce duplication and keep error handling consistent:
- Extract these shared error declarations into a common package (e.g.,
spnego/errors) and import them where needed.- Rename or differentiate the config-specific errors if they’re semantically distinct.
- Document the duplication clearly if keeping separate definitions is intentional.
spnego/identity.go (1)
5-7: Interface name duplicates utils.FiberContext; consider rename to avoid confusion.There’s another
FiberContextin spnego/utils. Different packages, but same name increases cognitive load. Optional rename, e.g.,FiberLocalsContext.spnego/README.md (4)
168-174: API signature for middleware should mention versioned package explicitly or show both.Currently ambiguous. Consider adding both v2 and v3 signatures or a note.
-### `NewSpnegoKrb5AuthenticateMiddleware(cfg spnego.Config) (fiber.Handler, error)` +### `v3.NewSpnegoKrb5AuthenticateMiddleware(cfg config.Config) (fiber.Handler, error)` +### `v2.NewSpnegoKrb5AuthenticateMiddleware(cfg config.Config) (fiber.Handler, error)`
198-199: Avoid documenting an unexported key; describe behavior instead.Mentioning
spnego.contextKeyOfIdentityleaks an internal detail and may change. Prefer describing the helper functions.-- Authenticated identities are stored in the Fiber context using `spnego.contextKeyOfIdentity` +- Authenticated identities are stored in the Fiber context via `spnego.SetAuthenticatedIdentityToContext` and can be retrieved with `spnego.GetAuthenticatedIdentityFromContext`.
70-73: Replace hard tab with spaces (markdownlint MD010).There’s a hard tab before
KeytabLookup:; use spaces.- authMiddleware, err := v3.NewSpnegoKrb5AuthenticateMiddleware(spnego.Config{ - KeytabLookup: keytabLookup, - }) + authMiddleware, err := v3.NewSpnegoKrb5AuthenticateMiddleware(spnego.Config{ + KeytabLookup: keytabLookup, + })
5-11: Add standard header block and link from root README.Per maintainer comment, adopt the contrib README header (badges, quick links) and add an entry in the repository root README pointing to this middleware.
I can open a follow-up PR to update the root README and add the standardized header here if you’d like.
spnego/utils/adapter.go (2)
49-57: Initialize adapter state in constructor.Initialize
headerand default status.func NewWrapFiberContext[T FiberContext[T]](ctx T) *WrapFiberContextAdaptHttpResponseWriter[T] { - return &WrapFiberContextAdaptHttpResponseWriter[T]{ - ctx: ctx, - } + return &WrapFiberContextAdaptHttpResponseWriter[T]{ + ctx: ctx, + header: make(http.Header), + statusCode: http.StatusOK, + } }
21-26: Name is overly verbose; consider a shorter, conventional type name.E.g.,
ResponseWriterAdapter[T]orFiberResponseWriter[T]. Improves readability without changing behavior.spnego/utils/adapter_test.go (2)
19-19: Fix test name typo: “Fiver” → “Fiber”.Keeps naming consistent and discoverable when running tests selectively.
-func TestNewWrapFiberContextOfFiverV2(t *testing.T) { +func TestNewWrapFiberContextOfFiberV2(t *testing.T) { @@ -func TestNewWrapFiberContextOfFiverV3(t *testing.T) { +func TestNewWrapFiberContextOfFiberV3(t *testing.T) {Also applies to: 41-41
19-33: Make tests parallel.Both tests are independent; running them in parallel speeds up the suite without side effects.
func TestNewWrapFiberContextOfFiberV2(t *testing.T) { + t.Parallel() @@ func TestNewWrapFiberContextOfFiberV3(t *testing.T) { + t.Parallel()Also applies to: 41-55
spnego/README.zh-CN.md (2)
22-31: Optional: add shell prompt language to installation block for better formatting.Improves readability and satisfies markdown linters in some setups.
-```bash +```bash # 对于Fiber v3 go get github.com/gofiber/contrib/spnego/v3 @@ # 对于Fiber v2 go get github.com/gofiber/contrib/spnego/v2--- `1-21`: **Consider adding a pointer to the repository root README as requested by reviewers.** A short “See root README for overview” link addresses the request from ReneWerner87 and helps discoverability. Would you like me to draft a concise entry (EN/zh-CN) for the root README that links to this guide and highlights SPNEGO support? </blockquote></details> <details> <summary>spnego/utils/keytab_test.go (1)</summary><blockquote> `10-46`: **Make the test parallel; otherwise looks solid.** Parallelizing is safe here and reduces suite time. The assertions are deterministic (Unix timestamps). ```diff func TestGetKeytabInfo(t *testing.T) { + t.Parallel() tm := time.Now()spnego/config_test.go (4)
17-27: Rename subtest and clarify its intent (invalid file vs. not found).The case writes an invalid keytab file, so "not found" is misleading. Rename to "invalid keytab file content" for clarity.
- t.Run("test not found keytab file", func(t *testing.T) { + t.Run("test invalid keytab file content", func(t *testing.T) {
18-22: Use t.TempDir to avoid filename collisions and manual cleanup.Writing to ./invalid.keytab can collide across runs and requires manual cleanup. Prefer t.TempDir + filepath.Join to isolate files.
- err := os.WriteFile("./invalid.keytab", []byte("12345"), 0600) + dir := t.TempDir() + invalid := filepath.Join(dir, "invalid.keytab") + err := os.WriteFile(invalid, []byte("12345"), 0o600) ... - fn, err := NewKeytabFileLookupFunc("./invalid.keytab") + fn, err := NewKeytabFileLookupFunc(invalid)Also add:
import ( "os" + "path/filepath" "testing" "time"Also applies to: 23-27
75-79: Multiple-files test: prefer temp paths; avoid hardcoded names and manual cleanup.Same rationale as above—use t.TempDir and filepath.Join; drop os.Remove via t.Cleanup.
- err = os.WriteFile("./invalid1.keytab", []byte("12345"), 0600) + dir := t.TempDir() + invalid := filepath.Join(dir, "invalid1.keytab") + err = os.WriteFile(invalid, []byte("12345"), 0o600) ... - fn, err := NewKeytabFileLookupFunc("./temp.keytab", "./invalid1.keytab") + fn, err := NewKeytabFileLookupFunc(filepath.Join(dir, "temp.keytab"), invalid)
81-131: Assert by content rather than relying on order, and use temp files.GetKeytabInfo sorts by principal; asserting indices is okay but brittle if sort criteria change. Prefer verifying via a map keyed by principal with per-pair checks. Also use t.TempDir for both files as suggested above.
- require.Equal(t, info[0].PrincipalName, "HTTP/[email protected]") - ... - require.Equal(t, info[1].PrincipalName, "HTTP/[email protected]") + m := map[string]utils.KeytabInfo{} + for _, it := range info { m[it.PrincipalName] = it } + it1, ok := m["HTTP/[email protected]"]; require.True(t, ok) + require.Len(t, it1.Pairs, 1) + require.Equal(t, uint8(2), it1.Pairs[0].Version) + require.Equal(t, int32(18), it1.Pairs[0].EncryptType) + require.Equal(t, tm.Unix(), it1.Pairs[0].CreateTime.Unix()) + it2, ok := m["HTTP/[email protected]"]; require.True(t, ok) + require.Len(t, it2.Pairs, 2) + // ...same assertions for it2spnego/v3/spnego_test.go (2)
24-41: Avoid brittle body equality; assert error classification and status only.Relying on the exact error message body may break across Fiber versions/config. Prefer status + substring or error class.
- require.Equal(t, http.StatusInternalServerError, ctx.Response.StatusCode()) - require.Equal(t, fmt.Sprintf("%s: mock keytab lookup error", spnego.ErrLookupKeytabFailed), string(ctx.Response.Body())) + require.Equal(t, http.StatusInternalServerError, ctx.Response.StatusCode()) + require.Contains(t, string(ctx.Response.Body()), spnego.ErrLookupKeytabFailed.Error()) + require.Contains(t, string(ctx.Response.Body()), "mock keytab lookup error")
74-88: Assert SPNEGO challenge headers on 401 to catch ResponseWriter adapter bugs.Add assertions for:
- WWW-Authenticate: Negotiate
- Vary: Authorization (recommended by RFC 7235 §4.1)
This will detect regressions where headers are written to the request header map instead of the response.handler(ctx) require.Equal(t, fasthttp.StatusUnauthorized, ctx.Response.StatusCode()) + require.Contains(t, string(ctx.Response.Header.Peek("WWW-Authenticate")), "Negotiate") + require.Contains(t, string(ctx.Response.Header.Peek("Vary")), "Authorization")spnego/utils/mock_keytab_test.go (2)
52-68: Rename and clarify: "no file output" case.Subtest name "none file created" is awkward; "no file persisted" is clearer. The checks look good.
- t.Run("test none file created", func(t *testing.T) { + t.Run("test no file persisted", func(t *testing.T) {
84-98: Same: restore global and avoid fixed path.Also, prefer a temp dir path to avoid collisions; the test currently asserts removal on failure, but creating "./temp.keytab" across runs can collide.
- defaultFileOperator = mockFileOperator{flag: 0x02} + prev := defaultFileOperator + defaultFileOperator = mockFileOperator{flag: 0x02} + t.Cleanup(func() { defaultFileOperator = prev }) ... - WithFilename("./temp.keytab"), + dir := t.TempDir() + WithFilename(filepath.Join(dir, "temp.keytab")),Also add import:
+ "path/filepath"spnego/utils/keytab.go (1)
73-76: Optional: also sort each principal’s Pairs for stable, deterministic outputIf the caller inspects
Pairs, consider sorting them (e.g., by EncryptType, then Version, then CreateTime). This makes outputs deterministic across runs and merges.sort.Slice(mk, func(i, j int) bool { return mk[i].PrincipalName < mk[j].PrincipalName }) +// Ensure deterministic order of pairs inside each principal +for i := range mk { + sort.Slice(mk[i].Pairs, func(a, b int) bool { + if mk[i].Pairs[a].EncryptType == mk[i].Pairs[b].EncryptType { + if mk[i].Pairs[a].Version == mk[i].Pairs[b].Version { + return mk[i].Pairs[a].CreateTime.Before(mk[i].Pairs[b].CreateTime) + } + return mk[i].Pairs[a].Version < mk[i].Pairs[b].Version + } + return mk[i].Pairs[a].EncryptType < mk[i].Pairs[b].EncryptType + }) +}spnego/config.go (2)
34-44: Optional: deduplicate entries across multiple keytabsIf the same entry exists in multiple files, the merged keytab will contain duplicates. Consider deduping by fields that uniquely identify a key entry (principal, enctype, version, timestamp).
return func() (*keytab.Keytab, error) { - var mergeKeytab keytab.Keytab + var mergeKeytab keytab.Keytab + seen := make(map[string]struct{}, 256) for _, keytabFile := range keytabFiles { kt, err := keytab.Load(keytabFile) if err != nil { return nil, fmt.Errorf("%w: file %s load failed: %w", ErrLoadKeytabFileFailed, keytabFile, err) } - mergeKeytab.Entries = append(mergeKeytab.Entries, kt.Entries...) + for _, e := range kt.Entries { + // principal | enctype | kvno | ts (unix seconds) – adjust if your uniqueness criteria differ + k := fmt.Sprintf("%s|%d|%d|%d", e.Principal.String(), e.Key.KeyType, e.KVNO8, e.Timestamp.Unix()) + if _, ok := seen[k]; ok { + continue + } + seen[k] = struct{}{} + mergeKeytab.Entries = append(mergeKeytab.Entries, e) + } } return &mergeKeytab, nil }, nil
28-45: Optional: add a caching wrapper to avoid re-reading files on every lookupIf
KeytabLookupis invoked per request or per handshake, consider an opt-in, TTL-based cache to reduce I/O, with a background refresh strategy.Example (new helper, can live alongside this file):
package spnego import ( "sync" "time" "github.com/jcmturner/gokrb5/v8/keytab" ) func NewCachedKeytabLookupFunc(loader KeytabLookupFunc, ttl time.Duration) KeytabLookupFunc { var ( mu sync.RWMutex kt *keytab.Keytab exp time.Time ) return func() (*keytab.Keytab, error) { now := time.Now() mu.RLock() if kt != nil && now.Before(exp) { defer mu.RUnlock() return kt, nil } mu.RUnlock() mu.Lock() defer mu.Unlock() if kt != nil && now.Before(exp) { // double-check return kt, nil } fresh, err := loader() if err != nil { return nil, err } kt = fresh exp = now.Add(ttl) return kt, nil } }spnego/utils/mock_keytab.go (2)
124-132: Wrap underlying error and validate inputs (principal/pairs) for clearer failures
- Use
%wto preserve error chains fromAddEntry.- Optionally fail fast when
PrincipalNameis empty or noPairsare provided; this prevents creating unusable keytabs in tests.opt := newDefaultMockOptions() opt.apply(opts...) kt := keytab.New() var err error +if opt.PrincipalName == "" { + return nil, nil, fmt.Errorf("principal name is required") +} +if len(opt.Pairs) == 0 { + return nil, nil, fmt.Errorf("at least one EncryptTypePair is required") +} for _, pair := range opt.Pairs { - if err = kt.AddEntry(opt.PrincipalName, opt.Realm, opt.Password, pair.CreateTime, pair.Version, pair.EncryptType); err != nil { - return nil, nil, fmt.Errorf("error adding entry: %v", err) + if err = kt.AddEntry(opt.PrincipalName, opt.Realm, opt.Password, pair.CreateTime, pair.Version, pair.EncryptType); err != nil { + return nil, nil, fmt.Errorf("add entry: %w", err) } }
92-103: Nit: clarify naming to reflect purpose
myFileOperator/defaultFileOperatorare fine, butosFileOperatorcommunicates intent slightly better.-type myFileOperator struct{} +type osFileOperator struct{} -func (m myFileOperator) OpenFile(filename string, flag int, perm os.FileMode) (*os.File, error) { +func (m osFileOperator) OpenFile(filename string, flag int, perm os.FileMode) (*os.File, error) { return os.OpenFile(filename, flag, perm) } -func (m myFileOperator) Remove(filename string) error { +func (m osFileOperator) Remove(filename string) error { return os.Remove(filename) } -var defaultFileOperator fileOperator = myFileOperator{} +var defaultFileOperator fileOperator = osFileOperator{}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (19)
spnego/README.md(1 hunks)spnego/README.zh-CN.md(1 hunks)spnego/config.go(1 hunks)spnego/config_test.go(1 hunks)spnego/doc.go(1 hunks)spnego/error.go(1 hunks)spnego/example/example.go(1 hunks)spnego/identity.go(1 hunks)spnego/identity_test.go(1 hunks)spnego/utils/adapter.go(1 hunks)spnego/utils/adapter_test.go(1 hunks)spnego/utils/keytab.go(1 hunks)spnego/utils/keytab_test.go(1 hunks)spnego/utils/mock_keytab.go(1 hunks)spnego/utils/mock_keytab_test.go(1 hunks)spnego/v2/spnego.go(1 hunks)spnego/v2/spnego_test.go(1 hunks)spnego/v3/spnego.go(1 hunks)spnego/v3/spnego_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- spnego/v2/spnego_test.go
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:22-25
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The `encryptcookie_test.go` file contains unit tests that validate key lengths for both `EncryptCookie` and `DecryptCookie` functions, ensuring that invalid key lengths raise appropriate errors.
Applied to files:
spnego/utils/keytab_test.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
spnego/utils/adapter_test.gospnego/utils/adapter.gospnego/v2/spnego.gospnego/v3/spnego.go
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
spnego/utils/adapter_test.go
📚 Learning: 2024-11-21T11:28:14.554Z
Learnt from: ReneWerner87
PR: gofiber/contrib#0
File: :0-0
Timestamp: 2024-11-21T11:28:14.554Z
Learning: Using `fiber.Ctx`, even when wrapped, does not achieve version independence, as it still relies on the specific Fiber implementation.
Applied to files:
spnego/utils/adapter_test.go
📚 Learning: 2024-11-29T15:24:06.083Z
Learnt from: ReneWerner87
PR: gofiber/contrib#0
File: :0-0
Timestamp: 2024-11-29T15:24:06.083Z
Learning: When customizing the Fiber context to achieve version independence, using generics in methods like `Status` and `Type` allows for fluent method chaining. Implementing a generic interface for `Ctx` on the `App` prevents class switching when registering custom contexts that use fluent methods. Providing multiple `New` methods allows users who do not wish to customize the context to continue using `fiber.New` without changes.
Applied to files:
spnego/utils/adapter_test.go
📚 Learning: 2024-09-25T15:57:10.221Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Applied to files:
spnego/v2/spnego.gospnego/v3/spnego.go
📚 Learning: 2025-05-13T00:19:16.407Z
Learnt from: sixcolors
PR: gofiber/fiber#3446
File: docs/middleware/logger.md:44-44
Timestamp: 2025-05-13T00:19:16.407Z
Learning: In documentation files for the Fiber framework, code examples are often partial and don't repeat import statements that were shown in earlier examples, focusing instead on demonstrating specific usage patterns.
Applied to files:
spnego/README.md
📚 Learning: 2024-11-23T19:50:41.765Z
Learnt from: norri
PR: gofiber/recipes#2701
File: clean-code/app/server/server.go:14-16
Timestamp: 2024-11-23T19:50:41.765Z
Learning: In the `clean-code` example at `clean-code/app/server/server.go` using the Go Fiber framework, it's acceptable to omit production-level features like context usage and graceful shutdown handling to keep the example simple.
Applied to files:
spnego/README.md
🧬 Code graph analysis (13)
spnego/utils/keytab_test.go (2)
spnego/utils/mock_keytab.go (5)
NewMockKeytab(123-150)WithRealm(40-44)WithPrincipal(32-36)WithPassword(67-71)WithPairs(58-62)spnego/utils/keytab.go (2)
EncryptTypePair(22-26)GetKeytabInfo(50-77)
spnego/identity_test.go (1)
spnego/identity.go (2)
SetAuthenticatedIdentityToContext(12-14)GetAuthenticatedIdentityFromContext(25-28)
spnego/utils/adapter_test.go (1)
spnego/utils/adapter.go (1)
NewWrapFiberContext(53-57)
spnego/example/example.go (5)
spnego/utils/mock_keytab.go (5)
NewMockKeytab(123-150)WithPrincipal(32-36)WithRealm(40-44)WithFilename(49-53)WithPairs(58-62)spnego/utils/keytab.go (1)
EncryptTypePair(22-26)spnego/config.go (2)
NewKeytabFileLookupFunc(30-45)Config(21-26)spnego/v3/spnego.go (1)
NewSpnegoKrb5AuthenticateMiddleware(26-59)spnego/identity.go (1)
GetAuthenticatedIdentityFromContext(25-28)
spnego/config_test.go (4)
spnego/config.go (1)
NewKeytabFileLookupFunc(30-45)spnego/error.go (2)
ErrConfigInvalidOfAtLeastOneKeytabFileRequired(15-15)ErrLoadKeytabFileFailed(18-18)spnego/utils/mock_keytab.go (5)
NewMockKeytab(123-150)WithPrincipal(32-36)WithRealm(40-44)WithPairs(58-62)WithFilename(49-53)spnego/utils/keytab.go (2)
EncryptTypePair(22-26)GetKeytabInfo(50-77)
spnego/identity.go (1)
spnego/utils/adapter.go (1)
FiberContext(12-19)
spnego/config.go (1)
spnego/error.go (2)
ErrConfigInvalidOfAtLeastOneKeytabFileRequired(15-15)ErrLoadKeytabFileFailed(18-18)
spnego/utils/mock_keytab_test.go (2)
spnego/utils/mock_keytab.go (6)
NewMockKeytab(123-150)WithPrincipal(32-36)WithRealm(40-44)WithPairs(58-62)WithFilename(49-53)WithPassword(67-71)spnego/utils/keytab.go (1)
EncryptTypePair(22-26)
spnego/utils/adapter.go (1)
spnego/identity.go (1)
FiberContext(5-7)
spnego/utils/mock_keytab.go (1)
spnego/utils/keytab.go (1)
EncryptTypePair(22-26)
spnego/v2/spnego.go (6)
spnego/v3/spnego.go (1)
NewSpnegoKrb5AuthenticateMiddleware(26-59)spnego/config.go (1)
Config(21-26)spnego/config/config.go (4)
Config(37-42)ErrConfigInvalidOfKeytabLookupFunctionRequired(12-12)ErrLookupKeytabFailed(15-15)ErrConvertRequestFailed(18-18)spnego/error.go (3)
ErrConfigInvalidOfKeytabLookupFunctionRequired(6-6)ErrLookupKeytabFailed(9-9)ErrConvertRequestFailed(12-12)spnego/identity.go (1)
SetAuthenticatedIdentityToContext(12-14)spnego/utils/adapter.go (1)
NewWrapFiberContext(53-57)
spnego/v3/spnego.go (6)
spnego/v2/spnego.go (1)
NewSpnegoKrb5AuthenticateMiddleware(26-60)spnego/config.go (1)
Config(21-26)spnego/config/config.go (4)
Config(37-42)ErrConfigInvalidOfKeytabLookupFunctionRequired(12-12)ErrLookupKeytabFailed(15-15)ErrConvertRequestFailed(18-18)spnego/error.go (3)
ErrConfigInvalidOfKeytabLookupFunctionRequired(6-6)ErrLookupKeytabFailed(9-9)ErrConvertRequestFailed(12-12)spnego/identity.go (1)
SetAuthenticatedIdentityToContext(12-14)spnego/utils/adapter.go (1)
NewWrapFiberContext(53-57)
spnego/v3/spnego_test.go (4)
spnego/v3/spnego.go (1)
NewSpnegoKrb5AuthenticateMiddleware(26-59)spnego/config.go (2)
Config(21-26)NewKeytabFileLookupFunc(30-45)spnego/utils/mock_keytab.go (1)
NewMockKeytab(123-150)spnego/identity.go (1)
GetAuthenticatedIdentityFromContext(25-28)
🪛 LanguageTool
spnego/README.md
[grammar] ~17-~17: There might be a mistake here.
Context: ...eros authentication via SPNEGO mechanism - Flexible keytab lookup system - Support ...
(QB_NEW_EN)
[grammar] ~18-~18: There might be a mistake here.
Context: ...echanism - Flexible keytab lookup system - Support for dynamic keytab retrieval fro...
(QB_NEW_EN)
[grammar] ~19-~19: There might be a mistake here.
Context: ...ic keytab retrieval from various sources - Integration with Fiber context for authe...
(QB_NEW_EN)
[grammar] ~20-~20: There might be a mistake here.
Context: ...ntext for authenticated identity storage - Configurable logging ## Version Compati...
(QB_NEW_EN)
[grammar] ~27-~27: There might be a mistake here.
Context: ...ses: - v2: Compatible with Fiber v2 - v3: Compatible with Fiber v3 ## Insta...
(QB_NEW_EN)
[grammar] ~184-~184: There might be a mistake here.
Context: ...ion that retrieves the keytab (required) - Log: The logger used for middleware logging...
(QB_NEW_EN)
[grammar] ~189-~189: There might be a mistake here.
Context: ...r) ## Requirements - Go 1.21 or higher - For v3: Fiber v3 - For v2: Fiber v2 - Ke...
(QB_NEW_EN)
[grammar] ~190-~190: There might be a mistake here.
Context: ... - Go 1.21 or higher - For v3: Fiber v3 - For v2: Fiber v2 - Kerberos infrastructu...
(QB_NEW_EN)
[grammar] ~191-~191: There might be a mistake here.
Context: ...er - For v3: Fiber v3 - For v2: Fiber v2 - Kerberos infrastructure ## Notes - Ens...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
spnego/README.md
71-71: Hard tabs
Column: 1
(MD010, no-hard-tabs)
spnego/README.zh-CN.md
190-190: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (8)
spnego/README.md (1)
32-38: Align SPNEGO module import paths across code and docsI’ve spotted mixed import patterns in the SPNEGO example and README files:
- Unversioned imports:
github.com/gofiber/contrib/spnegogithub.com/gofiber/contrib/spnego/utils- Versioned imports:
github.com/gofiber/contrib/spnego/v2github.com/gofiber/contrib/spnego/v3Please pick one repository‐wide pattern and make it consistent everywhere:
• Confirm the module path declared in the SPNEGO Go module (e.g. in
spnego/go.mod)—it should include the major version suffix if you’re using Go modules (for v3, for example, it should readmodule github.com/gofiber/contrib/spnego/v3).
• Update all installation commands inspnego/README.mdandspnego/README.zh-CN.mdto usego get github.com/gofiber/contrib/spnego/v{N}.
• Update all import statements (inspnego/example/example.go, README snippets, etc.) to match that same pattern—including nested packages, e.g.github.com/gofiber/contrib/spnego/v3/utilsrather than the unversioned path.
• Rungo mod tidy(or your workspace setup) to validate that the chosen import paths resolve correctly.Once imports and docs are fully aligned to the module’s declared path, this will avoid confusion and ensure smooth version upgrades.
spnego/v3/spnego_test.go (1)
19-22: Nil KeytabLookup validation looks good.This asserts the required config gate appropriately.
spnego/utils/mock_keytab_test.go (2)
35-51: Test name clarity and deterministic timing."test add keytab entry failed" is fine; consider fixing create times to tm := time.Unix(...) for deterministic snapshots if needed. No action required.
130-155: Functional options tests look good; minor style nit.Option application order and content are validated well. No issues.
spnego/v2/spnego.go (1)
31-35: Sane default logger in v2 looks good.Creating a fresh *log.Logger avoids unsafe assertions. No changes needed.
spnego/utils/keytab.go (2)
51-68: Aggregation logic is clear and correctGrouping entries by
Principal.String()and accumulatingEncryptTypePairlooks good. Using a temporary map and then materializing a slice is appropriate before sorting.
6-7: Confirm Go 1.21+ requirement formapsimportThe new import of the standard
mapspackage (spnego/utils/keytab.go, lines 6-7) requires Go 1.21 or newer. I wasn’t able to find ago.modorgo.workdeclaring a minimum Go version, nor any CI workflows or Dockerfiles pinning the Go image. Please verify that:
- A
go.mod(orgo.work) file exists at the repository root (or relevant module directory) withor higher.go 1.21- Your CI pipelines (e.g. GitHub Actions, Travis, etc.) and any Dockerfiles use a Go 1.21+ image.
- If you’re not using modules, that your build environment is still configured to use Go 1.21 or later.
Ensuring these will prevent build errors when importing
maps.spnego/config.go (1)
30-45: Behavior is sound; loader composes multiple keytabs correctlyValidates input, loads files, merges entries, and wraps errors with useful context. API shape (variadic files, closure returning merged keytab) is clean.
| t.Run("test one keytab file", func(t *testing.T) { | ||
| tm := time.Now() | ||
| _, clean, err := utils.NewMockKeytab( | ||
| utils.WithPrincipal("HTTP/sso.example.com"), | ||
| utils.WithRealm("TEST.LOCAL"), | ||
| utils.WithPairs(utils.EncryptTypePair{ | ||
| Version: 2, | ||
| EncryptType: 18, | ||
| CreateTime: tm, | ||
| }), | ||
| utils.WithFilename("./temp.keytab"), | ||
| ) | ||
| require.NoError(t, err) | ||
| t.Cleanup(clean) | ||
| fn, err := NewKeytabFileLookupFunc("./temp.keytab") | ||
| require.NoError(t, err) | ||
| kt1, err := fn() | ||
| require.NoError(t, err) | ||
| info := utils.GetKeytabInfo(kt1) | ||
| require.Len(t, info, 1) | ||
| require.Equal(t, info[0].PrincipalName, "HTTP/[email protected]") | ||
| require.Equal(t, info[0].Realm, "TEST.LOCAL") | ||
| require.Len(t, info[0].Pairs, 1) | ||
| require.Equal(t, info[0].Pairs[0].Version, uint8(2)) | ||
| require.Equal(t, info[0].Pairs[0].EncryptType, int32(18)) | ||
| // Note: The creation time of keytab is only accurate to the second. | ||
| require.Equal(t, info[0].Pairs[0].CreateTime.Unix(), tm.Unix()) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden test keytab creation and remove flakiness.
- File handling: utils.NewMockKeytab persists with 0o666 in current impl, which is too permissive for keytab material. Use a temp directory and file (t.TempDir) and update the helper to use 0o600.
- Time checks: You already compare at second granularity; good. Keep using the same tm for all entries in this case.
- utils.WithFilename("./temp.keytab"),
+ dir := t.TempDir()
+ utils.WithFilename(filepath.Join(dir, "temp.keytab")),Follow-up change in spnego/utils/mock_keytab.go (outside this file):
- file, err := defaultFileOperator.OpenFile(opt.Filename, os.O_RDWR|os.O_CREATE, 0o666)
+ file, err := defaultFileOperator.OpenFile(opt.Filename, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o600)Committable suggestion skipped: line range outside the PR's diff.
| func ExampleNewSpnegoKrb5AuthenticateMiddleware() { | ||
| app := fiber.New() | ||
| // create mock keytab file | ||
| // you must use a real keytab file | ||
| _, clean, err := utils.NewMockKeytab( | ||
| utils.WithPrincipal("HTTP/sso1.example.com"), | ||
| utils.WithRealm("EXAMPLE.LOCAL"), | ||
| utils.WithFilename("./temp-sso1.keytab"), | ||
| utils.WithPairs(utils.EncryptTypePair{ | ||
| Version: 2, | ||
| EncryptType: 18, | ||
| CreateTime: time.Now(), | ||
| }), | ||
| ) | ||
| if err != nil { | ||
| log.Fatalf("create mock keytab error: %v", err) | ||
| } | ||
| defer clean() | ||
| keytabLookup, err := spnego.NewKeytabFileLookupFunc("./temp-sso1.keytab") | ||
| if err != nil { | ||
| panic(fmt.Errorf("create keytab lookup function failed: %w", err)) | ||
| } | ||
| authMiddleware, err := v3.NewSpnegoKrb5AuthenticateMiddleware(spnego.Config{ | ||
| KeytabLookup: keytabLookup, | ||
| }) | ||
| if err != nil { | ||
| panic(fmt.Errorf("create spnego middleware failed: %w", err)) | ||
| } | ||
| // Apply the middleware to protected routes | ||
| app.Use("/protected", authMiddleware) | ||
|
|
||
| // Access authenticated identity | ||
| app.Get("/protected/resource", func(c fiber.Ctx) error { | ||
| identity, ok := spnego.GetAuthenticatedIdentityFromContext(c) | ||
| if !ok { | ||
| return c.Status(fiber.StatusUnauthorized).SendString("Unauthorized") | ||
| } | ||
| return c.SendString(fmt.Sprintf("Hello, %s!", identity.UserName())) | ||
| }) | ||
| log.Info("Server is running on :3000") | ||
| go func() { | ||
| <-time.After(time.Second * 1) | ||
| fmt.Println("use curl -kv --negotiate http://sso.example.local:3000/protected/resource") | ||
| fmt.Println("if response is 401, execute `klist` to check use kerberos session") | ||
| <-time.After(time.Second * 2) | ||
| fmt.Println("close server") | ||
| if err = app.Shutdown(); err != nil { | ||
| panic(fmt.Errorf("shutdown server failed: %w", err)) | ||
| } | ||
| }() | ||
| if err := app.Listen("sso.example.local:3000"); err != nil { | ||
| panic(fmt.Errorf("start server failed: %w", err)) | ||
| } | ||
|
|
||
| // Output: Server is running on :3000 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Example will fail as a doc test (“Output: …”) and/or hang CI.
- The function is an Example with an
Output:comment, sogo testwill run it and compare stdout. However:- It starts a server and prints additional lines via
fmt.Println, which won’t match the single-line expected output. log.Info(Fiber logger) may not write to stdout in a waygo testcaptures.- Binding to
sso.example.local:3000can fail name resolution in CI.
Fix by removing theOutput:directive (examples without Output are compiled but not run), or simplify to a non-network example that only prints the expected line.
- It starts a server and prints additional lines via
Minimal safe fix (don’t execute the example in tests):
- // Output: Server is running on :3000
+ // (no Output: directive on purpose; this example should compile but not run during `go test`)Optional follow-ups:
- Change the listen address to
:3000orlocalhost:3000. - If you prefer keeping an executable example, remove the server code and just
fmt.Println("Server is running on :3000")to satisfy theOutput:check.
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
if err := app.Listen("sso.example.local:3000"); err != nil {
panic(fmt.Errorf("start server failed: %w", err))
}
// (no Output: directive on purpose; this example should compile but not run during `go test`)
}
🤖 Prompt for AI Agents
In spnego/example/example.go around lines 14-69 the Example function starts a
real server, does network calls and prints multiple lines but also includes an
`// Output: Server is running on :3000` doctest directive which makes `go test`
try to run and match stdout (causing failures/hangs in CI); fix by removing the
`Output:` directive so the example is compiled but not executed by tests, or
alternatively convert the example to a non-network doc example by removing
server startup and ancillary prints and leaving a single `fmt.Println("Server is
running on :3000")` that matches the `Output:` line (if you keep an Output,
ensure only that exact line is printed), and optionally change the listen
address to `:3000` or `localhost:3000` if you intend to run network code
locally.
| func TestGetAndSetAuthenticatedIdentityFromContextForFiberV2(t *testing.T) { | ||
| app := fiberV2.New() | ||
| id := goidentity.NewUser("[email protected]") | ||
| app.Use("/identity", func(ctx *fiberV2.Ctx) error { | ||
| SetAuthenticatedIdentityToContext(ctx, &id) | ||
| return ctx.Next() | ||
| }) | ||
| app.Get("/test", func(ctx *fiberV2.Ctx) error { | ||
| _, ok := GetAuthenticatedIdentityFromContext(ctx) | ||
| require.False(t, ok) | ||
| return ctx.SendStatus(fiberV2.StatusOK) | ||
| }) | ||
| app.Get("/identity/test", func(ctx *fiberV2.Ctx) error { | ||
| user, ok := GetAuthenticatedIdentityFromContext(ctx) | ||
| require.True(t, ok) | ||
| require.Equal(t, id.UserName(), user.UserName()) | ||
| require.Equal(t, id.Domain(), user.Domain()) | ||
| return ctx.SendStatus(fiberV2.StatusOK) | ||
| }) | ||
| app.Test(httptest.NewRequest("GET", "/test", nil)) | ||
| app.Test(httptest.NewRequest("GET", "/identity/test", nil)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Assert app.Test errors and status codes; close bodies; make test parallel.
Tightens the contract and avoids leaks. Using require inside handlers is fine, but still verify the outer request completed successfully.
func TestGetAndSetAuthenticatedIdentityFromContextForFiberV2(t *testing.T) {
+ t.Parallel()
@@
- app.Test(httptest.NewRequest("GET", "/test", nil))
- app.Test(httptest.NewRequest("GET", "/identity/test", nil))
+ resp1, err := app.Test(httptest.NewRequest("GET", "/test", nil))
+ require.NoError(t, err)
+ defer resp1.Body.Close()
+ require.Equal(t, fiberV2.StatusOK, resp1.StatusCode)
+
+ resp2, err := app.Test(httptest.NewRequest("GET", "/identity/test", nil))
+ require.NoError(t, err)
+ defer resp2.Body.Close()
+ require.Equal(t, fiberV2.StatusOK, resp2.StatusCode)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestGetAndSetAuthenticatedIdentityFromContextForFiberV2(t *testing.T) { | |
| app := fiberV2.New() | |
| id := goidentity.NewUser("[email protected]") | |
| app.Use("/identity", func(ctx *fiberV2.Ctx) error { | |
| SetAuthenticatedIdentityToContext(ctx, &id) | |
| return ctx.Next() | |
| }) | |
| app.Get("/test", func(ctx *fiberV2.Ctx) error { | |
| _, ok := GetAuthenticatedIdentityFromContext(ctx) | |
| require.False(t, ok) | |
| return ctx.SendStatus(fiberV2.StatusOK) | |
| }) | |
| app.Get("/identity/test", func(ctx *fiberV2.Ctx) error { | |
| user, ok := GetAuthenticatedIdentityFromContext(ctx) | |
| require.True(t, ok) | |
| require.Equal(t, id.UserName(), user.UserName()) | |
| require.Equal(t, id.Domain(), user.Domain()) | |
| return ctx.SendStatus(fiberV2.StatusOK) | |
| }) | |
| app.Test(httptest.NewRequest("GET", "/test", nil)) | |
| app.Test(httptest.NewRequest("GET", "/identity/test", nil)) | |
| } | |
| func TestGetAndSetAuthenticatedIdentityFromContextForFiberV2(t *testing.T) { | |
| t.Parallel() | |
| app := fiberV2.New() | |
| id := goidentity.NewUser("[email protected]") | |
| app.Use("/identity", func(ctx *fiberV2.Ctx) error { | |
| SetAuthenticatedIdentityToContext(ctx, &id) | |
| return ctx.Next() | |
| }) | |
| app.Get("/test", func(ctx *fiberV2.Ctx) error { | |
| _, ok := GetAuthenticatedIdentityFromContext(ctx) | |
| require.False(t, ok) | |
| return ctx.SendStatus(fiberV2.StatusOK) | |
| }) | |
| app.Get("/identity/test", func(ctx *fiberV2.Ctx) error { | |
| user, ok := GetAuthenticatedIdentityFromContext(ctx) | |
| require.True(t, ok) | |
| require.Equal(t, id.UserName(), user.UserName()) | |
| require.Equal(t, id.Domain(), user.Domain()) | |
| return ctx.SendStatus(fiberV2.StatusOK) | |
| }) | |
| resp1, err := app.Test(httptest.NewRequest("GET", "/test", nil)) | |
| require.NoError(t, err) | |
| defer resp1.Body.Close() | |
| require.Equal(t, fiberV2.StatusOK, resp1.StatusCode) | |
| resp2, err := app.Test(httptest.NewRequest("GET", "/identity/test", nil)) | |
| require.NoError(t, err) | |
| defer resp2.Body.Close() | |
| require.Equal(t, fiberV2.StatusOK, resp2.StatusCode) | |
| } |
🤖 Prompt for AI Agents
In spnego/identity_test.go around lines 13 to 34, make the test parallel and
assert the results of app.Test calls: add t.Parallel() at the top of the test,
capture the returned (resp, err) for each app.Test call and use
require.NoError(t, err) and require.Equal(t, fiberV2.StatusOK, resp.StatusCode)
to verify success, and ensure you close each response body (e.g., defer
resp.Body.Close() or resp.Body.Close() after assertions) to avoid leaks; keep
the existing require assertions inside handlers as-is.
| func TestGetAndSetAuthenticatedIdentityFromContextForFiberV3(t *testing.T) { | ||
| app := fiberV3.New() | ||
| id := goidentity.NewUser("[email protected]") | ||
| app.Use("/identity", func(ctx fiberV3.Ctx) error { | ||
| SetAuthenticatedIdentityToContext(ctx, &id) | ||
| return ctx.Next() | ||
| }) | ||
| app.Get("/test", func(ctx fiberV3.Ctx) error { | ||
| _, ok := GetAuthenticatedIdentityFromContext(ctx) | ||
| require.False(t, ok) | ||
| return ctx.SendStatus(fiberV3.StatusOK) | ||
| }) | ||
| app.Get("/identity/test", func(ctx fiberV3.Ctx) error { | ||
| user, ok := GetAuthenticatedIdentityFromContext(ctx) | ||
| require.True(t, ok) | ||
| require.Equal(t, id.UserName(), user.UserName()) | ||
| require.Equal(t, id.Domain(), user.Domain()) | ||
| return ctx.SendStatus(fiberV3.StatusOK) | ||
| }) | ||
| app.Test(httptest.NewRequest("GET", "/test", nil)) | ||
| app.Test(httptest.NewRequest("GET", "/identity/test", nil)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mirror the same robustness for Fiber v3 test.
Parallelize and verify responses.
func TestGetAndSetAuthenticatedIdentityFromContextForFiberV3(t *testing.T) {
+ t.Parallel()
@@
- app.Test(httptest.NewRequest("GET", "/test", nil))
- app.Test(httptest.NewRequest("GET", "/identity/test", nil))
+ resp1, err := app.Test(httptest.NewRequest("GET", "/test", nil))
+ require.NoError(t, err)
+ defer resp1.Body.Close()
+ require.Equal(t, fiberV3.StatusOK, resp1.StatusCode)
+
+ resp2, err := app.Test(httptest.NewRequest("GET", "/identity/test", nil))
+ require.NoError(t, err)
+ defer resp2.Body.Close()
+ require.Equal(t, fiberV3.StatusOK, resp2.StatusCode)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestGetAndSetAuthenticatedIdentityFromContextForFiberV3(t *testing.T) { | |
| app := fiberV3.New() | |
| id := goidentity.NewUser("[email protected]") | |
| app.Use("/identity", func(ctx fiberV3.Ctx) error { | |
| SetAuthenticatedIdentityToContext(ctx, &id) | |
| return ctx.Next() | |
| }) | |
| app.Get("/test", func(ctx fiberV3.Ctx) error { | |
| _, ok := GetAuthenticatedIdentityFromContext(ctx) | |
| require.False(t, ok) | |
| return ctx.SendStatus(fiberV3.StatusOK) | |
| }) | |
| app.Get("/identity/test", func(ctx fiberV3.Ctx) error { | |
| user, ok := GetAuthenticatedIdentityFromContext(ctx) | |
| require.True(t, ok) | |
| require.Equal(t, id.UserName(), user.UserName()) | |
| require.Equal(t, id.Domain(), user.Domain()) | |
| return ctx.SendStatus(fiberV3.StatusOK) | |
| }) | |
| app.Test(httptest.NewRequest("GET", "/test", nil)) | |
| app.Test(httptest.NewRequest("GET", "/identity/test", nil)) | |
| } | |
| func TestGetAndSetAuthenticatedIdentityFromContextForFiberV3(t *testing.T) { | |
| t.Parallel() | |
| app := fiberV3.New() | |
| id := goidentity.NewUser("[email protected]") | |
| app.Use("/identity", func(ctx fiberV3.Ctx) error { | |
| SetAuthenticatedIdentityToContext(ctx, &id) | |
| return ctx.Next() | |
| }) | |
| app.Get("/test", func(ctx fiberV3.Ctx) error { | |
| _, ok := GetAuthenticatedIdentityFromContext(ctx) | |
| require.False(t, ok) | |
| return ctx.SendStatus(fiberV3.StatusOK) | |
| }) | |
| app.Get("/identity/test", func(ctx fiberV3.Ctx) error { | |
| user, ok := GetAuthenticatedIdentityFromContext(ctx) | |
| require.True(t, ok) | |
| require.Equal(t, id.UserName(), user.UserName()) | |
| require.Equal(t, id.Domain(), user.Domain()) | |
| return ctx.SendStatus(fiberV3.StatusOK) | |
| }) | |
| resp1, err := app.Test(httptest.NewRequest("GET", "/test", nil)) | |
| require.NoError(t, err) | |
| defer resp1.Body.Close() | |
| require.Equal(t, fiberV3.StatusOK, resp1.StatusCode) | |
| resp2, err := app.Test(httptest.NewRequest("GET", "/identity/test", nil)) | |
| require.NoError(t, err) | |
| defer resp2.Body.Close() | |
| require.Equal(t, fiberV3.StatusOK, resp2.StatusCode) | |
| } |
🤖 Prompt for AI Agents
In spnego/identity_test.go around lines 36 to 57, the Fiber v3 test should
mirror the more robust Fiber v2 test by running subtests in parallel and
asserting responses; add t.Parallel() at the start of the test, invoke app.Test
for "/test" and "/identity/test" in separate goroutines or use subtests with
t.Run(..., func(t *testing.T){ t.Parallel() ... }), capture and require.NoError
on the returned responses and errors, assert resp.StatusCode == fiberV3.StatusOK
for both requests, and ensure resp.Body is closed after reads; this will
parallelize execution and verify the responses rather than calling app.Test
without checks.
| func SetAuthenticatedIdentityToContext[T FiberContext](ctx T, identity goidentity.Identity) { | ||
| ctx.Locals(contextKeyOfIdentity, identity) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify context key definition; otherwise these helpers won’t compile.
Both helpers rely on an unexported contextKeyOfIdentity. Confirm it exists in package spnego and isn’t typed as a plain string to avoid collisions.
Script to confirm presence once in this package:
If missing, add:
// prefer an unexported, unique key type to avoid collisions with user code
type contextKey string
var contextKeyOfIdentity contextKey = "spnego.identity"🏁 Script executed:
#!/bin/bash
rg -nP '^\s*(type\s+contextKey\b|var\s+contextKeyOfIdentity\b|const\s+contextKeyOfIdentity\b)' spnego -C2Length of output: 436
Ensure a unique, typed context key to avoid collisions
The contextKeyOfIdentity constant is defined in spnego/config.go:11, but it’s an untyped string, which can collide with other context keys. Update it to use a dedicated key type:
• File: spnego/config.go (around line 11)
- Current:
// contextKeyOfIdentity is the key used to store the authenticated identity in the Fiber context const contextKeyOfIdentity = "middleware.spnego.Identity"
- Replace with a private
contextKeytype and a typed variable:
// prefer an unexported, unique key type to avoid collisions with user code
+type contextKey string
-// contextKeyOfIdentity is the key used to store the authenticated identity in the Fiber context
-const contextKeyOfIdentity = "middleware.spnego.Identity"
+var contextKeyOfIdentity contextKey = "middleware.spnego.Identity"This ensures that only values using your contextKey type can collide, preventing accidental overlaps with other packages’ string-based keys.
🤖 Prompt for AI Agents
In spnego/config.go around line 11, replace the untyped string constant
contextKeyOfIdentity with a private typed context key to avoid collisions:
declare a new private type (e.g. type contextKey string) and then define a
package-level variable of that type for the identity key (e.g. var
contextKeyOfIdentity = contextKey("middleware.spnego.Identity")), then update
uses (like in spnego/identity.go lines 12-14) to continue using the variable;
ensure the new type and variable are unexported so only your package can use
that key.
| for item := range maps.Values(keytabMap) { | ||
| mk = append(mk, item) | ||
| } |
There was a problem hiding this comment.
Fix: range over values (not indices) when converting map to slice
maps.Values returns a slice of values; ranging with a single iteration variable yields the index (int), which then fails to append to MultiKeytabInfo. Use for _, item := range ... to iterate values.
- for item := range maps.Values(keytabMap) {
- mk = append(mk, item)
- }
+ for _, item := range maps.Values(keytabMap) {
+ mk = append(mk, item)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for item := range maps.Values(keytabMap) { | |
| mk = append(mk, item) | |
| } | |
| for _, item := range maps.Values(keytabMap) { | |
| mk = append(mk, item) | |
| } |
🤖 Prompt for AI Agents
In spnego/utils/keytab.go around lines 70 to 72, the loop uses "for item :=
range maps.Values(keytabMap)" which iterates indices (ints) rather than values;
change the loop to use a two-variable range ("for _, item := range
maps.Values(keytabMap)") so that the actual values are appended to mk
(MultiKeytabInfo) instead of indices, ensuring types match and the slice
contains the map values.
| t.Run("test file created", func(t *testing.T) { | ||
| defaultFileOperator = myFileOperator{} | ||
| tm := time.Now() | ||
| _, clean, err := NewMockKeytab( | ||
| WithPrincipal("HTTP/sso.example.com"), | ||
| WithRealm("TEST.LOCAL"), | ||
| WithPairs(EncryptTypePair{ | ||
| Version: 3, | ||
| EncryptType: 18, | ||
| CreateTime: tm, | ||
| }), | ||
| WithFilename("./temp.keytab"), | ||
| ) | ||
| require.NoError(t, err) | ||
| t.Cleanup(clean) | ||
| require.FileExists(t, "./temp.keytab") | ||
| kt, err := keytab.Load("./temp.keytab") | ||
| require.NoError(t, err) | ||
| _, kv, err := kt.GetEncryptionKey(types.NewPrincipalName(1, "HTTP/sso.example.com"), "TEST.LOCAL", 3, 18) | ||
| require.NoError(t, err) | ||
| require.Equal(t, 3, kv) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use secure permission and temp path in the positive case too.
Ensure the production path uses mode 0o600 (change in helper required) and avoid ./temp.keytab.
- defaultFileOperator = myFileOperator{}
+ prev := defaultFileOperator
+ defaultFileOperator = myFileOperator{}
+ t.Cleanup(func() { defaultFileOperator = prev })
...
- WithFilename("./temp.keytab"),
+ dir := t.TempDir()
+ WithFilename(filepath.Join(dir, "temp.keytab")),And in spnego/utils/mock_keytab.go (outside this file):
- defaultFileOperator.OpenFile(opt.Filename, os.O_RDWR|os.O_CREATE, 0o666)
+ defaultFileOperator.OpenFile(opt.Filename, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o600)Committable suggestion skipped: line range outside the PR's diff.
spnego/utils/mock_keytab.go
Outdated
| file, err := defaultFileOperator.OpenFile(opt.Filename, os.O_RDWR|os.O_CREATE, 0o666) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("error opening file: %w", err) | ||
| } | ||
| clean = func() { | ||
| _ = defaultFileOperator.Remove(opt.Filename) | ||
| } | ||
| if _, err = kt.Write(file); err != nil { | ||
| clean() | ||
| return nil, nil, fmt.Errorf("error writing to file: %w", err) | ||
| } | ||
| _ = file.Close() | ||
| return kt, clean, nil |
There was a problem hiding this comment.
Close-before-remove and use O_TRUNC for safe, cross-platform cleanup
On write error you remove the file while it may still be open; this can fail on Windows. Also, opening without O_TRUNC may leave stale bytes from a previous run if the new keytab is shorter.
- file, err := defaultFileOperator.OpenFile(opt.Filename, os.O_RDWR|os.O_CREATE, 0o666)
+ file, err := defaultFileOperator.OpenFile(opt.Filename, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o666)
if err != nil {
return nil, nil, fmt.Errorf("error opening file: %w", err)
}
- clean = func() {
- _ = defaultFileOperator.Remove(opt.Filename)
- }
- if _, err = kt.Write(file); err != nil {
- clean()
- return nil, nil, fmt.Errorf("error writing to file: %w", err)
- }
- _ = file.Close()
+ // cleanup closes and removes the file; safe to call multiple times
+ clean = func() {
+ _ = file.Close()
+ _ = defaultFileOperator.Remove(opt.Filename)
+ }
+ if _, err = kt.Write(file); err != nil {
+ clean() // ensures close-before-remove
+ return nil, nil, fmt.Errorf("error writing to file: %w", err)
+ }
+ // normal path: close the file; leaving remove to the returned cleanup
+ _ = file.Close()
return kt, clean, nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| file, err := defaultFileOperator.OpenFile(opt.Filename, os.O_RDWR|os.O_CREATE, 0o666) | |
| if err != nil { | |
| return nil, nil, fmt.Errorf("error opening file: %w", err) | |
| } | |
| clean = func() { | |
| _ = defaultFileOperator.Remove(opt.Filename) | |
| } | |
| if _, err = kt.Write(file); err != nil { | |
| clean() | |
| return nil, nil, fmt.Errorf("error writing to file: %w", err) | |
| } | |
| _ = file.Close() | |
| return kt, clean, nil | |
| file, err := defaultFileOperator.OpenFile(opt.Filename, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o666) | |
| if err != nil { | |
| return nil, nil, fmt.Errorf("error opening file: %w", err) | |
| } | |
| // cleanup closes and removes the file; safe to call multiple times | |
| clean = func() { | |
| _ = file.Close() | |
| _ = defaultFileOperator.Remove(opt.Filename) | |
| } | |
| if _, err = kt.Write(file); err != nil { | |
| clean() // ensures close-before-remove | |
| return nil, nil, fmt.Errorf("error writing to file: %w", err) | |
| } | |
| // normal path: close the file; leaving remove to the returned cleanup | |
| _ = file.Close() | |
| return kt, clean, nil |
🤖 Prompt for AI Agents
In spnego/utils/mock_keytab.go around lines 135 to 147, opening the temp keytab
uses os.O_RDWR|os.O_CREATE (missing O_TRUNC) which can leave stale bytes, and on
write error the cleanup removes the file while it may still be open (problematic
on Windows). Fix by opening the file with os.O_TRUNC in addition to the existing
flags so the file is truncated on open; on write error call file.Close()
(checking/ignoring its error) before calling the cleanup Remove; also ensure the
normal return path closes the file before returning and that the cleanup closure
only removes the file (not close it).
spnego/v3/spnego_test.go
Outdated
| tm := time.Now() | ||
| _, clean1, err1 := utils.NewMockKeytab( | ||
| utils.WithPrincipal("HTTP/sso1.example.com"), | ||
| utils.WithRealm("EXAMPLE.LOCAL"), | ||
| utils.WithFilename("./temp-sso1.keytab"), | ||
| utils.WithPairs(utils.EncryptTypePair{ | ||
| Version: 2, | ||
| EncryptType: 18, | ||
| CreateTime: tm, | ||
| }), | ||
| ) | ||
| require.NoError(t, err1) | ||
| t.Cleanup(clean1) | ||
| _, clean2, err2 := utils.NewMockKeytab( | ||
| utils.WithPrincipal("HTTP/sso2.example.com"), | ||
| utils.WithRealm("EXAMPLE.LOCAL"), | ||
| utils.WithFilename("./temp-sso2.keytab"), | ||
| utils.WithPairs(utils.EncryptTypePair{ | ||
| Version: 2, | ||
| EncryptType: 18, | ||
| CreateTime: tm, | ||
| }), | ||
| ) | ||
| require.NoError(t, err2) | ||
| t.Cleanup(clean2) | ||
| lookupFunc, err := spnego.NewKeytabFileLookupFunc("./temp-sso1.keytab", "./temp-sso2.keytab") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Secure keytab files and avoid hardcoded paths.
Persisting keytabs as ./temp-*.keytab and the helper’s 0o666 mode are not appropriate for secret material, even in tests. Use t.TempDir + filepath.Join and ensure creation mode 0o600 in utils.NewMockKeytab.
- utils.WithFilename("./temp-sso1.keytab"),
+ dir := t.TempDir()
+ utils.WithFilename(filepath.Join(dir, "sso1.keytab")),
...
- utils.WithFilename("./temp-sso2.keytab"),
+ utils.WithFilename(filepath.Join(dir, "sso2.keytab")),And update spnego/utils/mock_keytab.go (outside this file):
- file, err := defaultFileOperator.OpenFile(opt.Filename, os.O_RDWR|os.O_CREATE, 0o666)
+ file, err := defaultFileOperator.OpenFile(opt.Filename, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o600)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In spnego/v3/spnego_test.go around lines 43 to 68, the test writes keytab files
to hardcoded ./temp-*.keytab with permissive 0o666 mode; change the test to
create files inside t.TempDir() (use filepath.Join(t.TempDir(),
"temp-sso1.keytab") etc.) and pass those paths into NewMockKeytab; also update
utils.NewMockKeytab implementation (spnego/utils/mock_keytab.go) to create
keytab files with mode 0o600 (owner-read/write only) instead of 0o666 so test
keytabs are created with secure permissions.
test: Update test cases to use temporary directory instead of current directory refactor: Remove unused config package
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
spnego/utils/mock_keytab.go (1)
135-147: Close-before-remove on write error; tighten perms; open WRONLY (Windows-safe and credential-safe)
On write error the file is removed while still open; this can fail on Windows and leak a descriptor. Also, using 0o666 for keytab files is overly permissive for credential material; prefer 0o600. Opening WRONLY clarifies intent.Apply this diff:
- file, err := defaultFileOperator.OpenFile(opt.Filename, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o666) + file, err := defaultFileOperator.OpenFile(opt.Filename, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600) if err != nil { return nil, nil, fmt.Errorf("error opening file: %w", err) } - clean = func() { - _ = defaultFileOperator.Remove(opt.Filename) - } + // cleanup closes and removes the file; safe to call multiple times + clean = func() { + _ = file.Close() + _ = defaultFileOperator.Remove(opt.Filename) + } if _, err = kt.Write(file); err != nil { - clean() + clean() // ensures close-before-remove on error (important on Windows) return nil, nil, fmt.Errorf("error writing to file: %w", err) } _ = file.Close() return kt, clean, nilOptional verification to catch other overly-permissive creations:
#!/bin/bash # Find any file creations with 0o666 perms or OpenFile calls using O_RDWR where write-only suffices. rg -nP '0o666\b' -C2 rg -nP 'OpenFile\([^,]+,\s*os\.O_RDWR\b' -C2
🧹 Nitpick comments (3)
spnego/utils/mock_keytab.go (3)
126-133: Validate required fields when adding entriesIf Pairs is non-empty but PrincipalName is empty, gokrb5 may accept an invalid principal or error in surprising ways. Fail fast with a clear error.
Apply this diff:
kt := keytab.New() var err error + if len(opt.Pairs) > 0 && opt.PrincipalName == "" { + return nil, nil, fmt.Errorf("principal name must be set when adding keytab entries") + } for _, pair := range opt.Pairs {Optionally, add a unit test asserting that NewMockKeytab(WithPairs(...)) without WithPrincipal returns an error.
129-131: Wrap underlying error with %w for consistencyUse %w so callers can use errors.Is / errors.As.
Apply this diff:
- return nil, nil, fmt.Errorf("error adding entry: %v", err) + return nil, nil, fmt.Errorf("error adding entry: %w", err)
92-103: Minor naming nit: clarify file operator implementation
Rename myFileOperator to osFileOperator to reflect it delegates to the os package.Apply this diff:
-type myFileOperator struct{} +type osFileOperator struct{} -func (m myFileOperator) OpenFile(filename string, flag int, perm os.FileMode) (*os.File, error) { +func (m osFileOperator) OpenFile(filename string, flag int, perm os.FileMode) (*os.File, error) { return os.OpenFile(filename, flag, perm) } -func (m myFileOperator) Remove(filename string) error { +func (m osFileOperator) Remove(filename string) error { return os.Remove(filename) } -var defaultFileOperator fileOperator = myFileOperator{} +var defaultFileOperator fileOperator = osFileOperator{}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
spnego/utils/mock_keytab.go(1 hunks)spnego/utils/mock_keytab_test.go(1 hunks)spnego/v2/spnego_test.go(1 hunks)spnego/v3/spnego_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- spnego/utils/mock_keytab_test.go
- spnego/v3/spnego_test.go
- spnego/v2/spnego_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
spnego/utils/mock_keytab.go (1)
spnego/utils/keytab.go (1)
EncryptTypePair(22-26)
🔇 Additional comments (1)
spnego/utils/mock_keytab.go (1)
10-19: Good use of the Options pattern and solid documentationClear option constructors, defaults, and comments. This makes the helper flexible and easy to use in tests.
Also applies to: 21-29, 30-76
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
spnego/utils/mock_keytab.go (1)
135-148: Harden file creation: WRONLY, 0600 perms, idempotent cleanup, and fsync
- Open with O_WRONLY (not O_RDWR) since we don't read; keep O_TRUNC.
- Use 0o600 for credential-like files (even in tests) to avoid permissive modes under loose umasks.
- Make cleanup close+remove to be idempotent and safe if called on error or success.
- fsync before close to reduce flakiness on slow CI filesystems.
- file, err := defaultFileOperator.OpenFile(opt.Filename, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o666) + // Write-only, create, and truncate with restrictive perms. + file, err := defaultFileOperator.OpenFile(opt.Filename, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600) if err != nil { return nil, nil, fmt.Errorf("error opening file: %w", err) } - clean = func() { - _ = defaultFileOperator.Remove(opt.Filename) - } + // Idempotent cleanup: close then remove; safe to call multiple times. + clean = func() { + _ = file.Close() + _ = defaultFileOperator.Remove(opt.Filename) + } if _, err = kt.Write(file); err != nil { - file.Close() - clean() + clean() // ensures close-before-remove on error return nil, nil, fmt.Errorf("error writing to file: %w", err) } - _ = file.Close() + // Best effort flush then close; leave remove to returned cleanup. + if err := file.Sync(); err != nil { + clean() + return nil, nil, fmt.Errorf("error syncing file: %w", err) + } + _ = file.Close() return kt, clean, nilNote: You already addressed O_TRUNC and close-before-remove per the earlier review; this refines flags, perms, and idempotency.
🧹 Nitpick comments (3)
spnego/utils/mock_keytab.go (3)
124-132: Add basic option validation to prevent unusable keytabsGuard against missing principal/realm and optionally empty pairs to avoid surprising runtime errors.
Apply this diff to introduce validation right after applying options:
func NewMockKeytab(opts ...MockOption) (*keytab.Keytab, func(), error) { opt := newDefaultMockOptions() opt.apply(opts...) + if opt.PrincipalName == "" { + return nil, nil, fmt.Errorf("principal name is required; set WithPrincipal(...)") + } + if opt.Realm == "" { + return nil, nil, fmt.Errorf("realm is required; set WithRealm(...)") + } + // If an empty keytab is not desirable for your tests, uncomment the guard below: + // if len(opt.Pairs) == 0 { + // return nil, nil, fmt.Errorf("at least one EncryptTypePair is required; set WithPairs(...)") + // } kt := keytab.New() var err error for _, pair := range opt.Pairs {If you intentionally rely on empty pairs in some tests, keep the third check commented or toggle it via an option.
128-131: Use %w for proper error wrappingPreserve the original error in the chain for callers using errors.Is/As.
- if err = kt.AddEntry(opt.PrincipalName, opt.Realm, opt.Password, pair.CreateTime, pair.Version, pair.EncryptType); err != nil { - return nil, nil, fmt.Errorf("error adding entry: %v", err) + if err = kt.AddEntry(opt.PrincipalName, opt.Realm, opt.Password, pair.CreateTime, pair.Version, pair.EncryptType); err != nil { + return nil, nil, fmt.Errorf("error adding entry: %w", err) }
92-103: Name nit: myFileOperator → osFileOperator; consider exposing a setter for testsMinor clarity nit on the concrete type name. Also, external tests (utils_test package) can’t mutate unexported vars; a tiny setter keeps the seam while maintaining encapsulation.
-type myFileOperator struct{} +type osFileOperator struct{} -func (m myFileOperator) OpenFile(filename string, flag int, perm os.FileMode) (*os.File, error) { +func (m osFileOperator) OpenFile(filename string, flag int, perm os.FileMode) (*os.File, error) { return os.OpenFile(filename, flag, perm) } -func (m myFileOperator) Remove(filename string) error { +func (m osFileOperator) Remove(filename string) error { return os.Remove(filename) } -var defaultFileOperator fileOperator = myFileOperator{} +var defaultFileOperator fileOperator = osFileOperator{}Optionally add (outside this hunk):
// SetFileOperator allows tests in external packages to inject a fake FS. func SetFileOperator(op fileOperator) { if op != nil { defaultFileOperator = op } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
spnego/utils/mock_keytab.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
spnego/utils/mock_keytab.go (1)
spnego/utils/keytab.go (1)
EncryptTypePair(22-26)
🔇 Additional comments (2)
spnego/utils/mock_keytab.go (2)
30-76: LGTM: clean, idiomatic option pattern with helpful docsThe option functions are straightforward and composable; docs make intended usage obvious.
104-123: File-backed path testing confirmedThe
WithFilenameoption is exercised in multiple test suites covering both successful and failure scenarios:
Happy paths in
spnego/v3/spnego_test.go(lines 50–52, 62–64)spnego/utils/mock_keytab_test.go(lines 119–121)spnego/config_test.go(lines 38–40, 66–68, 90–92, 106–108)spnego/v2/spnego_test.go(lines 50–52, 62–64)Failure paths in
spnego/utils/mock_keytab_test.go(lines 84–86 expectingos.ErrPermission; lines 103–105 expectingos.ErrClosed)No additional tests are needed for the file-backed keytab path.
2 participants
Description
This PR adds enterprise-grade SPNEGO authentication middleware to Fiber, providing Kerberos-based single sign-on (SSO) capabilities. The implementation follows Fiber's middleware patterns with dynamic keytab loading and full RFC compliance.
Related Issue
Resolves #1367
Issue: SPNEGO Middleware Proposal
Technical Implementation
WWW-Authenticatechallenges andAuthorizationvalidationVerification