Skip to content

Add /configs/agent API to manage tenant agent configuration#1143

Merged
hwupathum merged 7 commits into
wso2:masterfrom
RovinKYK:consent_fix
Jul 2, 2026
Merged

Add /configs/agent API to manage tenant agent configuration#1143
hwupathum merged 7 commits into
wso2:masterfrom
RovinKYK:consent_fix

Conversation

@RovinKYK

@RovinKYK RovinKYK commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Purpose

Exposes a new server-configuration sub-resource, /configs/agent, that lets a tenant administrator manage agent settings. This is the API-server counterpart to the agent-config management service on the OAuth side (AgentConfigMgtService), and backs the "agents externally managed" toggle used during the authorize / token flows.

Related to: wso2/product-is#28110

What changed

New endpoints under /configs/agent:

Method Operation Description
GET getAgentConfiguration Retrieve the current tenant agent configuration.
PATCH patchAgentConfiguration Update the configuration via JSON Patch (e.g. /agentsExternallyManaged).
DELETE deleteAgentConfiguration Revert the tenant agent configuration to its default.

Supporting changes:

  • New models AgentConfiguration and AgentConfigPatch.
  • ServerConfigManagementServicegetAgentConfiguration / patchAgentConfiguration / deleteAgentConfiguration, delegating to the OAuth AgentConfigMgtService.
  • Wiring in ConfigsServiceHolder, Constants, and ServerConfigManagementServiceFactory.
  • Generated delegate/interface (ConfigsApi, ConfigsApiService) and configs.yaml spec updated.

Copilot AI review requested due to automatic review settings June 24, 2026 13:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a06bfaaa-7e33-4712-be9a-ac4537cc0f84

📥 Commits

Reviewing files that changed from the base of the PR and between 53bf66b and 183afe3.

