Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error when updating display name and avatar URL #15663

Closed
justdueck opened this issue Nov 11, 2020 · 4 comments · Fixed by #15693
Closed

Error when updating display name and avatar URL #15663

justdueck opened this issue Nov 11, 2020 · 4 comments · Fixed by #15693
Assignees
Labels
customer Important issues reported or desired by a customer. estimate/0.5d

Comments

@justdueck
Copy link
Contributor

  • Sourcegraph version:
  • Platform information:

Steps to reproduce:

  1. Update Display Name and Avatar URL in user settings
  2. Hit 'Save'

Expected behavior:

Saves successfully and updates display name and avatar

Actual behavior:

Receives Error: unable to change username because auth.enableUsernameChanges is false in site configuration

Customer support ticket: https://sourcegraph.atlassian.net/jira/servicedesk/projects/SG/queues/custom/2/SG-420

@github-actions
Copy link
Contributor

Heads up @tsenart - the "team/cloud" label was applied to this issue.

@flying-robot flying-robot self-assigned this Nov 11, 2020
@flying-robot
Copy link
Contributor

Going to pick this up.

@flying-robot
Copy link
Contributor

flying-robot commented Nov 11, 2020

What I think is happening is that the viewerCanChangeUsername function below falls through incorrectly:

func viewerCanChangeUsername(ctx context.Context, userID int32) bool {
if err := backend.CheckSiteAdminOrSameUser(ctx, userID); err != nil {
return false
}
if conf.Get().AuthEnableUsernameChanges {
return true
}
// 🚨 SECURITY: Only site admins are allowed to change a user's username when auth.enableUsernameChanges == false.
return backend.CheckCurrentUserIsSiteAdmin(ctx) == nil
}

If the user is a site admin or it's a user updating themselves, nothing is returned from CheckSiteAdminOrSameUser. In that case we fall through to the configuration check, which then falls through to the site admin check which triggers the error that was reported. A site admin then proceeds as expected, but a regular user is unable to update something like displayName as a result.

/cc @unknwon

@unknwon
Copy link
Member

unknwon commented Nov 12, 2020

A site admin then proceeds as expected,...

I don't think so? Both site admins and regular users are blocked by this config check. (but this is not important, just a minor detail) We have confused logic flow that I misunderstood.

@unknwon unknwon added customer Important issues reported or desired by a customer. estimate/0.5d labels Nov 12, 2020
@unknwon unknwon added this to the Cloud 2020-11-04 milestone Nov 12, 2020
flying-robot added a commit that referenced this issue Nov 12, 2020
…ite config prohibits username changes

Non-admin users are currently prevented from updating their display name and avatar URL if the site configuration prohibits username changes. There are a number of additional factors in play:

1. The UI will always include `username` in the mutation payload.
2. That triggers the `viewerCanChangeUsername` check, since `args.Username` is not null.
3. `viewerCanChangeUsername` falls through to the final site admin check, and the request fails.

The test was written first to demonstrate a failing case, and then `viewerIsChangingUsername` was introduced to short circuit if appropriate.

fixes #15663
flying-robot added a commit that referenced this issue Nov 13, 2020
…ite config prohibits username changes

Non-admin users are currently prevented from updating their display name and avatar URL if the site configuration prohibits username changes. There are a number of additional factors in play:

1. The UI will always include `username` in the mutation payload.
2. That triggers the `viewerCanChangeUsername` check, since `args.Username` is not null.
3. `viewerCanChangeUsername` falls through to the final site admin check, and the request fails.

The test was written first to demonstrate a failing case, and then `viewerIsChangingUsername` was introduced to short circuit if appropriate.

fixes #15663
flying-robot added a commit that referenced this issue Nov 13, 2020
…ite config prohibits username changes (#15693)

Non-admin users are currently prevented from updating their display name and avatar URL if the site configuration prohibits username changes. There are a number of additional factors in play:

1. The UI will always include `username` in the mutation payload.
2. That triggers the `viewerCanChangeUsername` check, since `args.Username` is not null.
3. `viewerCanChangeUsername` falls through to the final site admin check, and the request fails.

The test was written first to demonstrate a failing case, and then `viewerIsChangingUsername` was introduced to short circuit if appropriate.

fixes #15663
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer Important issues reported or desired by a customer. estimate/0.5d
Projects
None yet
3 participants