Skip to content

Conversation

@mms-gianni
Copy link
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Template (non-breaking change which adds a template)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • I've built the image and tested it on a kubernetes cluster

Test Configuration:

  • Operator Version:
  • Kubernetes Version:
  • Kubero CLI Version (if applicable):

Checklist:

  • I removed unnecessary debug logs
  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I documented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

@mms-gianni mms-gianni requested a review from Copilot July 15, 2025 06:12
@mms-gianni mms-gianni self-assigned this Jul 15, 2025
@mms-gianni mms-gianni added the feature New feature or request label Jul 15, 2025
@github-actions
Copy link

github-actions bot commented Jul 15, 2025

Lines Statements Branches Functions
Coverage: 83%
83.2% (12419/14925) 70.51% (598/848) 73.64% (327/444)
Tests Skipped Failures Errors Time
417 0 💤 0 ❌ 0 🔥 43.112s ⏱️

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enforces a read-only mode guard across new write routes and cleans up related authentication code, while also introducing a demo login prompt in the client and updating environment hints.

  • Add ReadonlyGuard to all write endpoints in users, tokens, roles, and groups controllers
  • Deprecate and update ReadonlyGuard behavior; remove legacy RBAC bypass logic in JWT and permissions guards
  • Update .env.template comments and enhance login prompt with demo credentials

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
server/src/users/users.controller.ts Imported and applied ReadonlyGuard to write routes
server/src/token/token.controller.ts Imported and applied ReadonlyGuard to write routes
server/src/roles/roles.controller.ts Imported and applied ReadonlyGuard to write routes
server/src/groups/groups.controller.ts Imported and applied ReadonlyGuard to write routes
server/src/common/guards/readonly.guard.ts Marked ReadonlyGuard as deprecated and added additional warning message
server/src/auth/strategies/jwt.guard.ts Commented out legacy RBAC skip logic
server/src/auth/permissions.guard.ts Commented out legacy RBAC skip logic
server/.env.template Clarified KUBERO_PROMETHEUS_ENDPOINT settings for local development
client/src/components/loginprompt.vue Added demo domain alert and auto-filled demo credentials
server/src/deployments/deployments.service.spec.ts.old Added legacy test file still suffixed .old
Comments suppressed due to low confidence (4)

server/src/common/guards/readonly.guard.ts:21

  • HTTP 202 (Accepted) is not typically used for blocking requests; consider returning 403 Forbidden to indicate the operation is disallowed.
      throw new HttpException('Kubero is in read-only mode', 202);

server/src/deployments/deployments.service.spec.ts.old:1

  • This test file ends with .spec.ts.old which may prevent it from running; consider renaming or removing the legacy .old suffix.
import { Test, TestingModule } from '@nestjs/testing';

server/.env.template:33

KUBERO_PROMETHEUS_ENDPOINT=http:/127.0.0.1:8080 # for local development

client/src/components/loginprompt.vue:129

  • [nitpick] Computed properties should be pure and without side effects; moving assignment logic into a watcher or lifecycle hook would be more maintainable.
                this.username = 'demo';

Comment on lines +23 to +31
/* Disabling for RBAC
if (
!process.env.KUBERO_USERS &&
!process.env.GITHUB_CLIENT_SECRET &&
!process.env.OAUTH2_CLIENT_SECRET
) {
return true;
}
*/
Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

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

[nitpick] Rather than leaving large commented-out blocks, consider removing this legacy code to keep the guard implementation clean.

Suggested change
/* Disabling for RBAC
if (
!process.env.KUBERO_USERS &&
!process.env.GITHUB_CLIENT_SECRET &&
!process.env.OAUTH2_CLIENT_SECRET
) {
return true;
}
*/
// Removed legacy code for RBAC disabling.

Copilot uses AI. Check for mistakes.
Comment on lines +14 to +23
/* Disabling for RBAC
if (
!process.env.KUBERO_USERS &&
!process.env.GITHUB_CLIENT_SECRET &&
!process.env.OAUTH2_CLIENT_SECRET
) {
return true;
}
*/

Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

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

[nitpick] This commented-out RBAC fallback adds noise; removing deprecated logic would improve readability.

Suggested change
/* Disabling for RBAC
if (
!process.env.KUBERO_USERS &&
!process.env.GITHUB_CLIENT_SECRET &&
!process.env.OAUTH2_CLIENT_SECRET
) {
return true;
}
*/

Copilot uses AI. Check for mistakes.
@mms-gianni mms-gianni merged commit 741c324 into main Jul 15, 2025
3 checks passed
@mms-gianni mms-gianni deleted the feature/apply-readonly-guard-to-new-routes branch September 3, 2025 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants