diff --git a/.env.example b/.env.example index 532a222eb..b9a3ea6a4 100644 --- a/.env.example +++ b/.env.example @@ -13,3 +13,7 @@ INPUT_GITHUB-PRIVATE-KEY=LS0tLS1CRUdJTiBQUklWQVRFIEtFWS0tLS0tXG5wcml2YXRla2V5Z29 INPUT_IGNORED-USERS=user1,user2 INPUT_SYNC-TEAM=TRUE INPUT_TEAM-SLUG=remarkable +# Optional: sync Google Workspace groups into GitHub teams. Use a file path +# (preferred) or inline JSON. team-map-file wins when both are set. +# INPUT_TEAM-MAP-FILE=.github/github-teams.json +# INPUT_TEAM-MAP={"engineering@remarkable.no":"engineering"} diff --git a/README.md b/README.md index a7690ddeb..bf62846f7 100644 --- a/README.md +++ b/README.md @@ -49,6 +49,7 @@ Right now this only handles the organization membership, it **does not** touch t - Client ID: `client id from user` - OAuth scopes: - `https://www.googleapis.com/auth/admin.directory.user.readonly` + - `https://www.googleapis.com/auth/admin.directory.group.member.readonly` (only needed for `team-map`/`team-map-file` group-to-team syncing) 1. Register new GitHub App 1. https://github.com/settings/organizations @@ -103,6 +104,54 @@ jobs: team-slug: 'remarkable' ``` +#### Syncing Google Workspace groups to GitHub teams + +Beyond `sync-team` (which adds *every* org member to a single team), you can +mirror the membership of specific Google Workspace groups into matching GitHub +teams. Provide a JSON object that maps a group's email address to a GitHub team +slug. The members of each group are resolved to their linked GitHub accounts +(via the same `Accounts.github` custom attribute) and reconciled against the +mapped team. Nested groups are expanded automatically. + +Keep the mapping in a JSON file in your repo for maintainability: + +```json +// .github/github-teams.json +{ + "engineering@remarkable.no": "engineering", + "design@remarkable.no": "design", + "platform@remarkable.no": "platform-team" +} +``` + +```yaml +- name: Github Org Membership Manager + uses: docker://ghcr.io/remarkable/githubusermanager/githubusermanager:v2.0.0 + with: + # ...the usual inputs... + github-org: 'reMarkable' + remove-users: 'true' # also prunes team members no longer in the group + team-map-file: .github/github-teams.json +``` + +Notes: + +- Members missing from a team are always added. Members present in a team but + not in the mapped group are only removed when `remove-users` is `true`, so you + can run in a safe, add-only mode first. Team removals also respect + `max-remove-users`, and a group that resolves to zero linked GitHub accounts + will never empty a team (guards against a mistyped group email). +- Any difference between a mapped team and its group sets the + `exit-code-on-mismatch` exit code, so `dry-run` + `exit-code-on-mismatch` + reports team drift in CI without applying changes. +- `ignored-users` are never added to or removed from mapped teams. +- Requires the + `https://www.googleapis.com/auth/admin.directory.group.member.readonly` scope + on the service account's domain-wide delegation. +- For a single inline mapping you can use `team-map` instead of a file, e.g. + `team-map: '{"engineering@remarkable.no":"engineering"}'`. `team-map-file` + takes precedence when both are set. + Add a dependabot configuration to always get updates! ```yaml @@ -174,6 +223,7 @@ them as `INPUT_` environment variables (upper-cased input name). | `add-users` | Set to TRUE to add users to the github organisation | `TRUE` | `false` | | `remove-users` | Set to TRUE to remove users from the github organisation | `TRUE` | `false` | | `remove-suspended_users` | Set to TRUE to remove users from the github organisation that are suspended in Google | `TRUE` | `false` | +| `dry-run` | Set to TRUE to log all intended org/team membership changes without applying them (useful with `exit-code-on-mismatch`) | `TRUE` | `false` | | `exit-code-on-mismatch` | Exit code to use when there's a mismatch, useful when combined with `add-users` and `remove-users` to be used in a dry-run mode | `1` | `0` | | `max-remove-users` | Safety limit on how many users may be removed in a single run | `100` | `100` | | `github-org` | GitHub Organization | `chrisnstest` | `null` | @@ -183,4 +233,6 @@ them as `INPUT_` environment variables (upper-cased input name). | `ignored-users` | Comma separated list of user ids to totally ignore always, useful for owners of an org you don't want accidentally removed | `owner1,owner2` | `null` | | `sync-team` | Set to TRUE to ensure every org member is added to the team named in `team-slug` | `TRUE` | `false` | | `team-slug` | Slug of the team every org member should be added to, e.g. to grant PR rights to shared repos | `remarkable` | `null` | +| `team-map` | Inline JSON mapping a Google Workspace group email to a GitHub team slug; members of each group are synced into the mapped team | `{"eng@x":"eng"}` | `null` | +| `team-map-file` | Path to a JSON file with the same group-to-team mapping as `team-map` (preferred for maintainability; wins if both are set) | `.github/teams.json` | `null` | | `debug` | Set to TRUE to enable debug level logging | `TRUE` | `false` | diff --git a/action.yml b/action.yml index 5b0adf9d2..ed0e72ec3 100644 --- a/action.yml +++ b/action.yml @@ -19,6 +19,9 @@ inputs: remove-suspended_users: description: 'Set to TRUE to remove suspended users from the github organisation' required: false + dry-run: + description: 'Set to TRUE to log all intended changes (org and team membership) without applying them. Combine with exit-code-on-mismatch for a reporting-only run.' + required: false exit-code-on-mismatch: description: 'Exit code to use when there is a mismatch, useful when combined with `ADD_USERS` and `REMOVE_USERS` to be used in a dry-run mode' required: false @@ -47,6 +50,12 @@ inputs: team-slug: description: 'Slug of the team that every org member should be added to (e.g. remarkable)' required: false + team-map: + description: 'Inline JSON object mapping a Google Workspace group email to a GitHub team slug, e.g. {"engineering@remarkable.no":"engineering"}. Members of each group are synced into the mapped team. Prefer team-map-file for maintainability. team-map-file takes precedence when both are set.' + required: false + team-map-file: + description: 'Path to a JSON file mapping Google Workspace group emails to GitHub team slugs. Members of each group are synced into the mapped team.' + required: false debug: description: 'Set to TRUE to enable debug level logging' required: false diff --git a/internal/app/app.go b/internal/app/app.go index 1327fa2ff..00a21fd68 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -14,6 +14,7 @@ import ( // GoogleProvider supplies the desired set of GitHub users from Google Workspace. type GoogleProvider interface { GithubUsersFromGoogle(ctx context.Context) (set.Set, error) + GithubUsersForGroups(ctx context.Context, groups []string) (map[string]set.Set, error) } // GitHubProvider reads and reconciles GitHub organisation membership. @@ -22,11 +23,15 @@ type GitHubProvider interface { AddUsersToOrg(ctx context.Context, users set.Set) error RemoveUsersFromOrg(ctx context.Context, users set.Set) error SyncTeamMembers(ctx context.Context) error + SyncTeamMap(ctx context.Context, desired map[string]set.Set) (bool, error) } // Run reconciles GitHub org membership against Google Workspace and returns the // process exit code (ExitCodeOnMismatch when the two sides differ, else 0). func Run(ctx context.Context, cfg *config.Config, google GoogleProvider, gh GitHubProvider) (int, error) { + if cfg.DryRun { + slog.Info("dry run enabled: no changes will be applied") + } googleUsers, err := google.GithubUsersFromGoogle(ctx) if err != nil { return 0, err @@ -72,8 +77,50 @@ func Run(ctx context.Context, cfg *config.Config, google GoogleProvider, gh GitH } } - if usersNotInGoogle.Len() > 0 || usersNotInGitHub.Len() > 0 { + teamMapDrift := false + if len(cfg.TeamMap) > 0 { + drift, err := syncTeamMap(ctx, cfg, google, gh) + if err != nil { + return 0, err + } + teamMapDrift = drift + } + + if usersNotInGoogle.Len() > 0 || usersNotInGitHub.Len() > 0 || teamMapDrift { return cfg.ExitCodeOnMismatch, nil } return 0, nil } + +// syncTeamMap resolves each configured Google Workspace group to its members' +// GitHub usernames and reconciles the mapped GitHub team membership. Multiple +// groups mapping to the same team slug are unioned. It returns true when any +// mapped team differed from its group so callers can flag a mismatch. +func syncTeamMap(ctx context.Context, cfg *config.Config, google GoogleProvider, gh GitHubProvider) (bool, error) { + groups := make([]string, 0, len(cfg.TeamMap)) + for group := range cfg.TeamMap { + groups = append(groups, group) + } + + byGroup, err := google.GithubUsersForGroups(ctx, groups) + if err != nil { + return false, err + } + + desired := make(map[string]set.Set, len(cfg.TeamMap)) + for group, slug := range cfg.TeamMap { + members := byGroup[group] + slog.Info("group members resolved to github", "group", group, "team", slug, + "users", strings.Join(members.Sorted(), ", ")) + team, ok := desired[slug] + if !ok { + team = set.New() + desired[slug] = team + } + for _, user := range members.Sorted() { + team.Add(user) + } + } + + return gh.SyncTeamMap(ctx, desired) +} diff --git a/internal/app/app_test.go b/internal/app/app_test.go index 3d8745280..b09da98db 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -13,11 +13,16 @@ import ( type fakeGoogle struct{ users set.Set } func (f fakeGoogle) GithubUsersFromGoogle(context.Context) (set.Set, error) { return f.users, nil } +func (f fakeGoogle) GithubUsersForGroups(context.Context, []string) (map[string]set.Set, error) { + return nil, nil +} type fakeGitHub struct { users set.Set added, removed set.Set syncCalled bool + teamMapDesired map[string]set.Set + teamMapDrift bool } func (f *fakeGitHub) UsersFromGitHub(context.Context) (set.Set, error) { return f.users, nil } @@ -27,6 +32,10 @@ func (f *fakeGitHub) RemoveUsersFromOrg(_ context.Context, u set.Set) error { return nil } func (f *fakeGitHub) SyncTeamMembers(context.Context) error { f.syncCalled = true; return nil } +func (f *fakeGitHub) SyncTeamMap(_ context.Context, desired map[string]set.Set) (bool, error) { + f.teamMapDesired = desired + return f.teamMapDrift, nil +} func init() { slog.SetDefault(slog.New(slog.NewTextHandler(io.Discard, nil))) } @@ -111,3 +120,59 @@ func TestMatchExitsZero(t *testing.T) { t.Errorf("no changes expected when matched: added=%v removed=%v", gh.added, gh.removed) } } + +type fakeGoogleGroups struct { + users set.Set + groups map[string]set.Set +} + +func (f fakeGoogleGroups) GithubUsersFromGoogle(context.Context) (set.Set, error) { + return f.users, nil +} +func (f fakeGoogleGroups) GithubUsersForGroups(_ context.Context, _ []string) (map[string]set.Set, error) { + return f.groups, nil +} + +func TestTeamMapSyncUnionsGroupsPerTeam(t *testing.T) { + g := fakeGoogleGroups{ + users: set.New("a"), + groups: map[string]set.Set{ + "eng@x": set.New("a", "b"), + "infra@x": set.New("b", "c"), + }, + } + gh := &fakeGitHub{users: set.New("a")} + cfg := &config.Config{TeamMap: map[string]string{ + "eng@x": "platform", + "infra@x": "platform", + }} + if _, err := Run(context.Background(), cfg, g, gh); err != nil { + t.Fatal(err) + } + got := gh.teamMapDesired["platform"] + if got == nil || got.Len() != 3 || !got.Has("a") || !got.Has("b") || !got.Has("c") { + t.Errorf("platform desired = %v, want [a b c]", got.Sorted()) + } +} + +func TestTeamMapSyncSkippedWhenUnset(t *testing.T) { + g := fakeGoogleGroups{users: set.New("a")} + gh := &fakeGitHub{users: set.New("a")} + if _, err := Run(context.Background(), &config.Config{}, g, gh); err != nil { + t.Fatal(err) + } + if gh.teamMapDesired != nil { + t.Errorf("SyncTeamMap should not be called, got %v", gh.teamMapDesired) + } +} + +func TestTeamMapDriftDrivesExitCode(t *testing.T) { + g := fakeGoogleGroups{users: set.New("a"), groups: map[string]set.Set{"eng@x": set.New("a")}} + gh := &fakeGitHub{users: set.New("a"), teamMapDrift: true} + cfg := &config.Config{ExitCodeOnMismatch: 7, TeamMap: map[string]string{"eng@x": "eng"}} + code, err := Run(context.Background(), cfg, g, gh) + _ = err + if code != 7 { + t.Errorf("exit code = %d, want 7 (team drift should flag mismatch)", code) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index 67f11448e..8f117125e 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -6,6 +6,7 @@ import ( "encoding/base64" "encoding/json" "fmt" + "os" "strconv" "strings" ) @@ -35,6 +36,7 @@ type Config struct { AddUsers bool RemoveUsers bool RemoveSuspendedUsers bool + DryRun bool ExitCodeOnMismatch int MaxRemoveUsers int Debug bool @@ -45,7 +47,10 @@ type Config struct { GitHubPrivateKey string GitHubTeamSlug string SyncTeam bool - IgnoredUsers []string + // TeamMap maps a Google Workspace group email (lower-cased) to the GitHub + // team slug whose membership should mirror that group. Empty when unset. + TeamMap map[string]string + IgnoredUsers []string GoogleEmailAddress string GoogleCredentials GoogleCredentials @@ -60,6 +65,7 @@ func Load(src InputSource) (*Config, error) { AddUsers: boolVal(src.GetInput("add-users")), RemoveUsers: boolVal(src.GetInput("remove-users")), RemoveSuspendedUsers: boolVal(src.GetInput("remove-suspended_users")), + DryRun: boolVal(src.GetInput("dry-run")), ExitCodeOnMismatch: intOr(src.GetInput("exit-code-on-mismatch"), 0), MaxRemoveUsers: intOr(src.GetInput("max-remove-users"), 100), Debug: boolVal(src.GetInput("debug")), @@ -97,9 +103,52 @@ func Load(src InputSource) (*Config, error) { return nil, fmt.Errorf("google-credentials is not valid JSON: %w", err) } + teamMap, err := loadTeamMap(src.GetInput("team-map-file"), src.GetInput("team-map")) + if err != nil { + return nil, err + } + c.TeamMap = teamMap + return c, nil } +// loadTeamMap resolves the group-to-team mapping from either a JSON file +// (team-map-file, preferred for maintainability) or inline JSON (team-map). +// The file takes precedence when both are set. Keys (group emails) are +// lower-cased and trimmed; empty entries are dropped. +func loadTeamMap(path, inline string) (map[string]string, error) { + raw := strings.TrimSpace(inline) + if p := strings.TrimSpace(path); p != "" { + b, err := os.ReadFile(p) + if err != nil { + return nil, fmt.Errorf("reading team-map-file %q: %w", p, err) + } + raw = strings.TrimSpace(string(b)) + } + if raw == "" { + return nil, nil + } + + var parsed map[string]string + if err := json.Unmarshal([]byte(raw), &parsed); err != nil { + return nil, fmt.Errorf("team map is not valid JSON (expected {\"group@domain\":\"team-slug\"}): %w", err) + } + + out := make(map[string]string, len(parsed)) + for group, slug := range parsed { + g := strings.TrimSpace(strings.ToLower(group)) + s := strings.TrimSpace(slug) + if g == "" || s == "" { + continue + } + out[g] = s + } + if len(out) == 0 { + return nil, nil + } + return out, nil +} + func boolVal(s string) bool { return strings.EqualFold(strings.TrimSpace(s), "true") } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 767c518c0..b93026621 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -2,6 +2,7 @@ package config import ( "encoding/base64" + "os" "reflect" "testing" ) @@ -121,3 +122,90 @@ func TestLoadRejectsBadBase64(t *testing.T) { t.Error("expected error for invalid base64 github-private-key") } } + +func TestTeamMapInlineJSON(t *testing.T) { + inputs := validInputs() + inputs["team-map"] = `{"Eng@remarkable.no":"engineering"," design@remarkable.no ":"design","":"skip","empty-slug@x":""}` + cfg, err := Load(inputs) + if err != nil { + t.Fatal(err) + } + want := map[string]string{ + "eng@remarkable.no": "engineering", + "design@remarkable.no": "design", + } + if !reflect.DeepEqual(cfg.TeamMap, want) { + t.Errorf("TeamMap = %v, want %v", cfg.TeamMap, want) + } +} + +func TestTeamMapFileTakesPrecedence(t *testing.T) { + dir := t.TempDir() + path := dir + "/teams.json" + if err := os.WriteFile(path, []byte(`{"platform@remarkable.no":"platform"}`), 0o600); err != nil { + t.Fatal(err) + } + inputs := validInputs() + inputs["team-map"] = `{"ignored@x":"ignored"}` + inputs["team-map-file"] = path + cfg, err := Load(inputs) + if err != nil { + t.Fatal(err) + } + want := map[string]string{"platform@remarkable.no": "platform"} + if !reflect.DeepEqual(cfg.TeamMap, want) { + t.Errorf("TeamMap = %v, want %v", cfg.TeamMap, want) + } +} + +func TestTeamMapUnsetIsNil(t *testing.T) { + cfg, err := Load(validInputs()) + if err != nil { + t.Fatal(err) + } + if cfg.TeamMap != nil { + t.Errorf("TeamMap = %v, want nil", cfg.TeamMap) + } +} + +func TestTeamMapRejectsBadJSON(t *testing.T) { + inputs := validInputs() + inputs["team-map"] = `not json` + if _, err := Load(inputs); err == nil { + t.Error("expected error for invalid team-map JSON") + } +} + +func TestDryRun(t *testing.T) { + cfg, _ := Load(validInputs()) + if cfg.DryRun { + t.Error("DryRun should default to false") + } + inputs := validInputs() + inputs["dry-run"] = "TRUE" + cfg, _ = Load(inputs) + if !cfg.DryRun { + t.Error("DryRun should be true when dry-run=TRUE") + } +} + +func TestTeamMapFileMissing(t *testing.T) { + inputs := validInputs() + inputs["team-map-file"] = "/no/such/teams.json" + if _, err := Load(inputs); err == nil { + t.Error("expected error when team-map-file does not exist") + } +} + +func TestTeamMapFileInvalidJSON(t *testing.T) { + dir := t.TempDir() + path := dir + "/teams.json" + if err := os.WriteFile(path, []byte("not json"), 0o600); err != nil { + t.Fatal(err) + } + inputs := validInputs() + inputs["team-map-file"] = path + if _, err := Load(inputs); err == nil { + t.Error("expected error for invalid JSON in team-map-file") + } +} diff --git a/internal/githuborg/github.go b/internal/githuborg/github.go index ae075f77d..2e7014473 100644 --- a/internal/githuborg/github.go +++ b/internal/githuborg/github.go @@ -7,6 +7,7 @@ import ( "fmt" "log/slog" "net/http" + "sort" "strings" "github.com/google/go-github/v88/github" @@ -25,6 +26,7 @@ type api interface { RemoveMembership(ctx context.Context, org, username string) error ListTeamMembers(ctx context.Context, org, slug string) ([]string, error) AddTeamMembership(ctx context.Context, org, slug, username string) error + RemoveTeamMembership(ctx context.Context, org, slug, username string) error } // Service applies membership changes against a GitHub organisation. @@ -85,6 +87,10 @@ func (s *Service) AddUsersToOrg(ctx context.Context, users set.Set) error { slog.Info("ignoring add", "user", user) continue } + if s.cfg.DryRun { + slog.Info("dry run: skipping org invite", "user", user, "org", s.cfg.GitHubOrg) + continue + } userID, err := s.api.GetUserID(ctx, user) if err != nil { return fmt.Errorf("unable to find user id for %s: %w", user, err) @@ -105,6 +111,10 @@ func (s *Service) RemoveUsersFromOrg(ctx context.Context, users set.Set) error { continue } slog.Info("removing user/invitation", "user", user, "org", s.cfg.GitHubOrg) + if s.cfg.DryRun { + slog.Info("dry run: skipping org removal", "user", user, "org", s.cfg.GitHubOrg) + continue + } if err := s.api.RemoveMembership(ctx, s.cfg.GitHubOrg, user); err != nil { return fmt.Errorf("removing %s: %w", user, err) } @@ -139,6 +149,10 @@ func (s *Service) SyncTeamMembers(ctx context.Context) error { continue } slog.Info("adding user to team", "user", user, "team", s.cfg.GitHubTeamSlug) + if s.cfg.DryRun { + slog.Info("dry run: skipping team add", "user", user, "team", s.cfg.GitHubTeamSlug) + continue + } // Mirror the Node behaviour: log and continue past per-user failures // (e.g. SAML SSO validation errors) rather than aborting the sync. if err := s.api.AddTeamMembership(ctx, s.cfg.GitHubOrg, s.cfg.GitHubTeamSlug, user); err != nil { @@ -148,6 +162,96 @@ func (s *Service) SyncTeamMembers(ctx context.Context) error { return nil } +// SyncTeamMap reconciles each GitHub team against the desired set of members +// resolved from its mapped Google Workspace group. Missing members are always +// added; members present in the team but not in the group are only removed when +// remove-users is enabled, keeping the dry-run behaviour consistent with org +// membership reconciliation. It returns true when any team differed from its +// group (regardless of whether changes were applied) so callers can surface a +// mismatch exit code. +func (s *Service) SyncTeamMap(ctx context.Context, desired map[string]set.Set) (bool, error) { + slugs := make([]string, 0, len(desired)) + for slug := range desired { + slugs = append(slugs, slug) + } + sort.Strings(slugs) + + drift := false + for _, slug := range slugs { + want := desired[slug] + currentRaw, err := s.api.ListTeamMembers(ctx, s.cfg.GitHubOrg, slug) + if err != nil { + return drift, fmt.Errorf("listing members of team %s: %w", slug, err) + } + current := formatUserList(currentRaw) + + missing := want.Diff(current) + extra := current.Diff(want) + + if missing.Len() == 0 && extra.Len() == 0 { + slog.Info("team already in sync with group", "team", slug) + continue + } + drift = true + + for _, user := range missing.Sorted() { + if s.isIgnored(user) { + slog.Info("ignoring team add", "user", user, "team", slug) + continue + } + slog.Info("adding user to team", "user", user, "team", slug) + if s.cfg.DryRun { + slog.Info("dry run: skipping team add", "user", user, "team", slug) + continue + } + // Mirror SyncTeamMembers: log and continue past per-user failures + // (e.g. SAML SSO validation errors) rather than aborting. + if err := s.api.AddTeamMembership(ctx, s.cfg.GitHubOrg, slug, user); err != nil { + slog.Warn("skipping team add", "user", user, "team", slug, "err", err) + } + } + + if extra.Len() == 0 { + continue + } + if !s.cfg.RemoveUsers { + slog.Info("team members not in group (not removing, remove-users disabled)", + "team", slug, "users", strings.Join(extra.Sorted(), ", ")) + continue + } + // Safety: a group that resolves to no linked GitHub accounts (e.g. a + // typo'd group email or members who haven't linked GitHub) must not be + // treated as "remove everyone". + if want.Len() == 0 { + slog.Warn("group resolved to zero github users; refusing to empty team", + "team", slug, "current", strings.Join(extra.Sorted(), ", ")) + continue + } + // Safety: mirror the org MaxRemoveUsers guard so a mapping mistake can't + // mass-remove a whole team in one run. + if extra.Len() > s.cfg.MaxRemoveUsers { + slog.Warn("not removing team members because there are too many", + "team", slug, "count", extra.Len(), "max", s.cfg.MaxRemoveUsers) + continue + } + for _, user := range extra.Sorted() { + if s.isIgnored(user) { + slog.Info("ignoring team remove", "user", user, "team", slug) + continue + } + slog.Info("removing user from team", "user", user, "team", slug) + if s.cfg.DryRun { + slog.Info("dry run: skipping team remove", "user", user, "team", slug) + continue + } + if err := s.api.RemoveTeamMembership(ctx, s.cfg.GitHubOrg, slug, user); err != nil { + slog.Warn("skipping team remove", "user", user, "team", slug, "err", err) + } + } + } + return drift, nil +} + // formatUserList lower-cases and de-duplicates a list of GitHub logins. func formatUserList(logins []string) set.Set { s := set.New() diff --git a/internal/githuborg/github_test.go b/internal/githuborg/github_test.go index 58639813a..36d3f4b5f 100644 --- a/internal/githuborg/github_test.go +++ b/internal/githuborg/github_test.go @@ -22,6 +22,7 @@ type fakeAPI struct { invited []int64 removed []string teamAdded []string + teamRemoved []string addTeamErr map[string]error getUserIDErr error } @@ -54,6 +55,10 @@ func (f *fakeAPI) AddTeamMembership(_ context.Context, _, _, u string) error { f.teamAdded = append(f.teamAdded, u) return nil } +func (f *fakeAPI) RemoveTeamMembership(_ context.Context, _, _, u string) error { + f.teamRemoved = append(f.teamRemoved, u) + return nil +} func svc(api *fakeAPI, cfg *config.Config) *Service { if cfg == nil { @@ -157,3 +162,125 @@ func TestSyncTeamContinuesPastErrors(t *testing.T) { t.Errorf("teamAdded = %v, want [foo]", api.teamAdded) } } + +func TestSyncTeamMapAddsMissing(t *testing.T) { + api := &fakeAPI{teamMembers: []string{"keep"}} + s := svc(api, &config.Config{GitHubOrg: "myorg"}) + desired := map[string]set.Set{"engineering": set.New("keep", "foo", "bar")} + if _, err := s.SyncTeamMap(context.Background(), desired); err != nil { + t.Fatal(err) + } + want := set.New("foo", "bar") + if len(api.teamAdded) != 2 { + t.Fatalf("teamAdded = %v, want foo,bar", api.teamAdded) + } + for _, u := range api.teamAdded { + if !want.Has(u) { + t.Errorf("unexpected team add %s", u) + } + } + if len(api.teamRemoved) != 0 { + t.Errorf("teamRemoved = %v, want none (remove-users disabled)", api.teamRemoved) + } +} + +func TestSyncTeamMapRemovesExtraOnlyWhenEnabled(t *testing.T) { + // remove-users disabled: extra member kept. + api := &fakeAPI{teamMembers: []string{"foo", "stale"}} + s := svc(api, &config.Config{GitHubOrg: "myorg"}) + desired := map[string]set.Set{"eng": set.New("foo")} + if _, err := s.SyncTeamMap(context.Background(), desired); err != nil { + t.Fatal(err) + } + if len(api.teamRemoved) != 0 { + t.Errorf("teamRemoved = %v, want none", api.teamRemoved) + } + + // remove-users enabled: extra member removed, ignored user preserved. + api = &fakeAPI{teamMembers: []string{"foo", "stale", "owner"}} + s = svc(api, &config.Config{GitHubOrg: "myorg", RemoveUsers: true, MaxRemoveUsers: 100, IgnoredUsers: []string{"owner"}}) + if _, err := s.SyncTeamMap(context.Background(), desired); err != nil { + t.Fatal(err) + } + if len(api.teamRemoved) != 1 || api.teamRemoved[0] != "stale" { + t.Errorf("teamRemoved = %v, want [stale]", api.teamRemoved) + } +} + +func TestDryRunSkipsOrgAndTeamMutations(t *testing.T) { + api := &fakeAPI{ + userIDs: map[string]int64{"foo": 42}, + teamMembers: []string{"stale"}, + } + cfg := &config.Config{GitHubOrg: "myorg", GitHubTeamSlug: "remarkable", DryRun: true, RemoveUsers: true} + s := svc(api, cfg) + + if err := s.AddUsersToOrg(context.Background(), set.New("foo")); err != nil { + t.Fatal(err) + } + if err := s.RemoveUsersFromOrg(context.Background(), set.New("bar")); err != nil { + t.Fatal(err) + } + if _, err := s.SyncTeamMap(context.Background(), map[string]set.Set{"remarkable": set.New("foo")}); err != nil { + t.Fatal(err) + } + + if len(api.invited) != 0 { + t.Errorf("dry run invited = %v, want none", api.invited) + } + if len(api.removed) != 0 { + t.Errorf("dry run removed = %v, want none", api.removed) + } + if len(api.teamAdded) != 0 { + t.Errorf("dry run teamAdded = %v, want none", api.teamAdded) + } + if len(api.teamRemoved) != 0 { + t.Errorf("dry run teamRemoved = %v, want none", api.teamRemoved) + } +} + +func TestSyncTeamMapRefusesEmptyGroupWipe(t *testing.T) { + api := &fakeAPI{teamMembers: []string{"foo", "bar"}} + s := svc(api, &config.Config{GitHubOrg: "myorg", RemoveUsers: true, MaxRemoveUsers: 100}) + // Group resolved to zero github users -> must not empty the team. + drift, err := s.SyncTeamMap(context.Background(), map[string]set.Set{"eng": set.New()}) + if err != nil { + t.Fatal(err) + } + if !drift { + t.Error("expected drift=true when team differs from (empty) group") + } + if len(api.teamRemoved) != 0 { + t.Errorf("teamRemoved = %v, want none (empty group must not wipe team)", api.teamRemoved) + } +} + +func TestSyncTeamMapRemovalCapped(t *testing.T) { + api := &fakeAPI{teamMembers: []string{"a", "b", "c"}} + s := svc(api, &config.Config{GitHubOrg: "myorg", RemoveUsers: true, MaxRemoveUsers: 2}) + // Desired keeps one member; 2 would be removed but cap is... 2, so allowed. + // Make extra exceed the cap: keep none-desired beyond cap. + drift, err := s.SyncTeamMap(context.Background(), map[string]set.Set{"eng": set.New("keep")}) + if err != nil { + t.Fatal(err) + } + if !drift { + t.Error("expected drift=true") + } + // current has a,b,c; want keep -> extra=3 > max 2 -> skip all removals. + if len(api.teamRemoved) != 0 { + t.Errorf("teamRemoved = %v, want none (over cap)", api.teamRemoved) + } +} + +func TestSyncTeamMapReturnsDriftFalseWhenInSync(t *testing.T) { + api := &fakeAPI{teamMembers: []string{"foo"}} + s := svc(api, &config.Config{GitHubOrg: "myorg"}) + drift, err := s.SyncTeamMap(context.Background(), map[string]set.Set{"eng": set.New("foo")}) + if err != nil { + t.Fatal(err) + } + if drift { + t.Error("expected drift=false when team already matches group") + } +} diff --git a/internal/githuborg/rest.go b/internal/githuborg/rest.go index 5bb4aa7d3..e6fa5e49c 100644 --- a/internal/githuborg/rest.go +++ b/internal/githuborg/rest.go @@ -94,3 +94,8 @@ func (r restAPI) AddTeamMembership(ctx context.Context, org, slug, username stri _, _, err := r.client.Teams.AddTeamMembershipBySlug(ctx, org, slug, username, nil) return err } + +func (r restAPI) RemoveTeamMembership(ctx context.Context, org, slug, username string) error { + _, err := r.client.Teams.RemoveTeamMembershipBySlug(ctx, org, slug, username) + return err +} diff --git a/internal/googlews/google.go b/internal/googlews/google.go index c87958d1b..bc7b7ad01 100644 --- a/internal/googlews/google.go +++ b/internal/googlews/google.go @@ -16,12 +16,16 @@ import ( "github.com/reMarkable/githubUserManager/internal/set" ) -const scope = admin.AdminDirectoryUserReadonlyScope +const ( + userScope = admin.AdminDirectoryUserReadonlyScope + memberScope = admin.AdminDirectoryGroupMemberReadonlyScope +) // Service fetches GitHub usernames from Google Workspace. type Service struct { - cfg *config.Config - users usersListCaller + cfg *config.Config + users usersListCaller + members membersListCaller } // usersListCaller is the slice of the Admin SDK we depend on, extracted as an @@ -30,10 +34,16 @@ type usersListCaller interface { List(ctx context.Context, pageToken string) (*admin.Users, error) } +// membersListCaller lists the members of a Google Workspace group, extracted as +// an interface so the group resolution logic can be unit tested. +type membersListCaller interface { + ListMembers(ctx context.Context, groupKey, pageToken string) (*admin.Members, error) +} + // New builds a Service backed by the real Admin Directory API using JWT // domain-wide delegation (impersonating the configured admin email address). func New(ctx context.Context, cfg *config.Config) (*Service, error) { - jwtCfg, err := google.JWTConfigFromJSON(cfg.GoogleCredentialsJSON, scope) + jwtCfg, err := google.JWTConfigFromJSON(cfg.GoogleCredentialsJSON, userScope, memberScope) if err != nil { return nil, fmt.Errorf("parsing google credentials: %w", err) } @@ -44,7 +54,8 @@ func New(ctx context.Context, cfg *config.Config) (*Service, error) { return nil, fmt.Errorf("creating admin directory service: %w", err) } - return &Service{cfg: cfg, users: apiCaller{svc: svc, cfg: cfg}}, nil + caller := apiCaller{svc: svc, cfg: cfg} + return &Service{cfg: cfg, users: caller, members: caller}, nil } // GithubUsersFromGoogle returns the set of GitHub usernames linked across all @@ -68,36 +79,148 @@ func (s *Service) GithubUsersFromGoogle(ctx context.Context) (set.Set, error) { return accounts, nil } -// formatUserList extracts the lower-cased, trimmed github account values from -// the users' Accounts custom schema. CustomSchemas values arrive as raw JSON. -func formatUserList(users []*admin.User) []string { - var out []string - for _, u := range users { - if u == nil { +// GithubUsersForGroups resolves each Google Workspace group (by email) to the +// set of GitHub usernames linked to its members. Membership is expanded +// recursively so nested groups are included. Members without a linked GitHub +// account are simply skipped. +func (s *Service) GithubUsersForGroups(ctx context.Context, groups []string) (map[string]set.Set, error) { + emailToGithub, err := s.emailToGithub(ctx) + if err != nil { + return nil, err + } + + out := make(map[string]set.Set, len(groups)) + for _, group := range groups { + group = strings.TrimSpace(strings.ToLower(group)) + if group == "" { continue } - raw, ok := u.CustomSchemas["Accounts"] - if !ok { - continue + emails, err := s.groupMemberEmails(ctx, group, map[string]bool{}) + if err != nil { + return nil, fmt.Errorf("listing members of group %s: %w", group, err) } - var parsed struct { - Github []struct { - Value string `json:"value"` - } `json:"github"` + accounts := set.New() + for _, email := range emails { + for _, gh := range emailToGithub[email].Sorted() { + accounts.Add(gh) + } } - if err := json.Unmarshal(raw, &parsed); err != nil { - continue + out[group] = accounts + } + return out, nil +} + +// emailToGithub builds a map of lower-cased primary email to the set of GitHub +// usernames linked on that Workspace user, paging through the directory. +func (s *Service) emailToGithub(ctx context.Context) (map[string]set.Set, error) { + out := map[string]set.Set{} + pageToken := "" + for { + resp, err := s.users.List(ctx, pageToken) + if err != nil { + return nil, fmt.Errorf("listing google users: %w", err) } - for _, acc := range parsed.Github { - v := strings.TrimSpace(strings.ToLower(acc.Value)) - if v != "" { - out = append(out, v) + for _, u := range resp.Users { + if u == nil || u.PrimaryEmail == "" { + continue } + linked := githubValues(u) + if len(linked) == 0 { + continue + } + email := strings.TrimSpace(strings.ToLower(u.PrimaryEmail)) + accounts, ok := out[email] + if !ok { + accounts = set.New() + out[email] = accounts + } + for _, v := range linked { + accounts.Add(v) + } + } + pageToken = resp.NextPageToken + if pageToken == "" { + break + } + } + return out, nil +} + +// groupMemberEmails returns the lower-cased emails of all USER members of the +// group, recursing into nested GROUP members while guarding against cycles. +func (s *Service) groupMemberEmails(ctx context.Context, group string, visited map[string]bool) ([]string, error) { + if visited[group] { + return nil, nil + } + visited[group] = true + + var emails []string + pageToken := "" + for { + resp, err := s.members.ListMembers(ctx, group, pageToken) + if err != nil { + return nil, err + } + for _, m := range resp.Members { + if m == nil || m.Email == "" { + continue + } + if strings.EqualFold(m.Type, "GROUP") { + nested, err := s.groupMemberEmails(ctx, strings.ToLower(m.Email), visited) + if err != nil { + return nil, err + } + emails = append(emails, nested...) + continue + } + emails = append(emails, strings.TrimSpace(strings.ToLower(m.Email))) + } + pageToken = resp.NextPageToken + if pageToken == "" { + break + } + } + return emails, nil +} + +// githubValues extracts the lower-cased, trimmed github account values from a +// single user's Accounts custom schema. CustomSchemas values arrive as raw JSON. +func githubValues(u *admin.User) []string { + if u == nil { + return nil + } + raw, ok := u.CustomSchemas["Accounts"] + if !ok { + return nil + } + var parsed struct { + Github []struct { + Value string `json:"value"` + } `json:"github"` + } + if err := json.Unmarshal(raw, &parsed); err != nil { + return nil + } + var out []string + for _, acc := range parsed.Github { + v := strings.TrimSpace(strings.ToLower(acc.Value)) + if v != "" { + out = append(out, v) } } return out } +// formatUserList extracts the lower-cased, trimmed github account values from +// the users' Accounts custom schema. CustomSchemas values arrive as raw JSON. +func formatUserList(users []*admin.User) []string { + var out []string + for _, u := range users { + out = append(out, githubValues(u)...) + } + return out +} + type apiCaller struct { svc *admin.Service cfg *config.Config @@ -108,7 +231,7 @@ func (a apiCaller) List(ctx context.Context, pageToken string) (*admin.Users, er Customer("my_customer"). CustomFieldMask("Accounts"). Projection("custom"). - Fields("users(customSchemas/Accounts/github(value)),nextPageToken"). + Fields("users(primaryEmail,customSchemas/Accounts/github(value)),nextPageToken"). MaxResults(250). Context(ctx) if a.cfg.RemoveSuspendedUsers { @@ -119,3 +242,13 @@ func (a apiCaller) List(ctx context.Context, pageToken string) (*admin.Users, er } return call.Do() } + +func (a apiCaller) ListMembers(ctx context.Context, groupKey, pageToken string) (*admin.Members, error) { + call := a.svc.Members.List(groupKey). + MaxResults(200). + Context(ctx) + if pageToken != "" { + call = call.PageToken(pageToken) + } + return call.Do() +} diff --git a/internal/googlews/google_test.go b/internal/googlews/google_test.go index 8f5c1e02c..4048f0131 100644 --- a/internal/googlews/google_test.go +++ b/internal/googlews/google_test.go @@ -7,6 +7,7 @@ import ( admin "google.golang.org/api/admin/directory/v1" "google.golang.org/api/googleapi" + "github.com/reMarkable/githubUserManager/internal/config" "github.com/reMarkable/githubUserManager/internal/set" ) @@ -73,3 +74,48 @@ func TestGithubUsersFromGooglePaginates(t *testing.T) { t.Errorf("got %v, want [a b]", got.Sorted()) } } + +func userWithEmail(email, accountsJSON string) *admin.User { + u := user(accountsJSON) + u.PrimaryEmail = email + return u +} + +// fakeMembers implements membersListCaller with in-memory group membership. +type fakeMembers struct{ groups map[string][]*admin.Member } + +func (f *fakeMembers) ListMembers(_ context.Context, groupKey, _ string) (*admin.Members, error) { + return &admin.Members{Members: f.groups[groupKey]}, nil +} + +func TestGithubUsersForGroupsResolvesMembers(t *testing.T) { + users := &fakeUsers{pages: []*admin.Users{{Users: []*admin.User{ + userWithEmail("alice@remarkable.no", `{"github":[{"value":"Alice"}]}`), + userWithEmail("bob@remarkable.no", `{"github":[{"value":"bob"}]}`), + userWithEmail("carol@remarkable.no", `{"github":[{"value":"carol"}]}`), + userWithEmail("nolink@remarkable.no", ""), + }}}} + members := &fakeMembers{groups: map[string][]*admin.Member{ + "eng@remarkable.no": { + {Email: "alice@remarkable.no", Type: "USER"}, + {Email: "nested@remarkable.no", Type: "GROUP"}, + {Email: "nolink@remarkable.no", Type: "USER"}, + }, + "nested@remarkable.no": { + {Email: "carol@remarkable.no", Type: "USER"}, + // cycle back into the parent group; must not recurse forever. + {Email: "eng@remarkable.no", Type: "GROUP"}, + }, + }} + s := &Service{cfg: &config.Config{}, users: users, members: members} + + got, err := s.GithubUsersForGroups(context.Background(), []string{"eng@remarkable.no"}) + if err != nil { + t.Fatal(err) + } + eng := got["eng@remarkable.no"] + want := set.New("alice", "carol") + if eng.Len() != want.Len() || !eng.Has("alice") || !eng.Has("carol") { + t.Errorf("got %v, want %v", eng.Sorted(), want.Sorted()) + } +}