Skip to content
This repository was archived by the owner on Feb 3, 2026. It is now read-only.

Commit 1d36b9b

Browse files
jalsethjalseth
committed
feat: Support role updates and removals in EnterpriseUserWriter
This allows the SCIM provisioner to manage the roles granted to users over time. Previously, roles would be added on user creation but never updated or removed. Signed-off-by: James Alseth <jalseth@google.com>
1 parent 2e24a62 commit 1d36b9b

File tree

3 files changed

+200
-37
lines changed

3 files changed

+200
-37
lines changed

‎pkg/github/enterpriseuserwriter.go‎

Lines changed: 40 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,11 @@ import (
1919
"errors"
2020
"fmt"
2121
"net/http"
22+
"slices"
2223

2324
"github.com/abcxyz/pkg/logging"
2425
"github.com/abcxyz/team-link/v2/pkg/groupsync"
26+
"github.com/google/go-github/v67/github"
2527
)
2628

2729
const defaultMaxUsersToProvision = 1000
@@ -70,8 +72,6 @@ func (w *EnterpriseUserWriter) SetMembers(ctx context.Context, _ string, members
7072
return fmt.Errorf("failed to list users: %w", err)
7173
}
7274
desiredUsersMap := make(map[string]*SCIMUser)
73-
// Use a list to maintain the ordering of the desired users to avoid unit test flakiness.
74-
desiredUsersName := []string{}
7575
for _, m := range members {
7676
if !m.IsUser() {
7777
logger.DebugContext(ctx, "skipping non-user member", "member", m.ID())
@@ -84,7 +84,6 @@ func (w *EnterpriseUserWriter) SetMembers(ctx context.Context, _ string, members
8484
continue
8585
}
8686
desiredUsersMap[scimUser.UserName] = scimUser
87-
desiredUsersName = append(desiredUsersName, scimUser.UserName)
8887
}
8988

9089
var merr error
@@ -94,59 +93,65 @@ func (w *EnterpriseUserWriter) SetMembers(ctx context.Context, _ string, members
9493
if scimUser.Active != nil && !*scimUser.Active {
9594
continue
9695
}
97-
// Deactivate user who is not in desiredUsersMap
96+
// Deactivate user who is not in desiredUsersMap and remove any role grants.
9897
if _, ok := desiredUsersMap[username]; !ok {
9998
logger.InfoContext(ctx, "deactivating user", "user", username)
100-
if _, _, err := w.scimClient.DeactivateUser(ctx, *scimUser.ID); err != nil {
99+
scimUser.Active = github.Bool(false)
100+
scimUser.Roles = nil
101+
if _, _, err := w.scimClient.UpdateUser(ctx, *scimUser.ID, scimUser); err != nil {
101102
merr = errors.Join(merr, fmt.Errorf("failed to deactivate %q: %w", username, err))
102103
}
103104
}
104105
}
105106

106107
// 2. Create and reactivate users.
107108
var count int64
108-
for _, username := range desiredUsersName {
109+
for username, desiredUser := range desiredUsersMap {
109110
count++
110111
if count > w.maxUsersToProvision {
111112
merr = errors.Join(merr, fmt.Errorf("exceeded max users to provision: %d", w.maxUsersToProvision))
112113
break
113114
}
114115

115-
scimUser, ok := currentUsersMap[username]
116+
// Create the user if not found in currentUsersMap.
117+
currentUser, ok := currentUsersMap[username]
116118
if !ok {
117-
// Create user if not found in currentUsersMap
118119
logger.InfoContext(ctx, "creating user", "user", username)
119-
if _, _, err := w.scimClient.CreateUser(ctx, desiredUsersMap[username]); err != nil {
120+
if _, _, err := w.scimClient.CreateUser(ctx, desiredUser); err != nil {
120121
merr = errors.Join(merr, fmt.Errorf("failed to create %q: %w", username, err))
121122
}
122-
} else {
123-
// Reactivate user if user status is unknown or deactivated.
124-
if scimUser.Active == nil || !*scimUser.Active {
125-
logger.InfoContext(ctx, "deactivating user before reactivating",
126-
"user", username,
127-
"active", scimUser.Active,
128-
)
129-
deactivated, _, err := w.scimClient.DeactivateUser(ctx, *scimUser.ID)
130-
if err != nil {
131-
merr = errors.Join(merr, fmt.Errorf("failed to deactivate %q: %w", username, err))
132-
}
133-
logger.InfoContext(ctx, "reactivating user",
134-
"user", deactivated.UserName,
135-
"active", deactivated.Active,
136-
)
137-
if _, _, err := w.scimClient.ReactivateUser(ctx, *scimUser.ID); err != nil {
138-
merr = errors.Join(merr, fmt.Errorf("failed to reactivate %q: %w", username, err))
139-
}
140-
updated, _, err := w.scimClient.GetUser(ctx, *scimUser.ID)
141-
if err != nil {
142-
merr = errors.Join(merr, fmt.Errorf("after reactivation, failed to get %q: %w", username, err))
143-
}
144-
logger.InfoContext(ctx, "reactivated user",
145-
"user", updated.UserName,
146-
"active", updated.Active,
147-
)
123+
continue
124+
}
125+
126+
// Update the user if fields we care about have changed.
127+
desiredUser.ID = currentUser.ID
128+
desiredUser.Active = github.Bool(true)
129+
if entUserNeedsUpdate(currentUser, desiredUser) {
130+
logger.InfoContext(ctx, "updating user",
131+
"user", username,
132+
"before", currentUser,
133+
"after", desiredUser,
134+
)
135+
if _, _, err := w.scimClient.UpdateUser(ctx, *currentUser.ID, desiredUser); err != nil {
136+
merr = errors.Join(merr, fmt.Errorf("failed to update %q: %w", username, err))
148137
}
149138
}
150139
}
151140
return merr
152141
}
142+
143+
func entUserNeedsUpdate(have, want *SCIMUser) bool {
144+
if have.GetActive() != want.GetActive() {
145+
return true
146+
}
147+
return !slices.Equal(entRoleNames(have), entRoleNames(want))
148+
}
149+
150+
func entRoleNames(user *SCIMUser) []string {
151+
roleNames := make([]string, 0, len(user.Roles))
152+
for _, role := range user.Roles {
153+
roleNames = append(roleNames, role.Value)
154+
}
155+
slices.Sort(roleNames)
156+
return roleNames
157+
}

‎pkg/github/enterpriseuserwriter_test.go‎

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ func TestEnterpriseUserWriter_SetMembers(t *testing.T) {
8383
wantUsersOnServer: map[string]*SCIMUser{
8484
"scim-id-user.old": {
8585
SCIMUserAttributes: github.SCIMUserAttributes{
86+
Schemas: []string{"urn:ietf:params:scim:schemas:core:2.0:User"},
8687
ID: github.String("scim-id-user.old"),
8788
UserName: "user.old",
8889
Active: github.Bool(false),
@@ -147,6 +148,7 @@ func TestEnterpriseUserWriter_SetMembers(t *testing.T) {
147148
wantUsersOnServer: map[string]*SCIMUser{
148149
"scim-id-user.one": {
149150
SCIMUserAttributes: github.SCIMUserAttributes{
151+
Schemas: []string{"urn:ietf:params:scim:schemas:core:2.0:User"},
150152
ID: github.String("scim-id-user.one"),
151153
UserName: "user.one",
152154
Active: github.Bool(false),
@@ -181,6 +183,7 @@ func TestEnterpriseUserWriter_SetMembers(t *testing.T) {
181183
wantUsersOnServer: map[string]*SCIMUser{
182184
"scim-id-user.one": {
183185
SCIMUserAttributes: github.SCIMUserAttributes{
186+
Schemas: []string{"urn:ietf:params:scim:schemas:core:2.0:User"},
184187
ID: github.String("scim-id-user.one"),
185188
UserName: "user.one",
186189
Active: github.Bool(true),
@@ -255,6 +258,7 @@ func TestEnterpriseUserWriter_SetMembers(t *testing.T) {
255258
wantUsersOnServer: map[string]*SCIMUser{
256259
"scim-id-user.old": {
257260
SCIMUserAttributes: github.SCIMUserAttributes{
261+
Schemas: []string{"urn:ietf:params:scim:schemas:core:2.0:User"},
258262
ID: github.String("scim-id-user.old"),
259263
UserName: "user.old",
260264
Active: github.Bool(false),
@@ -292,6 +296,145 @@ func TestEnterpriseUserWriter_SetMembers(t *testing.T) {
292296
},
293297
},
294298
},
299+
{
300+
name: "chaging roles",
301+
initialUsers: map[string]*SCIMUser{
302+
"scim-id-nochange": {
303+
SCIMUserAttributes: github.SCIMUserAttributes{
304+
ID: github.String("scim-id-nochange"),
305+
UserName: "nochange",
306+
Active: github.Bool(true),
307+
Schemas: []string{"urn:ietf:params:scim:schemas:core:2.0:User"},
308+
},
309+
},
310+
"scim-id-toadd": {
311+
SCIMUserAttributes: github.SCIMUserAttributes{
312+
ID: github.String("scim-id-toadd"),
313+
UserName: "toadd",
314+
Active: github.Bool(true),
315+
Schemas: []string{"urn:ietf:params:scim:schemas:core:2.0:User"},
316+
},
317+
},
318+
"scim-id-toremove": {
319+
Roles: []*SCIMUserRole{
320+
{Value: "role_xyz"},
321+
},
322+
SCIMUserAttributes: github.SCIMUserAttributes{
323+
ID: github.String("scim-id-toremove"),
324+
UserName: "toremove",
325+
Active: github.Bool(true),
326+
Schemas: []string{"urn:ietf:params:scim:schemas:core:2.0:User"},
327+
},
328+
},
329+
"scim-id-todisable": {
330+
Roles: []*SCIMUserRole{
331+
{Value: "role_xyz"},
332+
},
333+
SCIMUserAttributes: github.SCIMUserAttributes{
334+
ID: github.String("scim-id-todisable"),
335+
UserName: "todisable",
336+
Active: github.Bool(true),
337+
Schemas: []string{"urn:ietf:params:scim:schemas:core:2.0:User"},
338+
},
339+
},
340+
},
341+
desiredMembers: []groupsync.Member{
342+
&groupsync.UserMember{
343+
Usr: &groupsync.User{
344+
ID: "nochange",
345+
Attributes: &SCIMUser{
346+
SCIMUserAttributes: github.SCIMUserAttributes{
347+
UserName: "nochange",
348+
},
349+
},
350+
},
351+
},
352+
&groupsync.UserMember{
353+
Usr: &groupsync.User{
354+
ID: "toadd",
355+
Attributes: &SCIMUser{
356+
Roles: []*SCIMUserRole{
357+
{Value: "role_to_add"},
358+
},
359+
SCIMUserAttributes: github.SCIMUserAttributes{
360+
UserName: "toadd",
361+
},
362+
},
363+
},
364+
},
365+
&groupsync.UserMember{
366+
Usr: &groupsync.User{
367+
ID: "toremove",
368+
Attributes: &SCIMUser{
369+
SCIMUserAttributes: github.SCIMUserAttributes{
370+
UserName: "toremove",
371+
},
372+
},
373+
},
374+
},
375+
&groupsync.UserMember{
376+
Usr: &groupsync.User{
377+
ID: "tocreate",
378+
Attributes: &SCIMUser{
379+
Roles: []*SCIMUserRole{
380+
{Value: "role_to_create"},
381+
},
382+
SCIMUserAttributes: github.SCIMUserAttributes{
383+
UserName: "tocreate",
384+
},
385+
},
386+
},
387+
},
388+
},
389+
wantUsersOnServer: map[string]*SCIMUser{
390+
"scim-id-tocreate": {
391+
Roles: []*SCIMUserRole{
392+
{Value: "role_to_create"},
393+
},
394+
SCIMUserAttributes: github.SCIMUserAttributes{
395+
ID: github.String("scim-id-tocreate"),
396+
Schemas: []string{"urn:ietf:params:scim:schemas:core:2.0:User"},
397+
UserName: "tocreate",
398+
Active: github.Bool(true),
399+
},
400+
},
401+
"scim-id-nochange": {
402+
SCIMUserAttributes: github.SCIMUserAttributes{
403+
ID: github.String("scim-id-nochange"),
404+
Schemas: []string{"urn:ietf:params:scim:schemas:core:2.0:User"},
405+
UserName: "nochange",
406+
Active: github.Bool(true),
407+
},
408+
},
409+
"scim-id-toadd": {
410+
Roles: []*SCIMUserRole{
411+
{Value: "role_to_add"},
412+
},
413+
SCIMUserAttributes: github.SCIMUserAttributes{
414+
ID: github.String("scim-id-toadd"),
415+
Schemas: []string{"urn:ietf:params:scim:schemas:core:2.0:User"},
416+
UserName: "toadd",
417+
Active: github.Bool(true),
418+
},
419+
},
420+
"scim-id-toremove": {
421+
SCIMUserAttributes: github.SCIMUserAttributes{
422+
ID: github.String("scim-id-toremove"),
423+
Schemas: []string{"urn:ietf:params:scim:schemas:core:2.0:User"},
424+
UserName: "toremove",
425+
Active: github.Bool(true),
426+
},
427+
},
428+
"scim-id-todisable": {
429+
SCIMUserAttributes: github.SCIMUserAttributes{
430+
ID: github.String("scim-id-todisable"),
431+
Schemas: []string{"urn:ietf:params:scim:schemas:core:2.0:User"},
432+
UserName: "todisable",
433+
Active: github.Bool(false),
434+
},
435+
},
436+
},
437+
},
295438
}
296439

297440
for _, tc := range cases {
@@ -321,6 +464,12 @@ func TestEnterpriseUserWriter_SetMembers(t *testing.T) {
321464
if diff := testutil.DiffErrString(err, tc.wantErrStr); diff != "" {
322465
t.Errorf("error mismatch (-want +got):\n%s", diff)
323466
}
467+
// Users are processed via Go maps which are unordered. If there is an
468+
// error part way through, we cannot know the server state, so we skip
469+
// the check.
470+
if err != nil {
471+
return
472+
}
324473
if diff := cmp.Diff(tc.wantUsersOnServer, userData.allUsers); diff != "" {
325474
t.Errorf("users on server mismatch (-want +got):\n%s", diff)
326475
}

‎pkg/github/scim.go‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"io"
2323
"net/http"
2424
"net/url"
25+
"slices"
2526
"strconv"
2627
"strings"
2728

@@ -83,6 +84,10 @@ type scimPatchPayload struct {
8384
Operations []scimPatchOp `json:"Operations"`
8485
}
8586

87+
const (
88+
scimV2UserSchema = "urn:ietf:params:scim:schemas:core:2.0:User"
89+
)
90+
8691
// NewSCIMClient creates a new client for the GHES SCIM API.
8792
func NewSCIMClient(httpClient *http.Client, baseURL string) (*SCIMClient, error) {
8893
u, err := url.Parse(strings.TrimSuffix(baseURL, "/") + ghesSCIMURLPath)
@@ -126,7 +131,9 @@ func (c *SCIMClient) ListUsers(ctx context.Context) (map[string]*SCIMUser, error
126131
func (c *SCIMClient) CreateUser(ctx context.Context, user *SCIMUser) (*SCIMUser, *github.Response, error) {
127132
path := "Users"
128133
// Schema for POST: https://datatracker.ietf.org/doc/html/rfc7644#section-3.3
129-
user.Schemas = append(user.Schemas, "urn:ietf:params:scim:schemas:core:2.0:User")
134+
if !slices.Contains(user.Schemas, scimV2UserSchema) {
135+
user.Schemas = append(user.Schemas, scimV2UserSchema)
136+
}
130137
user.Active = github.Bool(true)
131138
var createdUser SCIMUser
132139
resp, err := c.do(ctx, http.MethodPost, c.baseURL.ResolveReference(&url.URL{Path: path}).String(), user, &createdUser)
@@ -151,7 +158,9 @@ func (c *SCIMClient) GetUser(ctx context.Context, scimID string) (*SCIMUser, *gi
151158
func (c *SCIMClient) UpdateUser(ctx context.Context, scimID string, user *SCIMUser) (*SCIMUser, *github.Response, error) {
152159
path := fmt.Sprintf("Users/%s", scimID)
153160
// Schema for PUT: https://datatracker.ietf.org/doc/html/rfc7644#section-3.5.1
154-
user.Schemas = append(user.Schemas, "urn:ietf:params:scim:schemas:core:2.0:User")
161+
if !slices.Contains(user.Schemas, scimV2UserSchema) {
162+
user.Schemas = append(user.Schemas, scimV2UserSchema)
163+
}
155164
var updatedUser SCIMUser
156165
resp, err := c.do(ctx, http.MethodPut, c.baseURL.ResolveReference(&url.URL{Path: path}).String(), user, &updatedUser)
157166
if err != nil {

0 commit comments

Comments
 (0)