elastic / elastic/beats

[refactor-opportunist] Extract shared JSON file auth helper for CEL and HTTPJSON OAuth2 configs

Open
#49,783 1 comment 0 reactions 0 assignees View on GitHub
needs_team
Dominant language
Go
Stars
12.7k
Forks
5k
Avg merge
2d 15m
Merged PRs (30d)
385

Description

## 🏗️ Refactor Proposal

**Summary:** Consolidate duplicated OAuth2 JSON-file loading logic used by both CEL and HTTPJSON inputs into a shared internal helper package, then continue extracting the remaining shared OAuth2/Okta auth core.

## Problem
Both inputs carry duplicated auth plumbing, which increases parallel maintenance cost and cross-input drift risk.

Concrete duplication examples:
- `x-pack/filebeat/input/cel/config_auth.go:186` and `x-pack/filebeat/input/httpjson/config_auth.go:128` define the same `oAuth2Provider` abstraction.
- `x-pack/filebeat/input/cel/config_auth.go:420` and `x-pack/filebeat/input/httpjson/config_auth.go:371` both implement `validateOktaProvider`.
- `x-pack/filebeat/input/cel/config_okta_auth.go:43` and `x-pack/filebeat/input/httpjson/config_okta_auth.go:43` both implement `fetchOktaOauthClient`.
- Prior to this proof slice, both files had near-identical `populateJSONFromFile` implementations.

Coupling/churn signal:
- In the last 60 days, **5 commits** touched both `x-pack/filebeat/input/cel/` and `x-pack/filebeat/input/httpjson/` (computed from `git log --since="60 days ago" --name-only`).

This is structural (not cosmetic): auth behavior and validation logic must stay consistent across two high-churn inputs but currently lives in duplicated module-local implementations.

## Proposed Approach
Introduce a shared internal auth package (`x-pack/filebeat/input/internal/httpauth`) and progressively move duplicated OAuth2/Okta helpers there.

What changes:
- Common auth utility code moves to one package and is consumed by both inputs.

What stays the same:
- Input-level config schema, public behavior, and error messages.
- Existing call sites keep their local function signatures during migration.

Why this is better:
- One implementation point for shared auth mechanics.
- Lower chance of drift/bug fixes landing in only one input.
- Incremental migration path with low risk.

## Proof of Concept
I partially implemented this refactor on one representative slice to verify viability:

**Files changed:**
- `x-pack/filebeat/input/internal/httpauth/jsonfile.go` (new)
- `x-pack/filebeat/input/cel/config_auth.go`
- `x-pack/filebeat/input/httpjson/config_auth.go`

**Before → After:**
- Before: both CEL and HTTPJSON each defined their own `populateJSONFromFile` body (same `os.Stat`/`os.ReadFile`/`json.Valid` flow and error strings).
- After: shared implementation in `x-pack/filebeat/input/internal/httpauth/jsonfile.go:15-33` and both call sites delegate:
- `x-pack/filebeat/input/cel/config_auth.go:457-459`
- `x-pack/filebeat/input/httpjson/config_auth.go:408-410`

## Verification
- Baseline (before change):
- `go test ./x-pack/filebeat/input/cel ./x-pack/filebeat/input/httpjson` ✅
- After PoC change:
- `go test ./x-pack/filebeat/input/cel ./x-pack/filebeat/input/httpjson ./x-pack/filebeat/input/internal/httpauth` ✅
- Existing tests still pass for both impacted input packages.

## Incremental Rollout Plan
This refactor can be completed incrementally:
1. **Done (proved):** extract shared JSON file-loading helper and delegate from both inputs.
2. Extract shared OAuth2 provider canonicalization and provider-specific validation helpers (Azure/Google/Okta) into `internal/httpauth`.
3. Extract shared Okta token/JWT exchange client logic from `config_okta_auth.go` in both inputs; keep input-specific differences behind thin adapters.

## Risks and Mitigations
- **Risk:** subtle behavior drift during extraction.
- **Mitigation:** keep wrapper functions at current call sites and preserve existing error strings/validation order, with targeted package tests on every step.
- **Risk:** migration scope grows too large.
- **Mitigation:** merge in small slices (one helper family at a time), each with green tests before moving to next extraction.

## Evidence
- Duplicate definitions and logic locations listed above (`cel` and `httpjson` auth files).
- Churn/coupling signal from last 60 days (`git log --since="60 days ago"`).
- Passing test runs before and after the PoC extraction.

> [!NOTE]
>
> 🔒 Integrity filtering filtered 27 items
>
> Integrity filtering activated and filtered the following items during workflow execution.
> This happens when a tool call accesses a resource that does not meet the required integrity or secrecy level of the workflow.
>
> - [#28365](https://github.com/elastic/beats/pull/28365) (`search_pull_requests`: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".)
> - [#25000](https://github.com/elastic/beats/pull/25000) (`search_pull_requests`: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".)
> - [#28546](https://github.com/elastic/beats/pull/28546) (`search_pull_requests`: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".)
> - [#28292](https://github.com/elastic/beats/pull/28292) (`search_pull_requests`: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".)
> - [#5408](https://github.com/elastic/beats/pull/5408) (`search_pull_requests`: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".)
> - [#17653](https://github.com/elastic/beats/pull/17653) (`search_pull_requests`: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".)
> - [#14010](https://github.com/elastic/beats/pull/14010) (`search_pull_requests`: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".)
> - [#14165](https://github.com/elastic/beats/pull/14165) (`search_pull_requests`: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".)
> - [#14080](https://github.com/elastic/beats/pull/14080) (`search_pull_requests`: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".)
> - [#937](https://github.com/elastic/beats/pull/937) (`search_pull_requests`: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".)
> - [#1069](https://github.com/elastic/beats/pull/1069) (`search_pull_requests`: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".)
> - [#44212](https://github.com/elastic/beats/issues/44212) (`search_issues`: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".)
> - [#40748](https://github.com/elastic/beats/issues/40748) (`search_issues`: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".)
> - [#37678](https://github.com/elastic/beats/issues/37678) (`search_issues`: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".)
> - [#10238](https://github.com/elastic/beats/issues/10238) (`search_issues`: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".)
> - [#29694](https://github.com/elastic/beats/issues/29694) (`search_issues`: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".)
> - ... and 11 more items
>
>

---
[What is this?](https://ela.st/github-ai-tools) | [From workflow: Refactor Opportunist](https://github.com/elastic/beats/actions/runs/23750328646)

Give us feedback! React with 🚀 if perfect, 👍 if helpful, 👎 if not.

Contributor guide

Open the contributing guide

Research direction

Start with x-pack/filebeat/input/cel/config_auth.go and config_okta_auth.go, then compare the corresponding files under x-pack/filebeat/input/httpjson/. Read the existing internal/httpauth/jsonfile.go extraction and run the CEL, HTTPJSON, and internal/httpauth Go tests. Done means the remaining shared OAuth2 and Okta helpers are migrated while preserving call signatures, behavior, error strings, and passing tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
authentication, backend, security
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
50/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.