Skip to content

Add workflows permission scope to WorkflowParser#4338

Draft
salmanmkc wants to merge 2 commits intomainfrom
salmanmkc/workflows-permission
Draft

Add workflows permission scope to WorkflowParser#4338
salmanmkc wants to merge 2 commits intomainfrom
salmanmkc/workflows-permission

Conversation

@salmanmkc
Copy link
Copy Markdown
Contributor

Summary

Adds workflows as a recognized permission scope for GITHUB_TOKEN, gated behind the AllowWorkflowsPermission feature flag.

This enables workflow authors to declare permissions: workflows: write so the token can update workflow files, once the feature flag is enabled.

Changes

Permissions.cs

  • Added Workflows property with [DataMember(Name = "workflows")]
  • Added to copy constructor
  • Added to ComparisonKeyMapping
  • Explicitly excluded from write-all/read-all bulk constructors (set to NoAccess) — this is a security-sensitive scope that must be explicitly requested

WorkflowTemplateConverter.cs

  • Added case "workflows": in ConvertToPermissions()
  • Gated behind AllowWorkflowsPermission feature flag (same pattern as models)
  • read downgrades to NoAccess since workflows is a write-only scope

WorkflowFeatures.cs

  • Added AllowWorkflowsPermission property (bool, default false)
  • Added to GetDefaults()

Design Decisions

  • Feature-flagged: AllowWorkflowsPermission defaults to false for controlled rollout
  • Excluded from bulk permissions: workflows is NOT included in write-all / read-all (too security-sensitive — must be explicitly declared)
  • Write-only: permissions: workflows: read downgrades to NoAccess since the GitHub API only supports write for this scope

Testing

  • All 959 existing tests pass
  • Build and format clean

Related

Add 'workflows' as a recognized permission scope for GITHUB_TOKEN,
gated behind AllowWorkflowsPermission feature flag.

Changes:
- Permissions.cs: Add Workflows property, copy constructor, comparison
  key mapping. Excluded from write-all/read-all bulk constructors.
- WorkflowTemplateConverter.cs: Parse 'workflows' permission with
  feature flag guard. Read downgrades to NoAccess (write-only scope).
- WorkflowFeatures.cs: Add AllowWorkflowsPermission flag, default false.
Copilot AI review requested due to automatic review settings April 10, 2026 22:34
@salmanmkc salmanmkc requested a review from a team as a code owner April 10, 2026 22:34
Copy link
Copy Markdown
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

Adds support for a new permissions: workflows: write scope in the WorkflowParser, gated behind a new AllowWorkflowsPermission feature flag, and ensures the scope is excluded from read-all/write-all bulk permission construction.

Changes:

  • Added AllowWorkflowsPermission to WorkflowFeatures defaults (off by default).
  • Added Workflows to the Permissions model, including cloning and max-permission comparison mapping.
  • Updated WorkflowTemplateConverter.ConvertToPermissions() to parse workflows under a feature flag and treat read as none.
Show a summary per file
File Description
src/Sdk/WorkflowParser/WorkflowFeatures.cs Introduces AllowWorkflowsPermission feature flag (default disabled).
src/Sdk/WorkflowParser/Permissions.cs Adds the Workflows permission field and includes it in cloning/comparison logic; keeps it excluded from bulk constructors.
src/Sdk/WorkflowParser/Conversion/WorkflowTemplateConverter.cs Adds parsing logic for permissions.workflows gated by AllowWorkflowsPermission.

Copilot's findings

  • Files reviewed: 3/3 changed files
  • Comments generated: 1

Comment on lines +1963 to +1970
// Workflows only supports write; downgrade read to none
if (permissionLevel == PermissionLevel.Read)
{
permissions.Workflows = PermissionLevel.NoAccess;
}
else
{
permissions.Workflows = permissionLevel;
Copy link

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

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

workflows is not currently declared in the workflow schema (src/Sdk/WorkflowParser/workflow-v1.0.json under permissions-mapping.properties). With the current schema, TemplateReader will treat workflows as an unexpected key and skip it, so this case "workflows" block is effectively unreachable even when AllowWorkflowsPermission is enabled. Update the schema to include workflows (and align the allowed value type with the intended behavior, e.g., whether read should be accepted and downgraded or rejected by schema).

Suggested change
// Workflows only supports write; downgrade read to none
if (permissionLevel == PermissionLevel.Read)
{
permissions.Workflows = PermissionLevel.NoAccess;
}
else
{
permissions.Workflows = permissionLevel;
// Workflows only supports write.
if (permissionLevel == PermissionLevel.Write)
{
permissions.Workflows = permissionLevel;
}
else
{
context.Error(key, $"The permission 'workflows' only supports 'write'");

Copilot uses AI. Check for mistakes.
@salmanmkc salmanmkc marked this pull request as draft April 10, 2026 22:39
The write-all permission level should include workflows:write when the
AllowWorkflowsPermission feature flag is enabled, matching the behavior
of other gated permissions like copilot-requests. Previously workflows
was unconditionally excluded from write-all. This aligns with the ADR
decision that write-all means permissive access.
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