fix(SCIM): don't allow SCIM users to change fields (#63130)

This commit is contained in:
Vincent 2024-06-06 19:17:27 +01:00 committed by GitHub
parent 9e724bc596
commit c157fa82ff
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 89 additions and 4 deletions

View File

@ -460,27 +460,44 @@ func (r *schemaResolver) UpdateUser(ctx context.Context, args *updateUserArgs) (
}
update := database.UserUpdate{
DisplayName: args.DisplayName,
AvatarURL: args.AvatarURL,
AvatarURL: args.AvatarURL,
}
user, err := r.db.Users().GetByID(ctx, userID)
if err != nil {
return nil, errors.Wrap(err, "getting user from the database")
}
usernameChanged := args.Username != nil && user.Username != *args.Username
// Check if the user account is SCIM controlled
if user.SCIMControlled {
errUserScimManaged := errors.Errorf("cannot update externally managed user")
if usernameChanged {
return nil, errUserScimManaged
}
if args.DisplayName != nil && user.DisplayName != *args.DisplayName {
return nil, errUserScimManaged
}
}
update.DisplayName = args.DisplayName
// If user is changing their username, we need to verify if this action can be
// done.
if args.Username != nil && user.Username != *args.Username {
if usernameChanged {
if !viewerCanChangeUsername(actor.FromContext(ctx), r.db, userID) {
return nil, errors.Errorf("unable to change username because auth.enableUsernameChanges is false in site configuration")
}
update.Username = *args.Username
}
if err := r.db.Users().Update(ctx, userID, update); err != nil {
return nil, err
}
if featureflag.FromContext(ctx).GetBoolOr("auditlog-expansion", false) {
// Log an event when a user account is modified/updated
if err := r.db.SecurityEventLogs().LogSecurityEvent(ctx, database.SecurityEventNameAccountModified, "", uint32(userID), "", "BACKEND", args); err != nil {
r.logger.Error("Error logging security event", log.Error(err))
@ -821,6 +838,7 @@ func viewerCanChangeUsername(a *actor.Actor, db database.DB, userID int32) bool
if conf.Get().AuthEnableUsernameChanges {
return true
}
// 🚨 SECURITY: Only site admins are allowed to change a user's username when auth.enableUsernameChanges == false.
return auth.CheckCurrentActorIsSiteAdmin(a, db) == nil
}

View File

@ -491,6 +491,73 @@ func TestUpdateUser(t *testing.T) {
})
})
t.Run("scim controlled user cannot change display or username", func(t *testing.T) {
conf.Mock(&conf.Unified{
SiteConfiguration: schema.SiteConfiguration{
AuthEnableUsernameChanges: false,
},
})
defer conf.Mock(nil)
mockUser := &types.User{
ID: 1,
SCIMControlled: true,
Username: "alice",
DisplayName: "alice-updated",
AvatarURL: "http://www.example.com/alice.png",
}
users := dbmocks.NewMockUserStore()
users.GetByIDFunc.SetDefaultReturn(mockUser, nil)
users.GetByCurrentAuthUserFunc.SetDefaultReturn(mockUser, nil)
users.UpdateFunc.SetDefaultReturn(nil)
db.UsersFunc.SetDefaultReturn(users)
RunTests(t, []*Test{
{
Context: actor.WithActor(context.Background(), &actor.Actor{UID: 1}),
Schema: mustParseGraphQLSchema(t, db),
Query: `
mutation {
updateUser(
user: "VXNlcjox",
displayName: "alice-changed"
) {
displayName,
}
}
`,
ExpectedResult: `null`,
ExpectedErrors: []*gqlerrors.QueryError{
{
Path: []any{string("updateUser")},
Message: "cannot update externally managed user",
},
},
},
{
Context: actor.WithActor(context.Background(), &actor.Actor{UID: 1}),
Schema: mustParseGraphQLSchema(t, db),
Query: `
mutation {
updateUser(
user: "VXNlcjox",
username: "alice-updated"
) {
username,
}
}
`,
ExpectedResult: `null`,
ExpectedErrors: []*gqlerrors.QueryError{
{
Path: []any{string("updateUser")},
Message: "cannot update externally managed user",
},
},
},
})
})
t.Run("only allowed by authenticated user on Sourcegraph.com", func(t *testing.T) {
users := dbmocks.NewMockUserStore()
db.UsersFunc.SetDefaultReturn(users)