Skip to content

security: add authorization checks to WorkflowStatsSLAHttpHandler#16159

Open
adilburaksen wants to merge 1 commit into
cdapio:developfrom
adilburaksen:fix/workflow-stats-handler-authorization
Open

security: add authorization checks to WorkflowStatsSLAHttpHandler#16159
adilburaksen wants to merge 1 commit into
cdapio:developfrom
adilburaksen:fix/workflow-stats-handler-authorization

Conversation

@adilburaksen

Copy link
Copy Markdown

Summary

WorkflowStatsSLAHttpHandler serves workflow run statistics, run details, and run comparisons for a namespace/application/workflow selected entirely from the request path, but it did not enforce any access check before reading from the Store. The handler only injected Store, MRJobInfoFetcher, and MetricsSystemClient, so a principal could read run statistics for workflows in any namespace without an authorization check.

This change adds StandardPermission.GET enforcement on the targeted workflow program before the data read, consistent with the sibling program/workflow handlers in app-fabric.

Changes

  • Inject ContextAccessEnforcer via the existing @Inject constructor (Guice resolves it from AuthorizationEnforcementModule; the handler binding in AppFabricServiceRuntimeModule is unchanged).
  • Add a small enforceWorkflowAccess(namespace, app, workflow) helper that builds the workflow ProgramReference and calls contextAccessEnforcer.enforce(programReference, StandardPermission.GET).
  • Call the helper at the start of all three read endpoints:
    • workflowStats.../workflows/{workflow-id}/statistics
    • workflowRunDetail.../workflows/{workflow-id}/runs/{run-id}/statistics
    • compare.../workflows/{workflow-id}/runs/{run-id}/compare

All three endpoints are read-only, so StandardPermission.GET is the appropriate permission. The enforcement mirrors the idiom already used in ProgramLifecycleService and ArtifactHttpHandler.

Testing

  • mvn -o -pl cdap-app-fabric -am compile — BUILD SUCCESS.

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces authorization checks in WorkflowStatsSLAHttpHandler by injecting ContextAccessEnforcer and enforcing StandardPermission.GET on workflow programs before retrieving run statistics. A review comment suggests simplifying the instantiation of ProgramReference by using its constructor directly instead of chaining methods via Ids.

Comment on lines +88 to +89
ProgramReference programReference = Ids.namespace(namespaceId).appReference(appId)
.program(ProgramType.WORKFLOW, workflowId);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Instead of using the helper class Ids and chaining multiple method calls (Ids.namespace(...).appReference(...).program(...)), you can directly instantiate ProgramReference using its public constructor. This is more direct, improves readability, and avoids the need to import io.cdap.cdap.proto.id.Ids.

Suggested change
ProgramReference programReference = Ids.namespace(namespaceId).appReference(appId)
.program(ProgramType.WORKFLOW, workflowId);
ProgramReference programReference = new ProgramReference(namespaceId, appId,
ProgramType.WORKFLOW, workflowId);

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.

1 participant