⛔ Files ignored due to path filters (1)
  • components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/model/AgentConfigPatch.java is excluded by !**/gen/**
📒 Files selected for processing (2)
  • components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/core/ServerConfigManagementService.java
  • components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/resources/configs.yaml
💤 Files with no reviewable changes (1)
  • components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/resources/configs.yaml
👮 Files not reviewed due to content moderation or server errors (1)
  • components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/core/ServerConfigManagementService.java

📝 Walkthrough

A detailed high-level summary could not be generated for this review. Here is an overview derived from the analyzed file changes:

  • components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.common/src/main/java/org/wso2/carbon/identity/api/server/configs/common/ConfigsServiceHolder.java: ## AI-generated summary of changes
  • components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.common/src/main/java/org/wso2/carbon/identity/api/server/configs/common/Constants.java: ## AI-generated summary of changes
  • components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/core/ServerConfigManagementService.java: ## AI-generated summary of changes
  • components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/factories/ServerConfigManagementServiceFactory.java: ## AI-generated summary of changes
  • components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/impl/ConfigsApiServiceImpl.java: ## AI-generated summary of changes
  • components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/resources/configs.yaml: ## AI-generated summary of changes
  • pom.xml: ## AI-generated summary of changes
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description covers purpose and changed endpoints, but misses most required template sections like Goals, Approach, tests, and release note. Add the missing template sections with concise details for goals, approach, user stories, checklist, release note, docs, tests, and other required fields.
Docstring Coverage ⚠️ Warning Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: adding a /configs/agent API for tenant agent configuration.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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
`@components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.common/src/main/java/org/wso2/carbon/identity/api/server/configs/common/Constants.java`:
- Around line 224-232: The new agent configuration entries in the ErrorMessage
enum reuse existing error codes, causing duplicate CNF identifiers. Update
ERROR_CODE_AGENT_CONFIG_RETRIEVE, ERROR_CODE_AGENT_CONFIG_UPDATE, and
ERROR_CODE_AGENT_CONFIG_DELETE in Constants.ErrorMessage to use new unused
numeric codes that do not overlap with the consent mapping entries, keeping the
enum values unique and traceable.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2a328954-ec0b-4379-a044-1fdd7e502b09

📥 Commits

Reviewing files that changed from the base of the PR and between 5d086f7 and 7e811f8.

⛔ Files ignored due to path filters (4)
  • components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/ConfigsApi.java is excluded by !**/gen/**
  • components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/ConfigsApiService.java is excluded by !**/gen/**
  • components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/model/AgentConfigPatch.java is excluded by !**/gen/**
  • components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/gen/java/org/wso2/carbon/identity/api/server/configs/v1/model/AgentConfiguration.java is excluded by !**/gen/**
📒 Files selected for processing (6)
  • components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.common/src/main/java/org/wso2/carbon/identity/api/server/configs/common/ConfigsServiceHolder.java
  • components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.common/src/main/java/org/wso2/carbon/identity/api/server/configs/common/Constants.java
  • components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/core/ServerConfigManagementService.java
  • components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/factories/ServerConfigManagementServiceFactory.java
  • components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/impl/ConfigsApiServiceImpl.java
  • components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/resources/configs.yaml

@jenkins-is-staging

Copy link
Copy Markdown

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/28493333049

@jenkins-is-staging

Copy link
Copy Markdown

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/28493333049
Status: failure

@jenkins-is-staging

Copy link
Copy Markdown

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/28497337221

@sahandilshan

Copy link
Copy Markdown
Contributor

@coderabbitai can you review this

@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== ConfigsServiceHolder.java ==="
fd ConfigsServiceHolder.java --exec cat -n {}

echo "=== Constants.java (relevant section) ==="
fd Constants.java --path '*configs*' --exec grep -n "AGENT_CONFIG\|ERROR_CODE_AGENT" {} \;

echo "=== ServerConfigManagementService.java ==="
fd ServerConfigManagementService.java --exec cat -n {}

Length of output: 14847


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Constants.java ==="
fd Constants.java | grep configs | xargs grep -n "AGENT_CONFIG\|ERROR_CODE_AGENT" 

echo "=== ServerConfigManagementService.java ==="
fd ServerConfigManagementService.java --exec cat -n {}

echo "=== ConfigsApiServiceImpl.java (agent methods) ==="
fd ConfigsApiServiceImpl.java --exec grep -n -A 20 "AgentConfiguration\|patchAgentConfiguration\|deleteAgentConfiguration" {}

echo "=== ServerConfigManagementServiceFactory.java ==="
fd ServerConfigManagementServiceFactory.java --exec cat -n {}

Length of output: 169398


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== ServerConfigManagementServiceFactory.java ==="
fd ServerConfigManagementServiceFactory.java --exec cat -n {}

echo "=== Constants.java (full, agent + relevant regex/error area) ==="
fd Constants.java | grep configs | xargs sed -n '1,260p'

echo "=== AgentConfigPatch model ==="
fd AgentConfigPatch.java --exec cat -n {}

echo "=== AgentConfiguration model ==="
fd AgentConfiguration.java --exec cat -n {}

echo "=== configs.yaml agent section ==="
fd configs.yaml --exec grep -n -A 60 "agent:" {}

Length of output: 33701


@sahandilshan Thanks for the ping — I reviewed the new /configs/agent implementation. Overall the pattern (service holder, factory wiring, patch handling, exception mapping) closely follows the existing Impersonation/CORS config implementations, which is good for consistency. One issue I'd like to flag:

Unformatted %s placeholder in agent-config error messages

In components/org.wso2.carbon.identity.api.server.configs/org.wso2.carbon.identity.api.server.configs.v1/src/main/java/org/wso2/carbon/identity/api/server/configs/v1/core/ServerConfigManagementService.java:

  • Constants.ErrorMessage.ERROR_CODE_AGENT_CONFIG_RETRIEVE and ERROR_CODE_AGENT_CONFIG_UPDATE both have a %s placeholder in their description (e.g., "Server encountered an error while retrieving the Agent configuration of %s.").
  • However, getAgentConfiguration() (Line 547) and both catch blocks in patchAgentConfiguration() (Lines 568 and 594) invoke handleAgentConfigException(e, ERROR_CODE_AGENT_CONFIG_RETRIEVE/UPDATE, null) — passing null as the data argument.
  • Since includeData() only substitutes the placeholder when data is non-blank, the literal string %s will leak into the error response shown to API consumers instead of the tenant domain, e.g.: "Server encountered an error while retrieving the Agent configuration of %s."
  • Only deleteAgentConfiguration() (Line 609) correctly passes tenantDomain as the data argument.

Suggested fix — pass tenantDomain consistently:

public AgentConfiguration getAgentConfiguration() {

    String tenantDomain = ContextLoader.getTenantDomainFromContext();
    AgentConfiguration agentConfiguration = new AgentConfiguration();
    try {
        AgentConfig agentConfig = agentConfigMgtService.getAgentConfig(tenantDomain);
        return agentConfiguration.agentsExternallyManaged(agentConfig.isAgentsExternallyManaged());
    } catch (AgentConfigMgtException e) {
        throw handleAgentConfigException(e, Constants.ErrorMessage.ERROR_CODE_AGENT_CONFIG_RETRIEVE, tenantDomain);
    }
}

and similarly for both ERROR_CODE_AGENT_CONFIG_RETRIEVE/ERROR_CODE_AGENT_CONFIG_UPDATE calls inside patchAgentConfiguration().

Note: I noticed the same null-data bug already exists for the pre-existing ERROR_CODE_IMP_CONFIG_RETRIEVE/ERROR_CODE_IMP_CONFIG_UPDATE calls in getImpersonationConfiguration()/patchImpersonationConfiguration(). That's out of scope for this PR, but since the agent-config code appears to have copied that pattern, it may be worth fixing both while you're in the area (or as a quick follow-up).

Everything else — OSGi service wiring in ConfigsServiceHolder/ServerConfigManagementServiceFactory, patch operation validation restricted to REPLACE/ADD, and the OpenAPI schema additions in configs.yaml — looks correct and consistent with the rest of the module.

@jenkins-is-staging

Copy link
Copy Markdown

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/28497337221
Status: failure

@jenkins-is-staging

Copy link
Copy Markdown

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/28514916491

@jenkins-is-staging

Copy link
Copy Markdown

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/28514916491
Status: success

@jenkins-is-staging jenkins-is-staging left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/28514916491

@RovinKYK

RovinKYK commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Unformatted %s placeholder in agent-config error messages

Fixed the issue reported by coderabbit.

@hwupathum
hwupathum merged commit ff99a37 into wso2:master Jul 2, 2026
2 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.

5 participants