Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 50 additions & 2 deletions internal/appsetup/appsetup.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ type Setup struct {
ui *ui.Printer
knownSlugs map[string]string
secretExists SecretExistsFunc
permErrors []string
}

// NewSetup creates a new Setup instance.
Expand Down Expand Up @@ -147,7 +148,7 @@ func (s *Setup) Run(ctx context.Context, org, role string) (*AppCredentials, err
}

if found {
return s.handleExistingApp(ctx, inst, role)
return s.handleExistingApp(ctx, inst, org, role)
}

// No existing app found — run the manifest flow.
Expand Down Expand Up @@ -213,14 +214,16 @@ func (s *Setup) findExistingInstallation(
//
// When an existing app is found with valid credentials, it is reused
// automatically. To get fresh apps, run uninstall first, then install.
func (s *Setup) handleExistingApp(ctx context.Context, inst *forge.Installation, role string) (*AppCredentials, error) {
func (s *Setup) handleExistingApp(ctx context.Context, inst *forge.Installation, org, role string) (*AppCredentials, error) {
s.ui.StepDone(fmt.Sprintf("Found existing app: %s (ID: %d)", inst.AppSlug, inst.AppID))

clientID, err := s.client.GetAppClientID(ctx, inst.AppSlug)
if err != nil {
return nil, fmt.Errorf("looking up client ID for %s: %w", inst.AppSlug, err)
}

s.checkPermissions(inst, org, role)

if s.secretExists != nil {
Comment thread
rh-hemartin marked this conversation as resolved.
Resolved
exists, err := s.secretExists(role)
if err != nil {
Expand Down Expand Up @@ -257,6 +260,51 @@ func (s *Setup) handleExistingApp(ctx context.Context, inst *forge.Installation,
}, nil
}

// checkPermissions warns if the installed app is missing permissions that
// the current manifest expects.
// PermissionErrors returns a combined error if any apps have stale permissions,
// or nil if all permissions are up to date. Call after all roles have been
// processed so the user sees every mismatch at once.
func (s *Setup) PermissionErrors() error {
if len(s.permErrors) == 0 {
return nil
}
return fmt.Errorf("apps have stale permissions:\n %s", strings.Join(s.permErrors, "\n "))
}

func (s *Setup) checkPermissions(inst *forge.Installation, org, role string) {
if inst.Permissions == nil {
return
}
expected := ghTypes.AgentAppConfig(org, role).Permissions
Comment thread
rh-hemartin marked this conversation as resolved.
Resolved
want := map[string]string{
"issues": expected.Issues,
"pull_requests": expected.PullRequests,
"checks": expected.Checks,
"contents": expected.Contents,
"workflows": expected.Workflows,
"administration": expected.Administration,
"members": expected.Members,
}
Comment thread
rh-hemartin marked this conversation as resolved.
Resolved
var missing []string
for perm, level := range want {
if level == "" {
continue
}
if inst.Permissions[perm] != level {
missing = append(missing, fmt.Sprintf("%s:%s", perm, level))
}
}
if len(missing) == 0 {
return
}
s.ui.StepWarn(fmt.Sprintf("app %s missing permissions: %s", inst.AppSlug, strings.Join(missing, ", ")))
s.permErrors = append(s.permErrors, fmt.Sprintf(
"%s — update at https://github.com/organizations/%s/settings/apps/%s/permissions",
inst.AppSlug, org, inst.AppSlug,
))
}

// manifestResponse is the JSON response from GitHub's app manifest conversion.
type manifestResponse struct {
ID int `json:"id"`
Expand Down
70 changes: 70 additions & 0 deletions internal/appsetup/appsetup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,76 @@ func TestManifestFlow_HTMLForm(t *testing.T) {
<-errCh
}

func TestSetup_StalePermissions_AllRolesChecked(t *testing.T) {
client := &forge.FakeClient{
Installations: []forge.Installation{
{
ID: 100, AppID: 10, AppSlug: "myorg-fullsend",
Permissions: map[string]string{
"contents": "write",
"issues": "read",
"pull_requests": "write",
"checks": "read",
"administration": "write",
"members": "read",
// missing: workflows
},
},
{
ID: 101, AppID: 11, AppSlug: "myorg-triage",
Permissions: map[string]string{
// has issues:read but needs issues:write
"issues": "read",
},
},
},
}
printer := ui.New(&discardWriter{})

setup := NewSetup(client, &fakePrompter{}, newFakeBrowser(), printer).
WithSecretExists(func(_ string) (bool, error) { return true, nil })

// Run both roles — each should succeed individually.
_, err := setup.Run(context.Background(), "myorg", "fullsend")
require.NoError(t, err)
_, err = setup.Run(context.Background(), "myorg", "triage")
require.NoError(t, err)

// PermissionErrors should report both apps.
permErr := setup.PermissionErrors()
require.Error(t, permErr)
assert.Contains(t, permErr.Error(), "myorg-fullsend")
assert.Contains(t, permErr.Error(), "myorg-triage")
}

func TestSetup_CorrectPermissions_NoError(t *testing.T) {
client := &forge.FakeClient{
Installations: []forge.Installation{
{
ID: 100, AppID: 10, AppSlug: "myorg-fullsend",
Permissions: map[string]string{
"contents": "write",
"workflows": "write",
"issues": "read",
"pull_requests": "write",
"checks": "read",
"administration": "write",
"members": "read",
},
},
},
}
printer := ui.New(&discardWriter{})

setup := NewSetup(client, &fakePrompter{}, newFakeBrowser(), printer).
WithSecretExists(func(_ string) (bool, error) { return true, nil })

_, err := setup.Run(context.Background(), "myorg", "fullsend")
require.NoError(t, err)

assert.NoError(t, setup.PermissionErrors())
}

// discardWriter implements io.Writer, discarding all output.
type discardWriter struct{}

Expand Down
4 changes: 4 additions & 0 deletions internal/cli/admin.go
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,10 @@ func runAppSetup(ctx context.Context, client forge.Client, printer *ui.Printer,
})
}

if err := setup.PermissionErrors(); err != nil {
return nil, err
}

printer.Blank()
return creds, nil
}
Expand Down
7 changes: 4 additions & 3 deletions internal/forge/forge.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,10 @@ type IssueComment struct {

// Installation represents an app installation on an org.
type Installation struct {
ID int
AppID int
AppSlug string
ID int
AppID int
AppSlug string
Permissions map[string]string
}

// Client abstracts all git forge operations.
Expand Down
14 changes: 8 additions & 6 deletions internal/forge/github/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -1062,9 +1062,10 @@ func (c *LiveClient) ListOrgInstallations(ctx context.Context, org string) ([]fo

var result struct {
Installations []struct {
ID int `json:"id"`
AppID int `json:"app_id"`
AppSlug string `json:"app_slug"`
ID int `json:"id"`
AppID int `json:"app_id"`
AppSlug string `json:"app_slug"`
Permissions map[string]string `json:"permissions"`
} `json:"installations"`
}
if err := decodeJSON(resp, &result); err != nil {
Expand All @@ -1074,9 +1075,10 @@ func (c *LiveClient) ListOrgInstallations(ctx context.Context, org string) ([]fo
installs := make([]forge.Installation, len(result.Installations))
for i, inst := range result.Installations {
installs[i] = forge.Installation{
ID: inst.ID,
AppID: inst.AppID,
AppSlug: inst.AppSlug,
ID: inst.ID,
AppID: inst.AppID,
AppSlug: inst.AppSlug,
Permissions: inst.Permissions,
}
}
return installs, nil
Expand Down
Loading