Skip to content

feat: admin consent for resource dependencies on the existing consent API - #1942

Open
serguei-gorokhov wants to merge 2 commits into
rd/2-write-time-validationfrom
rd/3-admin-consent
Open

serguei-gorokhov wants to merge 2 commits into
rd/2-write-time-validationfrom
rd/3-admin-consent

Conversation

@serguei-gorokhov

@serguei-gorokhov serguei-gorokhov commented Sep 9, 2026

Copy link
Copy Markdown

Applicable issues

Description of changes

One consent API, extended — not a second one. ConsentController gains admin-only POST/DELETE /v1/consent/{deployment_id}/admin-consent (the external-services precedent: requireAdmin first so a refusal leaks nothing, shared consentOperation, GRANT/WITHDRAW audit in onComplete). The new route registers before USER_CONSENT, whose anchored pattern would otherwise swallow the admin-consent path as a deployment id.

ConsentService is extended, not bypassed: the built consent document gains a resources section from the root app's declaration (present whenever the app declares, regardless of features.consentRequired — consent is never an author-controlled flag); non-declaring apps' documents stay byte-identical. The typed admin record — one per app, public bucket — stores the approved declaration snapshot, deep-compared at check time: any declaration change re-requires the grant (content binding); a moved app is a new key, hence unconsented (fail-closed). The ADMIN_CONSENT resource type is deliberately unmapped in ResourceTypes.of(), so no generic Resource API path can address the record. The user-consent path (acceptConsent, verifyUserConsent, USER_CONSENT storage) is unchanged.

New ResourceDependencyAuditLog on the DIAL_RESOURCE_DEPS_AUDIT logger (sibling of ExternalServiceAuditLog): one event per admin decision, with the approved/withdrawn snapshot — never credential material.

The story ships as two commits; the review-hardening commit is part of this PR's intended diff (b7b9df7b):

  • requireAdmin uses hasExplicitAdminAccess (fail-closed: empty or unconfigured admin rules deny), deliberately diverging from the external-services precedent's hasAdminAccess — this endpoint mints an app-level consent reaching every user.
  • The record is keyed by the RESOLVED application's canonical name on both write sides, never by the raw request id: names carrying valid percent-sequences could otherwise land one app's approval on another app's record, silently defeating withdrawal. Withdraw resolves the deployment (404 on unknown) instead of deleting blind.
  • Null declaration entries are skipped in the snapshot builder (config-file apps bypass write-time validation); the error handler preserves HttpException headers; the log-forging sanitization now lives once in AuditLogSanitizer, with ExternalServiceAuditLog moved onto it in this PR — ResourceDependencyAuditLog is switched onto the shared sanitizer in the request-start resolution PR (feat: resolve declared resource dependencies at request start (chat path) #1943).

Behavior changes flagged for review

  • New admin-only endpoints POST/DELETE /v1/consent/{deployment_id}/admin-consent; 403 fail-closed on unconfigured admin rules (stricter than the external-services precedent, deliberately).
  • Declaring apps' consent documents gain a resources section — non-declaring apps byte-identical.
  • Known limitation, accepted: a deployment id that itself ends in /admin-consent would have its user-consent POST captured by the admin route (the route is registered first so the anchored USER_CONSENT pattern cannot swallow the admin path — the residual ambiguity is the inverse case).
  • The diff regenerates docs/open_api_core.yaml (+87 lines — the two new consent endpoints, the resources schema, the ADMIN_CONSENT enum value); generated docs, listed in the story's acceptance criteria.

Checklist

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

🤖 Generated with Claude Code

Serguei Gorokhov and others added 2 commits September 2, 2026 23:53
… API

One consent API, extended — not a second one. ConsentController gains
admin-only POST/DELETE /v1/consent/{deployment_id}/admin-consent
(the external-services precedent: requireAdmin first so a refusal leaks
nothing, shared consentOperation, GRANT/WITHDRAW audit in onComplete).
The new route registers before USER_CONSENT, whose anchored pattern
would otherwise swallow the admin-consent path as a deployment id.

ConsentService is extended, not bypassed: the built consent document
gains a resources section from the root app's declaration (present
whenever the app declares, regardless of features.consentRequired —
consent is never an author-controlled flag), and non-declaring apps'
documents stay byte-identical. The typed admin record — one per app,
public bucket, keyed by deployment id — stores the approved snapshot
deep-compared at check time (any declaration change re-requires the
grant; a moved app is a new key, hence unconsented: fail-closed). The
ADMIN_CONSENT resource type is deliberately unmapped in
ResourceTypes.of(), so no generic Resource API path can address the
record. The user-consent path (acceptConsent, verifyUserConsent,
USER_CONSENT storage) is unchanged.

New ResourceDependencyAuditLog on the DIAL_RESOURCE_DEPS_AUDIT logger
(sibling of ExternalServiceAuditLog): one event per admin decision,
with the approved/withdrawn snapshot, outcome mapping and the same
sanitization — never credential material.

Spec: documentation repo, offline-access-delegation/implementation-specs/pr3-admin-consent.md

Co-Authored-By: Claude Code <noreply@anthropic.com>
…cord key)

