Added thunder oauth & flow libraries#1871
Open
anushasunkada wants to merge 3 commits into
Open
Conversation
Signed-off-by: anushasunkada <anushasunkada@gmail.com>
Signed-off-by: anushasunkada <anushasunkada@gmail.com>
Signed-off-by: anushasunkada <anushasunkada@gmail.com>
WalkthroughThis PR integrates optional Thunder OAuth/OIDC embedding into the eSignet service. The service now conditionally routes to either a Thunder embed server or its existing Chi HTTP stack based on the ChangesThunder OAuth/OIDC Embed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@esignet-service/cmd/esignet/main.go`:
- Line 113: The code creates a fresh shutdown context using shutCtx :=
context.Background() which prevents external cancellation during the
ShutdownTimeout window; add a concise comment next to shutCtx that documents
this intentional choice (reference shutCtx, context.Background(), and
ShutdownTimeout) explaining that we intentionally avoid deriving from the
signal-cancelled ctx so a second SIGTERM won't shorten the graceful shutdown
period and to make the rationale explicit for future readers.
- Around line 68-71: The pingers map is created before confirming that
dependencies are fully initialized; update the code so the map is populated only
after successful initialization checks (or add an inline comment documenting the
assumption). Specifically, either move the construction of pingers (variable
pingers of type map[string]handler.Pinger) to after the existing
initialization/validation of postgres and redisClient (references: postgres,
redisClient, handler.Pinger) or add a clear comment next to the pingers
declaration stating it assumes postgres and redisClient were successfully
initialized and that any runtime ping failures will be surfaced by the health
check logic.
In `@esignet-service/config.yaml.example`:
- Around line 1-17: The README header wrongly implies YAML file loading via
CONFIG_FILE but the actual loader is config.go:Load(), which only calls
envconfig.Process() and never reads files; either remove/clarify the
CONFIG_FILE/YAML references in the example config so it’s clearly a
human-readable reference (no file support), or implement YAML loading before
envconfig.Process() by reading CONFIG_FILE (e.g., use gopkg.in/yaml.v3 to
unmarshal into the same config struct) and then let envconfig.Process() override
values from the environment; update the header comments to match the chosen
approach and mention CONFIG_FILE only if YAML support is implemented.
In `@esignet-service/internal/authn/provider.go`:
- Around line 21-31: Replace the blank identifiers in the Provider stub methods
with real parameter names for clarity: in Provider.Authenticate change "_
context.Context, _, _ map[string]interface{}, _ *authnprovider.AuthnMetadata" to
names such as "ctx context.Context, clientIDAndSecret map[string]interface{},
metadata *authnprovider.AuthnMetadata" (or similar clear names), and do the same
for the other stub method in this file (the second Provider method around the
subsequent stub) using appropriate names like "ctx", "token" or "credentials",
and "metadata"; keep signatures unchanged otherwise so linters may still warn
but the code is self-documenting.
In `@esignet-service/internal/thunderembed/server.go`:
- Line 22: The field shutdownTimeout currently stores a full config.ServerConfig
but only the ShutdownTimeout value is used; change the server struct's
shutdownTimeout to type time.Duration (instead of config.ServerConfig), update
the constructor (e.g., NewServer or wherever the struct is created) to accept or
extract cfg.ShutdownTimeout and assign that duration, and leave all usages
(e.g., the shutdown logic that reads shutdownTimeout) unchanged but expecting a
time.Duration; this removes unnecessary coupling to config.ServerConfig.
- Line 37: The call to strings.TrimSpace is redundant because cfg.Thunder.Home
is already trimmed upstream; update the embed.WireThunder invocation in
server.go to pass cfg.Thunder.Home directly (i.e., replace
strings.TrimSpace(cfg.Thunder.Home) with cfg.Thunder.Home) while leaving mux and
authn.New() unchanged so embed.WireThunder(mux, cfg.Thunder.Home, authn.New())
is used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7b682e8f-9ccc-4493-9bfd-f2edef2b651c
⛔ Files ignored due to path filters (1)
esignet-service/go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
esignet-service/README.mdesignet-service/cmd/esignet/main.goesignet-service/config.yaml.exampleesignet-service/go.modesignet-service/internal/authn/provider.goesignet-service/internal/cache/redis.goesignet-service/internal/config/config.goesignet-service/internal/config/config_test.goesignet-service/internal/db/postgres_test.goesignet-service/internal/middleware/middleware_test.goesignet-service/internal/server/server.goesignet-service/internal/server/server_test.goesignet-service/internal/thunderembed/server.goesignet-service/pkg/logger/logger_test.go
Comment on lines
+68
to
+71
| pingers := map[string]handler.Pinger{ | ||
| "postgres": postgres, | ||
| "redis": redisClient, | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider defensive initialization ordering.
The pingers map is constructed before verifying that dependencies are fully initialized for health checks. If either postgres.Ping() or redisClient.Ping() would fail during health checks due to post-initialization issues, the map still references them.
This is acceptable given that initialization errors are already caught at lines 49-66, but consider documenting that pingers assumes successful initialization.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@esignet-service/cmd/esignet/main.go` around lines 68 - 71, The pingers map is
created before confirming that dependencies are fully initialized; update the
code so the map is populated only after successful initialization checks (or add
an inline comment documenting the assumption). Specifically, either move the
construction of pingers (variable pingers of type map[string]handler.Pinger) to
after the existing initialization/validation of postgres and redisClient
(references: postgres, redisClient, handler.Pinger) or add a clear comment next
to the pingers declaration stating it assumes postgres and redisClient were
successfully initialized and that any runtime ping failures will be surfaced by
the health check logic.
| if err := srv.Shutdown(context.Background()); err != nil { | ||
| log.Error("graceful shutdown failed", slog.String("error", err.Error())) | ||
| os.Exit(1) | ||
| shutCtx := context.Background() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Document the shutdown context choice.
Using context.Background() here means graceful shutdown cannot be externally cancelled (e.g., a second SIGTERM won't interrupt the drain). This appears intentional to ensure the full ShutdownTimeout window is honored, but the choice is subtle.
Consider adding a comment explaining why a fresh context is used rather than deriving from the signal-cancelled ctx.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@esignet-service/cmd/esignet/main.go` at line 113, The code creates a fresh
shutdown context using shutCtx := context.Background() which prevents external
cancellation during the ShutdownTimeout window; add a concise comment next to
shutCtx that documents this intentional choice (reference shutCtx,
context.Background(), and ShutdownTimeout) explaining that we intentionally
avoid deriving from the signal-cancelled ctx so a second SIGTERM won't shorten
the graceful shutdown period and to make the rationale explicit for future
readers.
Comment on lines
+1
to
+17
| # ───────────────────────────────────────────────────────────────────────────── | ||
| # esignet-service YAML configuration | ||
| # | ||
| # Copy this file to config.yaml (or any path) and set CONFIG_FILE to point at | ||
| # it. Values here are defaults; any matching environment variable always wins. | ||
| # | ||
| # Load order (lowest → highest priority): | ||
| # 1. Struct defaults (hard-coded in the binary) | ||
| # 2. This YAML file | ||
| # 3. Environment variables | ||
| # | ||
| # Usage: | ||
| # CONFIG_FILE=/etc/esignet/config.yaml ./esignet | ||
| # CONFIG_FILE=config.yaml make dev | ||
| # | ||
| # Tip: commit a config.yaml for local development; never commit secrets. | ||
| # ───────────────────────────────────────────────────────────────────────────── |
There was a problem hiding this comment.
Remove or clarify misleading YAML loading documentation.
The header comments suggest using CONFIG_FILE environment variable and describe a YAML load order (lines 4, 7-10, 13-14), but config.go:Load() only calls envconfig.Process(), which does not load YAML files. The service reads configuration exclusively from environment variables.
This will confuse operators who expect CONFIG_FILE=config.yaml to work.
Options to fix:
-
Remove YAML support claims (simplest): Delete references to CONFIG_FILE and YAML loading; document this as a reference template showing default values in YAML format for human readability only.
-
Implement YAML loading (if actually desired): Add a YAML parsing library (e.g.,
gopkg.in/yaml.v3) and load the file referenced byCONFIG_FILEbefore callingenvconfig.Process(), allowing environment variables to override YAML values.
📝 Option 1: Clarify as reference-only
# ─────────────────────────────────────────────────────────────────────────────
# esignet-service YAML configuration
#
-# Copy this file to config.yaml (or any path) and set CONFIG_FILE to point at
-# it. Values here are defaults; any matching environment variable always wins.
+# This file documents the configuration structure and default values for
+# reference. The service reads configuration from ENVIRONMENT VARIABLES only.
#
-# Load order (lowest → highest priority):
-# 1. Struct defaults (hard-coded in the binary)
-# 2. This YAML file
-# 3. Environment variables
+# Use .env.example to see the environment variable names. Each YAML key below
+# corresponds to an environment variable (e.g., server.port → PORT).
#
-# Usage:
-# CONFIG_FILE=/etc/esignet/config.yaml ./esignet
-# CONFIG_FILE=config.yaml make dev
-#
-# Tip: commit a config.yaml for local development; never commit secrets.
+# Tip: use this as a reference when setting environment variables.
# ─────────────────────────────────────────────────────────────────────────────📝 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.
Suggested change
| # ───────────────────────────────────────────────────────────────────────────── | |
| # esignet-service YAML configuration | |
| # | |
| # Copy this file to config.yaml (or any path) and set CONFIG_FILE to point at | |
| # it. Values here are defaults; any matching environment variable always wins. | |
| # | |
| # Load order (lowest → highest priority): | |
| # 1. Struct defaults (hard-coded in the binary) | |
| # 2. This YAML file | |
| # 3. Environment variables | |
| # | |
| # Usage: | |
| # CONFIG_FILE=/etc/esignet/config.yaml ./esignet | |
| # CONFIG_FILE=config.yaml make dev | |
| # | |
| # Tip: commit a config.yaml for local development; never commit secrets. | |
| # ───────────────────────────────────────────────────────────────────────────── | |
| # ───────────────────────────────────────────────────────────────────────────── | |
| # esignet-service YAML configuration | |
| # | |
| # This file documents the configuration structure and default values for | |
| # reference. The service reads configuration from ENVIRONMENT VARIABLES only. | |
| # | |
| # Use .env.example to see the environment variable names. Each YAML key below | |
| # corresponds to an environment variable (e.g., server.port → PORT). | |
| # | |
| # Tip: use this as a reference when setting environment variables. | |
| # ───────────────────────────────────────────────────────────────────────────── |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@esignet-service/config.yaml.example` around lines 1 - 17, The README header
wrongly implies YAML file loading via CONFIG_FILE but the actual loader is
config.go:Load(), which only calls envconfig.Process() and never reads files;
either remove/clarify the CONFIG_FILE/YAML references in the example config so
it’s clearly a human-readable reference (no file support), or implement YAML
loading before envconfig.Process() by reading CONFIG_FILE (e.g., use
gopkg.in/yaml.v3 to unmarshal into the same config struct) and then let
envconfig.Process() override values from the environment; update the header
comments to match the chosen approach and mention CONFIG_FILE only if YAML
support is implemented.
Comment on lines
+21
to
+31
| func (p *Provider) Authenticate( | ||
| _ context.Context, | ||
| _, _ map[string]interface{}, | ||
| _ *authnprovider.AuthnMetadata, | ||
| ) (*authnprovider.AuthnResult, *authnprovider.ServiceError) { | ||
| return nil, authnprovider.NewClientError( | ||
| authnprovider.ErrorCodeNotImplemented, | ||
| "authentication not implemented", | ||
| "replace internal/authn.Provider with MOSIP integration", | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider retaining parameter names for documentation clarity.
Both stub methods use blank identifiers for all parameters. While this is valid for unused parameters, keeping the actual parameter names (e.g., ctx, clientIDAndSecret, metadata) would improve code documentation and make future implementation easier.
📝 Example with retained parameter names
func (p *Provider) Authenticate(
- _ context.Context,
- _, _ map[string]interface{},
- _ *authnprovider.AuthnMetadata,
+ ctx context.Context,
+ clientIDAndSecret, transactionData map[string]interface{},
+ metadata *authnprovider.AuthnMetadata,
) (*authnprovider.AuthnResult, *authnprovider.ServiceError) {
+ _, _, _, _ = ctx, clientIDAndSecret, transactionData, metadata // mark as intentionally unused
return nil, authnprovider.NewClientError(Or simply accept that linters may warn about unused parameters in stub code.
Also applies to: 34-45
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@esignet-service/internal/authn/provider.go` around lines 21 - 31, Replace the
blank identifiers in the Provider stub methods with real parameter names for
clarity: in Provider.Authenticate change "_ context.Context, _, _
map[string]interface{}, _ *authnprovider.AuthnMetadata" to names such as "ctx
context.Context, clientIDAndSecret map[string]interface{}, metadata
*authnprovider.AuthnMetadata" (or similar clear names), and do the same for the
other stub method in this file (the second Provider method around the subsequent
stub) using appropriate names like "ctx", "token" or "credentials", and
"metadata"; keep signatures unchanged otherwise so linters may still warn but
the code is self-documenting.
| type Server struct { | ||
| http *http.Server | ||
| log *slog.Logger | ||
| shutdownTimeout config.ServerConfig |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Store only the required timeout duration.
The shutdownTimeout field stores the entire config.ServerConfig struct but only uses ShutdownTimeout in line 69. This wastes memory and couples the server to unnecessary configuration fields.
♻️ Proposed fix to store only the duration
type Server struct {
http *http.Server
log *slog.Logger
- shutdownTimeout config.ServerConfig
+ shutdownTimeout time.Duration
}Then update the constructor and usage:
return &Server{
http: httpSrv,
log: log,
- shutdownTimeout: cfg.Server,
+ shutdownTimeout: cfg.Server.ShutdownTimeout,
}, nil func (s *Server) Shutdown(ctx context.Context) error {
s.log.Info("graceful shutdown initiated")
- shutCtx, cancel := context.WithTimeout(ctx, s.shutdownTimeout.ShutdownTimeout)
+ shutCtx, cancel := context.WithTimeout(ctx, s.shutdownTimeout)
defer cancel()📝 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.
Suggested change
| shutdownTimeout config.ServerConfig | |
| type Server struct { | |
| http *http.Server | |
| log *slog.Logger | |
| shutdownTimeout time.Duration | |
| } |
Suggested change
| shutdownTimeout config.ServerConfig | |
| return &Server{ | |
| http: httpSrv, | |
| log: log, | |
| shutdownTimeout: cfg.Server.ShutdownTimeout, | |
| }, nil |
Suggested change
| shutdownTimeout config.ServerConfig | |
| func (s *Server) Shutdown(ctx context.Context) error { | |
| s.log.Info("graceful shutdown initiated") | |
| shutCtx, cancel := context.WithTimeout(ctx, s.shutdownTimeout) | |
| defer cancel() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@esignet-service/internal/thunderembed/server.go` at line 22, The field
shutdownTimeout currently stores a full config.ServerConfig but only the
ShutdownTimeout value is used; change the server struct's shutdownTimeout to
type time.Duration (instead of config.ServerConfig), update the constructor
(e.g., NewServer or wherever the struct is created) to accept or extract
cfg.ShutdownTimeout and assign that duration, and leave all usages (e.g., the
shutdown logic that reads shutdownTimeout) unchanged but expecting a
time.Duration; this removes unnecessary coupling to config.ServerConfig.
|
|
||
| mux.HandleFunc("GET /health", handler.HealthHandler(log, pingers)) | ||
|
|
||
| if err := embed.WireThunder(mux, strings.TrimSpace(cfg.Thunder.Home), authn.New()); err != nil { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Remove redundant TrimSpace call.
The cfg.Thunder.Home value is already trimmed in main.go at line 74 before the conditional check. Trimming again here is unnecessary.
♻️ Proposed fix
- if err := embed.WireThunder(mux, strings.TrimSpace(cfg.Thunder.Home), authn.New()); err != nil {
+ if err := embed.WireThunder(mux, cfg.Thunder.Home, authn.New()); err != nil {
return nil, fmt.Errorf("thunder embed: %w", err)
}📝 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.
Suggested change
| if err := embed.WireThunder(mux, strings.TrimSpace(cfg.Thunder.Home), authn.New()); err != nil { | |
| if err := embed.WireThunder(mux, cfg.Thunder.Home, authn.New()); err != nil { | |
| return nil, fmt.Errorf("thunder embed: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@esignet-service/internal/thunderembed/server.go` at line 37, The call to
strings.TrimSpace is redundant because cfg.Thunder.Home is already trimmed
upstream; update the embed.WireThunder invocation in server.go to pass
cfg.Thunder.Home directly (i.e., replace strings.TrimSpace(cfg.Thunder.Home)
with cfg.Thunder.Home) while leaving mux and authn.New() unchanged so
embed.WireThunder(mux, cfg.Thunder.Home, authn.New()) is used.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores