Skip to content

fix(auth): allow auth module config to be injected#538

Merged
laouji merged 1 commit into
mainfrom
EN-518-allow-auth-module-config-to-be-injected
Jan 7, 2026
Merged

fix(auth): allow auth module config to be injected#538
laouji merged 1 commit into
mainfrom
EN-518-allow-auth-module-config-to-be-injected

Conversation

@laouji
Copy link
Copy Markdown
Contributor

@laouji laouji commented Jan 7, 2026

Fixes: EN-518

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jan 7, 2026

Walkthrough

The authentication module's factory functions are refactored for improved modularity. defaultModuleConfig is renamed to the public ModuleConfigFromFlags, now capturing additional flag values (CheckScopes, Service). A new ModuleOptions function separates option construction from module assembly, while KeySet and Authenticator provisioning are updated to conditionally short-circuit based on the Enabled flag in ModuleConfig.

Changes

Cohort / File(s) Summary
CLI Factory Functions
auth/cli.go
Renamed defaultModuleConfig to public ModuleConfigFromFlags; enhanced to collect AuthCheckScopesFlag and AuthServiceFlag values; returns enriched ModuleConfig with CheckScopes, Service, and initialized AdditionalChecks; updated three call sites to use the new function.
Module Assembly & Provisioning
auth/module.go
Introduced new exported ModuleOptions() []fx.Option to build FX options; refactored Module(cfg) to delegate to ModuleOptions(); updated KeySet provisioning to short-circuit to static key set when cfg.Enabled is false; updated Authenticator provisioning to return NoAuth when cfg.Enabled is false, otherwise returns JWT-based authenticator.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A factory renamed, flags now collected,
Modularity improved and well-perfected!
When Enabled is false, shortcuts do gleam,
Building options apart—a refactoring dream!
Static key sets hop in, NoAuth takes the lead,
Graceful degradation is all that we need! 🎉

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: refactoring to allow auth module config injection, which aligns with the core modifications in both cli.go and module.go files.
Description check ✅ Passed The description references issue EN-518 and relates to the changeset's objective of allowing auth module configuration to be injected, matching the PR's intent.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch EN-518-allow-auth-module-config-to-be-injected

Warning

Review ran into problems

🔥 Problems

Errors were encountered while retrieving linked issues.

Errors (1)
  • EN-518: Authentication required, not authenticated - You need to authenticate to access this operation.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@laouji laouji marked this pull request as ready for review January 7, 2026 15:47
@laouji laouji requested a review from a team as a code owner January 7, 2026 15:47
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 7, 2026

Codecov Report

❌ Patch coverage is 77.77778% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 29.14%. Comparing base (7fedd92) to head (5ffaa20).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
auth/cli.go 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #538      +/-   ##
==========================================
+ Coverage   29.11%   29.14%   +0.03%     
==========================================
  Files         166      166              
  Lines        6711     6714       +3     
==========================================
+ Hits         1954     1957       +3     
  Misses       4640     4640              
  Partials      117      117              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
auth/module.go (1)

37-56: Validate required configuration when enabled.

When cfg.Enabled is true but required fields like cfg.Issuer are empty, the OIDC discovery call at Line 46 will fail with a potentially unclear error. Consider adding explicit validation before attempting discovery.

✅ Add validation for required fields
 		fx.Provide(func(cfg ModuleConfig, httpClient *http.Client) (oidc.KeySet, error) {
 			if !cfg.Enabled {
 				// this won't be used by the NoAuth
 				return oidc.NewStaticKeySet(), nil
 			}
+			if cfg.Issuer == "" {
+				return nil, fmt.Errorf("auth issuer is required when auth is enabled")
+			}
 			retryableHttpClient := retryablehttp.NewClient()
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 7fedd92 and 5ffaa20.

📒 Files selected for processing (2)
  • auth/cli.go
  • auth/module.go
🧰 Additional context used
🧬 Code graph analysis (1)
auth/module.go (3)
oidc/keyset.go (2)
  • KeySet (27-30)
  • NewStaticKeySet (128-130)
auth/middleware.go (1)
  • Authenticator (11-13)
auth/no_auth.go (1)
  • NewNoAuth (11-13)
🔇 Additional comments (4)
auth/module.go (2)

24-30: LGTM! Clean separation of concerns.

The refactoring successfully separates option construction (ModuleOptions()) from module assembly, enabling external config injection while maintaining shared option logic.


60-72: LGTM! Correct conditional authentication provisioning.

The authenticator correctly returns NoAuth when disabled and JWT-based auth when enabled, properly utilizing all configuration fields including CheckScopes and Service.

auth/cli.go (2)

25-40: LGTM! Clean flag-to-config translation.

The exported ModuleConfigFromFlags function properly populates all ModuleConfig fields from CLI flags, including the newly captured CheckScopes and Service fields.


42-56: LGTM! Consistent usage of the new API.

All call sites correctly use ModuleConfigFromFlags(cmd) to populate configuration before passing it to Module(). The AdditionalChecks append pattern is preserved correctly in both organization-aware and custom-checks variants.

@laouji laouji added this pull request to the merge queue Jan 7, 2026
Merged via the queue into main with commit d042fba Jan 7, 2026
7 checks passed
@laouji laouji deleted the EN-518-allow-auth-module-config-to-be-injected branch January 7, 2026 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants