Merge pull request juju#17621 from Aflynn50/add-include-disabled-flag

juju#17621

The `includeDisabled` flag was present in the Mongo function `GetAllUsers` so needed to be added back here to the DQLite equivalent.

<!-- Why this change is needed and what it does. -->

## Checklist

<!-- If an item is not applicable, use `~strikethrough~`. -->

- [x] Code style: imports ordered, good names, simple structure, etc
- [x] Comments saying why design decisions were made
- [x] Go unit tests, with comments saying what you're testing
~- [ ] [Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing~
- [x] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages

## QA steps
### Unit tests
```
TEST_PACKAGES="./domain/access/state/... -count=1 " TEST_FILTER="Suite" make run-go-tests
TEST_PACKAGES="./apiserver/facades/client/usermanager/... -count=1 " TEST_FILTER="Suite" make run-go-tests
```

### Integration tests
```
$ juju bootstrap lxd test-all-users
$ juju add-model default
$ juju change-user-password admin
$ juju add-user joe
$ juju change-user-password joe
$ juju users
Controller: test-all-users

Name Display name Access Date created Last connection
admin* admin superuser 3 minutes ago just now
juju-metrics Juju Metrics login 3 minutes ago 0001-01-01

$ juju users --all
Controller: test-all-users

Name Display name Access Date created Last connection
admin* admin superuser 3 minutes ago just now
juju-metrics Juju Metrics login 3 minutes ago 0001-01-01
joe 40 seconds ago 0001-01-01 (disabled)
```

## Documentation changes
Added doc.go to access domain
<!-- How it affects user workflow (CLI or API). -->

## Links

<!-- Link to all relevant specification, documentation, bug, issue or JIRA card. -->

**Jira card:** [JUJU-6282](https://warthogs.atlassian.net/browse/JUJU-6282)



[JUJU-6282]: https://warthogs.atlassian.net/browse/JUJU-6282?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ

apiserver/facades/client/usermanager/usermanager.go

@@ -25,7 +25,7 @@ import (
 
 // AccessService defines the methods to operate with the database.
 type AccessService interface {
-	GetAllUsers(ctx context.Context) ([]coreuser.User, error)
+	GetAllUsers(ctx context.Context, includeDisabled bool) ([]coreuser.User, error)
 	GetUserByName(ctx context.Context, name string) (coreuser.User, error)
 	AddUser(ctx context.Context, arg service.AddUserArg) (coreuser.UUID, []byte, error)
 	EnableUserAuthentication(ctx context.Context, name string) error
@@ -312,7 +312,7 @@ func (api *UserManagerAPI) UserInfo(ctx context.Context, request params.UserInfo
 
 		if isAdmin {
 			// Get all users if isAdmin
-			users, err := api.accessService.GetAllUsers(ctx)
+			users, err := api.accessService.GetAllUsers(ctx, request.IncludeDisabled)
 			if err != nil {
 				return results, errors.Trace(err)
 			}

apiserver/facades/client/usermanager/usermanager_test.go

@@ -386,42 +386,56 @@ func (s *userManagerSuite) TestUserInfoAll(c *gc.C) {
 			Name:     "fred",
 			Disabled: false,
 		},
-		{
+	}
+	usersIncDisabled := append(users,
+		coreuser.User{
 			UUID:     newUserUUID(c),
 			Name:     "nancy",
-			Disabled: false,
+			Disabled: true,
 		},
-	}
+	)
 
-	// TODO (manadart 2024-02-14) This test is contrived to pass.
-	// The service is not correctly implemented as it does not
-	// factor the `IncludeDisabled` argument.
+	expected := params.UserInfoResults{
+		Results: []params.UserInfoResult{{
+			Result: &params.UserInfo{
+				Username:       "fred",
+				Disabled:       false,
+				Access:         "login",
+				LastConnection: &time.Time{},
+			},
+		}}}
+	expectedIncDisabled := params.UserInfoResults{
+		Results: append(expected.Results, params.UserInfoResult{
+			Result: &params.UserInfo{
+				Username:       "nancy",
+				Disabled:       true,
+				Access:         "",
+				LastConnection: &time.Time{},
+			},
+		}),
+	}
 
-	s.accessService.EXPECT().GetAllUsers(gomock.Any()).Return(users, nil).Times(2)
+	gomock.InOrder(
+		s.accessService.EXPECT().GetAllUsers(gomock.Any(), false).Return(users, nil),
+		s.accessService.EXPECT().GetAllUsers(gomock.Any(), true).Return(usersIncDisabled, nil),
+	)
 
+	// The access service is used only for none-deactivated users, deactivated
+	// users have NoPermissions.
 	s.accessService.EXPECT().ReadUserAccessForTarget(gomock.Any(), "fred", permission.ID{
 		ObjectType: permission.Controller,
 		Key:        "controller",
 	}).Return(permission.UserAccess{Access: permission.LoginAccess}, nil).Times(2)
 
-	s.accessService.EXPECT().ReadUserAccessForTarget(gomock.Any(), "nancy", permission.ID{
-		ObjectType: permission.Controller,
-		Key:        "controller",
-	}).Return(permission.UserAccess{Access: permission.LoginAccess}, nil).Times(2)
-
-	args := params.UserInfoRequest{IncludeDisabled: true}
-	_, err := s.api.UserInfo(context.Background(), args)
+	results, err := s.api.UserInfo(context.Background(), params.UserInfoRequest{})
 	c.Assert(err, jc.ErrorIsNil)
+	c.Check(results, jc.DeepEquals, expected)
 
-	//var expected params.UserInfoResults
-	//c.Check(results, jc.DeepEquals, expected)
-
-	_, err = s.api.UserInfo(context.Background(), params.UserInfoRequest{})
+	args := params.UserInfoRequest{IncludeDisabled: true}
+	results, err = s.api.UserInfo(context.Background(), args)
 	c.Assert(err, jc.ErrorIsNil)
+	c.Check(results, jc.DeepEquals, expectedIncDisabled)
 
-	// Same results as before, but without the deactivated user
-	//expected.Results = expected.Results[1:]
-	//c.Check(results, jc.DeepEquals, expected)
 }
 
 func (s *userManagerSuite) TestUserInfoNonControllerAdmin(c *gc.C) {

domain/access/doc.go

@@ -0,0 +1,7 @@
+// Copyright 2024 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+// Package access provides the services for managing users and permissions in
+// Juju. The users side is mainly concerned with authentication and the
+// permissions side with authorization.
+package access

domain/access/service/service.go

@@ -72,7 +72,7 @@ type UserState interface {
 	// GetAllUsers will retrieve all users with authentication information
 	// (last login, disabled) from the database. If no users exist an empty slice
 	// will be returned.
-	GetAllUsers(context.Context) ([]user.User, error)
+	GetAllUsers(ctx context.Context, includeDisabled bool) ([]user.User, error)
 
 	// GetUser will retrieve the user with authentication information (last login, disabled)
 	// specified by UUID from the database. If the user does not exist an error that satisfies

domain/access/service/user.go

@@ -36,8 +36,8 @@ func NewUserService(st UserState) *UserService {
 // GetAllUsers will retrieve all users with authentication information
 // (last login, disabled) from the database. If no users exist an empty slice
 // will be returned.
-func (s *UserService) GetAllUsers(ctx context.Context) ([]user.User, error) {
-	usrs, err := s.st.GetAllUsers(ctx)
+func (s *UserService) GetAllUsers(ctx context.Context, includeDisabled bool) ([]user.User, error) {
+	usrs, err := s.st.GetAllUsers(ctx, includeDisabled)
 	if err != nil {
 		return nil, errors.Annotate(err, "getting all users with auth info")
 	}

domain/access/service/user_test.go

@@ -308,7 +308,7 @@ func (s *userServiceSuite) TestGetUserByNameNotFound(c *gc.C) {
 func (s *userServiceSuite) TestGetAllUsers(c *gc.C) {
 	defer s.setupMocks(c).Finish()
 
-	s.state.EXPECT().GetAllUsers(gomock.Any()).Return([]user.User{
+	s.state.EXPECT().GetAllUsers(gomock.Any(), true).Return([]user.User{
 		{
 			UUID: newUUID(c),
 			Name: "user0",
@@ -319,7 +319,7 @@ func (s *userServiceSuite) TestGetAllUsers(c *gc.C) {
 		},
 	}, nil)
 
-	users, err := s.service().GetAllUsers(context.Background())
+	users, err := s.service().GetAllUsers(context.Background(), true)
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(users, gc.HasLen, 2)
 	c.Check(users[0].Name, gc.Equals, "user0")

domain/access/state/user.go

@@ -113,15 +113,13 @@ func (st *UserState) AddUserWithActivationKey(
 // GetAllUsers will retrieve all users with authentication information
 // (last login, disabled) from the database. If no users exist an empty slice
 // will be returned.
-func (st *UserState) GetAllUsers(ctx context.Context) ([]user.User, error) {
+func (st *UserState) GetAllUsers(ctx context.Context, includeDisabled bool) ([]user.User, error) {
 	db, err := st.DB()
 	if err != nil {
 		return nil, errors.Annotate(err, "getting DB access")
 	}
 
-	var usrs []user.User
-	err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error {
-		getAllUsersQuery := `
+	selectGetAllUsersStmt, err := st.Prepare(`
 SELECT (u.uuid, u.name, u.display_name, u.created_by_uuid, u.created_at, u.disabled, ull.last_login) AS (&dbUser.*),
        creator.name AS &dbUser.created_by_name
 FROM   v_user_auth u
@@ -130,29 +128,30 @@ FROM   v_user_auth u
        LEFT JOIN v_user_last_login AS ull 
        ON        u.uuid = ull.user_uuid
 WHERE  u.removed = false 
-`
-
-		selectGetAllUsersStmt, err := st.Prepare(getAllUsersQuery, dbUser{}, sqlair.M{})
-		if err != nil {
-			return errors.Annotate(err, "preparing select getAllUsers query")
-		}
+`, dbUser{})
+	if err != nil {
+		return nil, errors.Annotate(err, "preparing select getAllUsers query")
+	}
 
-		var results []dbUser
+	var results []dbUser
+	err = db.Txn(ctx, func(ctx context.Context, tx *sqlair.TX) error {
 		err = tx.Query(ctx, selectGetAllUsersStmt).GetAll(&results)
 		if err != nil && !errors.Is(err, sqlair.ErrNoRows) {
 			return errors.Annotate(err, "getting query results")
 		}
-
-		for _, result := range results {
-			usrs = append(usrs, result.toCoreUser())
-		}
-
 		return nil
 	})
 	if err != nil {
 		return nil, errors.Annotate(err, "getting all users")
 	}
 
+	var usrs []user.User
+	for _, result := range results {
+		if !result.Disabled || includeDisabled {
+			usrs = append(usrs, result.toCoreUser())
+		}
+	}
+
 	return usrs, nil
 }
 

domain/access/state/user_test.go

@@ -752,8 +752,8 @@ func (s *userStateSuite) TestGetAllUsersWihAuthInfo(c *gc.C) {
 	err = st.DisableUserAuthentication(context.Background(), "admin2")
 	c.Assert(err, jc.ErrorIsNil)
 
-	// Get all users with auth info.
-	users, err := st.GetAllUsers(context.Background())
+	// Get all users with auth info, including disabled users.
+	users, err := st.GetAllUsers(context.Background(), true)
 	c.Assert(err, jc.ErrorIsNil)
 
 	c.Assert(users, gc.HasLen, 2)
@@ -773,6 +773,20 @@ func (s *userStateSuite) TestGetAllUsersWihAuthInfo(c *gc.C) {
 	c.Check(users[1].CreatedAt, gc.NotNil)
 	c.Check(users[1].LastLogin, gc.NotNil)
 	c.Check(users[1].Disabled, gc.Equals, true)
+
+	// Get all users with auth info, excluding disabled users
+	users, err = st.GetAllUsers(context.Background(), false)
+	c.Assert(err, jc.ErrorIsNil)
+
+	c.Assert(users, gc.HasLen, 1)
+
+	c.Check(users[0].Name, gc.Equals, "admin1")
+	c.Check(users[0].DisplayName, gc.Equals, "admin1")
+	c.Check(users[0].CreatorUUID, gc.Equals, admin1UUID)
+	c.Check(users[0].CreatorName, gc.Equals, "admin1")
+	c.Check(users[0].CreatedAt, gc.NotNil)
+	c.Check(users[0].LastLogin, gc.NotNil)
+	c.Check(users[0].Disabled, gc.Equals, false)
 }
 
 // TestUserWithAuthInfo asserts that we can get a user with auth info from the