Review-driven fixes to the admin-consent commit:

- requireAdmin uses hasExplicitAdminAccess (fail-closed: empty or
  unconfigured admin rules deny), deliberately diverging from the
  external-services precedent's hasAdminAccess — this endpoint mints an
  app-level consent reaching every user, the same class of power the
  platform-bucket admin API gates fail-closed.
- The admin-consent record is keyed by the RESOLVED application's
  canonical name on both write sides (grant and withdraw both resolve
  the deployment first), never by the raw request id: names carrying
  valid percent-sequences could otherwise land one app's approval on
  another app's record, silently defeating withdrawal.
- Withdraw resolves the deployment (404 on unknown) instead of deleting
  blind — a mistyped withdrawal no longer returns a misleading 200.
- Null declaration entries are skipped in the snapshot builder instead
  of crashing (config-file apps bypass write-time validation).
- The error handler preserves HttpException headers via
  ProxyContext.respond(HttpException) and logs the rejection.
- The log-forging token/reason sanitization now lives once in
  AuditLogSanitizer, shared by ExternalServiceAuditLog and
  ResourceDependencyAuditLog — a hardening applied to one audit stream
  must not silently miss the other.

Co-Authored-By: Claude Code <noreply@anthropic.com>
Comment thread docs/open_api_core.yaml
- lang: cURL
label: CURL
source: "curl -X POST https://chat.<company>.com/v1/consent/{deployment_id} \\\n -H \"Api-Key: DIAL_API_KEY\" \n"
/v1/consent/{deployment_id}/admin-consent:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

add description per each API

),
// Registered BEFORE USER_CONSENT for POST/DELETE: the USER_CONSENT pattern is anchored and
// would otherwise swallow "/v1/consent/{id}/admin-consent" whole as a deployment id.
ADMIN_CONSENT(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

How should the selector choose the controller based on that request
Here is an app: applications//folder_name/admin-consent
and another app: applications//app_name/admin-consent

The result depends on the order: it would be admin consent or accept/get consent.

if (!resources.isEmpty()) {
newConsent.setResources(resources);
}
if (noneConsentRequired && resources.isEmpty()) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The application's dependency consent is different to admin consent.
The consent service was designed to ask user for consent: that requested application will call dependent applications.
However the admin consent has a different meaning.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Not entirely: the idea is to genalize the consent mechanism - it asks user (or admin) "whether this application can access the defined set of external dependencies (application, toolset, file, prompt, any resource).
Introduction of admin_consent api is the first step: later we'll level them up (regular /consent and /admin-consent) - will add applications to admin_consent and resources to the regular user consent api. Further improvements can include per-resource consent vs all-together consent, online/offline access consent.
Once again, this is very important: the ultimate idea is to define a consent as "I agree (or admin agrees) this deployment to access the listed external dependencies (any type of resources) from my behalf"
This approach essentially answers your question:

  • consent api: "I agree this app will call the listed applications (i.e. external dependencies) from my behalf"
  • admin consent api (now): "I agree this app will call (access) the listed resources (also external dependencies) from any user's behalf"
    Same pattern in both cases: "agree that app A to access [declared list of dependencies] from my-behalf/any-user-behalf".

* RESOLVED application's canonical name — the same identity the resolver reads — never by the
* raw request id, so the two sides cannot diverge for names that carry percent-sequences.
*/
public Consent grantAdminConsent(ProxyContext context, String deploymentId) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That's dangerous action. Supposing admin updates the application by modifying a list of dependent resources. And later the admin grants the consent. But just the moment before granting another admin modifies the list.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

So the admin should see what he approves

// Admin approval of an application's declared resource dependencies — the admin's yes, per app,
// in the public bucket (the user's yes lives in USER_CONSENT, per user). Deliberately NOT mapped in
// of(): like user_consent, it is internal-only — no generic Resource API path can address it.
ADMIN_CONSENT("admin_consent", true, TimeUnit.MINUTES.toMillis(5)),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I guess the entity is heavy used in the application calls. Let's lift the TTL to 30 days.

requireAdmin();
return operation.get();
})
.onComplete(result -> ResourceDependencyAuditLog.consent(context, deploymentId, action,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why do we wrap the error here with RuntimeException?

// treat them as line terminators. Tokens additionally forbid whitespace, '=' and '"' so a caller-supplied
// value can't forge key=value pairs within the line; reason keeps spaces (it is quoted) but drops '=' and
// '"' so it can neither escape its quotes nor carry a parseable forged token.
private static final Pattern TOKEN_UNSAFE = Pattern.compile("[\\p{Cntrl}\\s=\"\\u0085\\u2028\\u2029]");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Code duplication

// value can't forge key=value pairs within the line; reason keeps spaces (it is quoted) but drops '=' and
// '"' so it can neither escape its quotes nor carry a parseable forged token.
private static final Pattern TOKEN_UNSAFE = Pattern.compile("[\\p{Cntrl}\\s=\"\\u0085\\u2028\\u2029]");
private static final Pattern REASON_UNSAFE = Pattern.compile("[\\p{Cntrl}=\"\\u0085\\u2028\\u2029]");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

the same

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