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

BED-5060: Prevent user from changing their own role/auth #984

Merged
merged 11 commits into from
Dec 6, 2024
Merged

Conversation

wes-mil
Copy link
Contributor

@wes-mil wes-mil commented Nov 26, 2024

Description

Prevent a user from updating their own role or auth method within user configuration.

Also addresses two bugs discovered while addressing ticket:

  • An admin user could accidentally delete a user's password while adjusting their authentication method
  • GetAuthSecret didn't return ERRNOTFOUND while attempting to get an auth secret by ID

Motivation and Context

This PR addresses: BED_5060

Why is this change required? What problem does it solve?

An admin user could change their own role to a role that is not privileged or change their own authentication method to something else. While this could be intentional, it largely caused issues with misconfigured SSO providers or accidental role changing. Effectively preventing that user from authenticating or having privilege. In the worst case, this could occur have occured while there was only a single admin user in the account.

How Has This Been Tested?

An admin user is created an then the user attempts to change their role through user configuration. On the UI these fields are now grayed out. Through the API, a PATCH request is made using an API token from an admin user attempting configure those fields. The API returns a 400 response now.

Test cases were added for the changes to database method UpdateUser.

Screenshots (optional):

image

Screenshot from 2024-11-25 15-20-02

Screenshot from 2024-11-25 15-24-01

Types of changes

  • Chore (a change that does not modify the application functionality)
  • Bug fix (non-breaking change which fixes an issue)

Checklist:

@wes-mil wes-mil requested a review from mistahj67 November 26, 2024 21:36
Copy link
Contributor

@mistahj67 mistahj67 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the disabled route is more apparent to the user but the ticket requests it not appear at all. Likely can verify with Ryan which we prefer!

cmd/api/src/api/error.go Outdated Show resolved Hide resolved
cmd/api/src/database/auth.go Outdated Show resolved Hide resolved
@@ -431,7 +444,7 @@ func (s *BloodhoundDB) CreateAuthSecret(ctx context.Context, authSecret model.Au
func (s *BloodhoundDB) GetAuthSecret(ctx context.Context, id int32) (model.AuthSecret, error) {
var (
authSecret model.AuthSecret
result = s.db.WithContext(ctx).Find(&authSecret, id)
result = s.db.WithContext(ctx).First(&authSecret, id)
Copy link
Contributor Author

@wes-mil wes-mil Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This database function was not properly returning an error when there was no auth secret with ID

Copy link
Contributor

@mistahj67 mistahj67 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, code LGTM. I'd still snag another ✅ since we ended up pairing on this, particularly the db method choice.

Copy link
Contributor

@ALCooper12 ALCooper12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Appreciate the explanations to my questions earlier!

@wes-mil wes-mil merged commit 5cb0295 into main Dec 6, 2024
4 checks passed
@wes-mil wes-mil deleted the BED-5060 branch December 6, 2024 16:27
@github-actions github-actions bot locked and limited conversation to collaborators Dec 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants