Skip to content

Conversation

@omertuc
Copy link
Contributor

@omertuc omertuc commented Oct 1, 2025

Description

  • Move existing docs to docs/auth.md
  • Expand with more details

Type of change

  • Refactor
  • New feature
  • Bug fix
  • CVE fix
  • Optimization
  • Documentation Update
  • Configuration Update
  • Bump-up service version
  • Bump-up dependent library
  • Bump-up library or tool used for development (does not change the final image)
  • CI configuration change
  • Konflux configuration change
  • Unit tests improvement
  • Integration tests improvement
  • End to end tests improvement

Related Tickets & Documents

  • Related Issue #

  • Closes #

LCORE-597

Checklist before requesting a review

  • I have performed a self-review of my code.
  • PR has passed all pre-merge test jobs.
  • If it is a core feature, I have added thorough tests.

Testing

  • Please provide detailed steps to perform tests related to this code change.
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

No tests needed

Summary by CodeRabbit

  • Documentation
    • Consolidated authentication content by replacing in-file subsections with a single link to dedicated authentication and authorization documentation.
    • Added comprehensive auth docs covering available methods (no-op, token-based, Kubernetes, JWK), configuration guidance, required permissions, and validation flows.
    • Documented authorization model, role resolution, role-rule syntax and operators, and access rules with supported actions.
    • Included example configurations to help set up authentication and authorization end-to-end.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 1, 2025

Walkthrough

Consolidates authentication content by removing detailed README subsections and linking to a new docs/auth.md. Adds a comprehensive authentication and authorization guide covering modules (noop, noop-with-token, k8s, jwk-token), configuration, token validation flows, role and access rules, and example snippets.

Changes

Cohort / File(s) Summary
README authentication consolidation
README.md
Removed in-README authentication subsections (K8s, JWK, No-op) and replaced with a single link to docs/auth.md.
New auth/authorization documentation
docs/auth.md
Added detailed documentation for auth modules, Kubernetes and JWK validation flows, role and access rule systems, supported operators, and configuration examples.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User as Client
  participant Svc as Service
  participant Auth as Auth Module
  participant K8s as Kubernetes API
  participant AuthZ as Authorization

  Note over Svc,Auth: Kubernetes-based authentication (k8s)
  User->>Svc: Request + Bearer token (SA token)
  Svc->>Auth: Validate token (k8s)
  Auth->>K8s: TokenReview / SubjectAccessReview
  K8s-->>Auth: Validation result (+subject)
  Auth-->>Svc: Identity (subject), roles (if applicable)
  Svc->>AuthZ: Check access (action, roles)
  AuthZ-->>Svc: Allow / Deny
  Svc-->>User: Response (data or 403)
Loading
sequenceDiagram
  autonumber
  actor User as Client
  participant Svc as Service
  participant Auth as Auth Module
  participant JWK as JWK Provider
  participant AuthZ as Authorization

  rect rgba(230,240,255,0.5)
  Note over Svc,Auth: JWK-based authentication (jwk-token)
  User->>Svc: Request + JWT
  Svc->>Auth: Validate JWT
  Auth->>JWK: Fetch/refresh JWK Set (JWKS)
  JWK-->>Auth: Keys
  Auth-->>Svc: Claims, derived roles
  end

  Svc->>AuthZ: Evaluate role rules + access rules
  AuthZ-->>Svc: Allow / Deny
  Svc-->>User: Response (data or 403)
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • tisnik

Poem

A nibble of docs, a hop to auth-land,
README trimmed, a guide close at hand.
Tokens and claims in a carrot-bright queue,
Roles get mapped—permission comes through.
JWKS whispers, K8s nods along—
Thump-thump! says the rabbit: “Security strong.” 🥕🐇

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title “LCORE-597: Document auth/authz” succinctly captures the primary change of this pull request, which is to move and expand authentication and authorization documentation into docs/auth.md, and it is clear and specific enough for a teammate to understand the main update.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

🔧 markdownlint-cli2 (0.18.1)
README.md

/usr/bin/env: 'node': Permission denied

docs/auth.md

/usr/bin/env: 'node': Permission denied


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.

- Move existing docs to docs/auth.md
- Expand with more details
Copy link
Contributor

@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: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b5eb048 and 3448fe8.

📒 Files selected for processing (2)
  • README.md (1 hunks)
  • docs/auth.md (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build-pr
  • GitHub Check: e2e_tests

Comment on lines +101 to +103
k8s_ca_cert_path: /var/run/secrets/kubernetes.io/serviceaccount/ca.crt # optional, will be auto-detected
skip_tls_verification: false # optional, insecure
```
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Fix misleading TLS verification note.

The sample sets skip_tls_verification: false but labels that setting “insecure.” false is the safe option (TLS verification on); only true is insecure. Please adjust the comment so readers don’t disable verification by mistake.

-  skip_tls_verification: false  # optional, insecure
+  skip_tls_verification: false  # optional; set to true only if you must skip TLS verification (insecure)
📝 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
k8s_ca_cert_path: /var/run/secrets/kubernetes.io/serviceaccount/ca.crt # optional, will be auto-detected
skip_tls_verification: false # optional, insecure
```
k8s_ca_cert_path: /var/run/secrets/kubernetes.io/serviceaccount/ca.crt # optional, will be auto-detected
skip_tls_verification: false # optional; set to true only if you must skip TLS verification (insecure)
🤖 Prompt for AI Agents
In docs/auth.md around lines 101 to 103, the inline comment mislabels the TLS
verification setting: it marks `skip_tls_verification: false` as "insecure" even
though `false` is the safe option (verification enabled); update the comment so
it correctly states that `false` is secure (TLS verification on) and that only
`true` is insecure (disables verification), e.g., change the phrase to something
like "optional, secure when false; set to true to disable TLS verification
(insecure)" so readers aren't misled.

Copy link
Contributor

@tisnik tisnik left a comment

Choose a reason for hiding this comment

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

LGTM

@tisnik tisnik merged commit 1d7fa44 into lightspeed-core:main Oct 2, 2025
18 of 19 checks passed
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