Skip to content

Conversation

@pnmcosta
Copy link
Contributor

@pnmcosta pnmcosta commented Mar 5, 2025

Describe your changes

Issue ticket number and link

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)
  • Extended the README / documentation, if necessary

@pnmcosta pnmcosta force-pushed the refactor/permissions-account branch 6 times, most recently from 80cae4e to 17849b2 Compare March 9, 2025 22:25
@pnmcosta pnmcosta marked this pull request as ready for review March 10, 2025 08:54
@pnmcosta pnmcosta requested a review from pascal-fischer March 10, 2025 09:54
@pnmcosta pnmcosta force-pushed the refactor/permissions-account branch 2 times, most recently from 202df67 to f8cf9d5 Compare March 13, 2025 16:52
@mlsmaycon
Copy link
Collaborator

@pnmcosta can you handle sonar findings?

@mlsmaycon mlsmaycon requested a review from Copilot March 17, 2025 09:49
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the account management code to integrate a centralized permissions manager and adds new functionality to create and update accounts by private domain.

  • Introduces two new methods: CreateAccountByPrivateDomain and UpdateToPrimaryAccount.
  • Replaces direct account ID checks with calls to permissionsManager.ValidateAccountAccess in various methods.
  • Updates BuildManager signature, tests, and mocks throughout the codebase to support the new permissions manager.

Reviewed Changes

Copilot reviewed 32 out of 32 changed files in this pull request and generated no comments.

Show a summary per file
File Description
management/server/account.go Added new account creation/update methods and replaced direct checks with permissions validation.
management/server/event.go Updated event handling to populate user info with permissions considerations.
management/server/account_test.go Added tests for new methods and updated BuildManager usage to pass permissions manager.
management/server/peer.go Replaced direct account checks with calls to permissionsManager.ValidateAccountAccess.
management/server/mock_server/account_mock.go Added mocks for the new methods.
management/server/dns*.go, nameserver*.go, group.go Replaced direct user.AccountID comparisons with permissions validation calls.
client/internal/engine_test.go, client/cmd/testutil_test.go, etc. Updated tests to include the new permissions manager dependency.
management/cmd/management.go Updated BuildManager call to initialize the permissions manager via integrations.
management/server/http/handlers/events/events_handler.go Removed unused legacy event metadata filling code.
Comments suppressed due to low confidence (2)

management/server/account.go:1714

  • [nitpick] Consider replacing the magic number '2' with a named constant (e.g., retryCount) to improve clarity and maintainability.
for range 2 {

management/server/account.go:1110

  • The direct check 'user.AccountID != accountID' has been replaced with permissionsManager.ValidateAccountAccess; ensure that the new validation call handles all the required permission scenarios consistently.
if user.AccountID != accountID {

@pnmcosta pnmcosta force-pushed the refactor/permissions-account branch 3 times, most recently from 3766f0e to 61b0acd Compare March 21, 2025 15:54
@pnmcosta pnmcosta force-pushed the refactor/permissions-account branch from 61b0acd to 865d89a Compare March 21, 2025 18:12
pascal-fischer and others added 8 commits March 29, 2025 13:07
* refactor routes permissions

Signed-off-by: bcmmbaga <bethuelmbaga12@gmail.com>

* fix tests

---------

Signed-off-by: bcmmbaga <bethuelmbaga12@gmail.com>
Co-authored-by: Pascal Fischer <pascal@netbird.io>
# Conflicts:
#	client/server/server_test.go
#	go.mod
#	go.sum
@sonarqubecloud
Copy link

@pascal-fischer pascal-fischer merged commit cbec7bd into main Mar 30, 2025
38 checks passed
@pascal-fischer pascal-fischer deleted the refactor/permissions-account branch March 30, 2025 15:08
remipcomaite pushed a commit to remipcomaite/netbird that referenced this pull request Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants