Add functionality to Enable/Disable users #782

shiv-tyagi · 2025-02-07T16:14:45Z

As discussed in #640, this adds a property to the UserDB to mark a user enable/disabled. Before creating an authentication session in pam, we check if the user exists in the cache and is disabled.

This also adds the initial implementation of the authctl tool which can be used to enable/disable a user.

shiv-tyagi · 2025-02-07T16:15:41Z

@adombeck Does this look good?

I see that the tests are failing on main branch as well. So I am hoping that it is not me.

codecov-commenter · 2025-02-07T16:22:07Z

Codecov Report

Attention: Patch coverage is 86.15385% with 9 lines in your changes missing coverage. Please review.

Project coverage is 79.08%. Comparing base (36511cd) to head (5152313).
Report is 449 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/users/db/update.go	57.14%	4 Missing and 2 partials ⚠️
internal/services/pam/pam.go	57.14%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #782      +/-   ##
==========================================
- Coverage   83.43%   79.08%   -4.36%     
==========================================
  Files          83      102      +19     
  Lines        8689    10442    +1753     
  Branches       74       74              
==========================================
+ Hits         7250     8258    +1008     
- Misses       1111     1715     +604     
- Partials      328      469     +141

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

adombeck · 2025-02-10T13:15:09Z

internal/services/pam/pam.go

@@ -138,6 +138,12 @@ func (s Service) SelectBroker(ctx context.Context, req *authd.SBRequest) (resp *
 		lang = "C"
 	}

+	// Throw an error if the user trying to authenticate already exists in cache and is disabled


We use the term "database" instead of "cache" now (related: #775)

Suggested change

// Throw an error if the user trying to authenticate already exists in cache and is disabled

// Throw an error if the user trying to authenticate already exists in the database and is disabled

adombeck · 2025-02-10T13:17:56Z

Hi @shiv-tyagi, yes this looks good, thanks! One small thing: I would prefer to have a Disabled field instead of Enabled, so that the default value (false) means that the user is enabled, even when we're not using NewUserDB to instantiate the UserDB struct (like we do here).

adombeck · 2025-02-10T13:20:16Z

There should also be a new test case "Error_when_user_is_disabled" in https://github.com/shiv-tyagi/authd/blob/fed39218cfcce6c23e80e0d02fe2a2d748d3d912/internal/brokers/manager_test.go#L187-L190.

adombeck · 2025-02-10T13:22:24Z

Oh and I don't think it makes sense to merge this before we have code which actually causes a user to be disabled (i.e. the command-line tool). If you still plan to work on that soon, feel free to repurpose this PR. Otherwise, I'll start working on that soon and would then cherry-pick your commits to a new branch.

shiv-tyagi · 2025-02-10T17:30:14Z

I would prefer to have a Disabled field instead of Enabled, so that the default value (false) means that the user is enabled, even when we're not using NewUserDB to instantiate the UserDB struct (like we do here).

Sure. I will do that.

There should also be a new test case "Error_when_user_is_disabled"

Noted.

If you still plan to work on that soon, feel free to repurpose this PR.

Yes, I really intend to work on that. I will push my work soon :)

3v1n0 · 2025-02-11T15:58:15Z

internal/users/manager.go

@@ -328,6 +328,16 @@ func (m *Manager) UpdateBrokerForUser(username, brokerID string) error {
 	return nil
 }

+// IsUserDisabled returns true if the user with the given user name is disabled, false otherwise
+func (m *Manager) IsUserDisabled(username string) (bool, error) {
+	usr, err := m.cache.UserByName(username)


Suggested change

usr, err := m.cache.UserByName(username)

u, err := m.cache.UserByName(username)

Fixed. Thanks.

adombeck · 2025-02-17T11:34:05Z

internal/services/pam/pam.go

+	userIsDisabled, err := s.userManager.IsUserDisabled(username)
+	if err == nil && userIsDisabled {
+		return nil, status.Error(codes.PermissionDenied, fmt.Sprintf("user %s is disabled", username))
+	}
+


Let's handle unexpected errors here

Suggested change

userIsDisabled, err := s.userManager.IsUserDisabled(username)

if err == nil && userIsDisabled {

return nil, status.Error(codes.PermissionDenied, fmt.Sprintf("user %s is disabled", username))

}

userIsDisabled, err := s.userManager.IsUserDisabled(username)

if err != nil && !errors.Is(err, users.NoDataFoundError{}) {

return nil, fmt.Errorf("could not check if user %q is disabled: %w", username, err)

}

if err == nil && userIsDisabled {

return nil, status.Error(codes.PermissionDenied, fmt.Sprintf("user %s is disabled", username))

}

Thanks for suggesting. I have incorporated this change.

adombeck · 2025-02-18T14:31:00Z

I rebased on main and resolved the conflicts caused by #779.

shiv-tyagi · 2025-02-18T17:41:33Z

I rebased on main and resolved the conflicts caused by #779.

Thanks for this @adombeck. I had some more work which I had not pushed yet. I would need to fix that as well. Planning to push that work soon.

shiv-tyagi · 2025-02-19T06:53:46Z

I have finished the part till extending the GRPC API with new methods and adding unit tests for those. Now only the part of creating the CLI tool to call those methods remains.

shiv-tyagi · 2025-02-26T05:28:29Z

@adombeck I have added the initial implementation of the authctl tool. Please check if it looks good and provide any suggestions for me to proceed. This is my first time writing Go code, so I apologise if I made any rookie mistakes. I’ll be happy to fix them.

Thanks in advance.

adombeck

I've got a few comments but the overall code style is good! Thanks a lot, @shiv-tyagi!

adombeck · 2025-02-26T16:32:38Z

internal/users/manager_test.go

+		wantErr     bool
+		wantErrType error
+	}{
+		"Successfully_update_broker_for_user": {},


This test case is not named correctly

Sorry that was a copy and paste error. I should have been more careful. I have fixed this.

adombeck · 2025-02-26T16:32:59Z

internal/users/manager_test.go

+		wantErr     bool
+		wantErrType error
+	}{
+		"Successfully_update_broker_for_user": {},


Same here, please update the test case name

Fixed this.

adombeck · 2025-02-26T16:39:23Z

internal/users/manager_test.go

+			// We don't care about the output of gpasswd in this test, but we still need to mock it.
+			_ = localgroupstestutils.SetupGPasswdMock(t, "empty.group")


I don't understand why we would need to mock gpasswd in this test, but I also don't see why that's done in many of the other tests in this file which are unrelated to /etc/group. I see that was introduced in fd10538, do you remember the reason, @denisonbarbosa?

adombeck · 2025-02-26T16:41:54Z

internal/users/manager_test.go

+		wantErr     bool
+		wantErrType error
+	}{
+		"Successfully_update_broker_for_user": {},


There should be a test case which uses testdata with a disabled user. Currently, it only tests enabling a user which was already enabled before.

I think I am already doing that. Please see this.

adombeck · 2025-02-26T16:52:14Z

internal/proto/authd/authd.proto

+  rpc DisablePasswd(DisablePasswdRequest) returns (Empty);
+  rpc EnablePasswd(EnablePasswdRequest) returns (Empty);


The GetPasswd... methods are named like that because they return an entry like the ones in the /etc/passwd file. This analogy doesn't really apply to the disable/enable functionality, so let's name these methods DisableUser and EnableUser.

adombeck · 2025-02-26T17:02:16Z

cmd/authctl/main.go

+
+func main() {
+	if err := rootCmd.Execute(); err != nil {
+		fmt.Fprintf(os.Stderr, "Error while executing authctl '%s'\n", err)


"Error while executing authctl" doesn't provide any useful info. Let's just print the error.

Suggested change

fmt.Fprintf(os.Stderr, "Error while executing authctl '%s'\n", err)

fmt.Fprintln(os.Stderr, err.Error())

adombeck · 2025-02-26T17:07:32Z

cmd/authctl/user/disable.go

+
+var DisableCmd = &cobra.Command{
+	Use:   "disable",
+	Short: "Disable a user to log in",


"Disable a user to log in" is not a correct sentence. Also, I would like to convey that this can only disable users managed by authd (which is also an opportunity to at least implicitly tell users that the user accounts created by authd are managed separately from other user accounts on the system), so how about:

Suggested change

Short: "Disable a user to log in",

Short: "Disable a user managed by authd",

Sounds better. Fixed that.

adombeck · 2025-02-26T17:09:30Z

cmd/authctl/user/disable.go

+		}
+
+		client := authd.NewNSSClient(conn)
+		_, err = client.EnablePasswd(context.Background(), &authd.EnablePasswdRequest{Name: args[0]})


That's the wrong API method, you want to call DisablePasswd.

Oops. Sorry. I have fixed that.

adombeck · 2025-02-26T17:13:45Z

cmd/authctl/user/disable.go

+		conn, err := grpc.NewClient("unix://"+consts.DefaultSocketPath, grpc.WithTransportCredentials(insecure.NewCredentials()))
+
+		if err != nil {
+			return err
+		}


nitpick: no newline between function call and error handling. please also change that in the other occurrences in cmd/authctl/user/disable.go and cmd/authctl/user/enable.go.

Suggested change

conn, err := grpc.NewClient("unix://"+consts.DefaultSocketPath, grpc.WithTransportCredentials(insecure.NewCredentials()))

if err != nil {

return err

}

conn, err := grpc.NewClient("unix://"+consts.DefaultSocketPath, grpc.WithTransportCredentials(insecure.NewCredentials()))

if err != nil {

return err

}

Fixed this.

adombeck · 2025-02-26T17:15:55Z

cmd/authctl/user/user.go

+
+var UserCmd = &cobra.Command{
+	Use:   "user",
+	Short: "Commands retaled to working with users",


Suggested change

Short: "Commands retaled to working with users",

Short: "Commands related to users",

Done. Thanks.

This also adds tests for the same.

shiv-tyagi · 2025-03-04T18:36:52Z

Thank you so much @adombeck for reviewing this. I have addressed all your suggestions. What next? How should I test it locally?

shiv-tyagi requested a review from a team as a code owner February 7, 2025 16:14

shiv-tyagi changed the title ~~Check is a user is disabled before logging in~~ Check if a user is disabled before logging in Feb 10, 2025

adombeck reviewed Feb 10, 2025

View reviewed changes

shiv-tyagi force-pushed the user-disable branch from e6410f4 to a7d839c Compare February 10, 2025 17:27

3v1n0 reviewed Feb 11, 2025

View reviewed changes

shiv-tyagi force-pushed the user-disable branch 4 times, most recently from 7183092 to 5e81b35 Compare February 15, 2025 18:22

adombeck reviewed Feb 17, 2025

View reviewed changes

adombeck force-pushed the user-disable branch from 5e81b35 to 54aae19 Compare February 18, 2025 14:30

shiv-tyagi force-pushed the user-disable branch 2 times, most recently from 7f1338f to fce5e88 Compare February 19, 2025 06:51

shiv-tyagi force-pushed the user-disable branch from fce5e88 to 7208138 Compare February 26, 2025 05:20

shiv-tyagi changed the title ~~Check if a user is disabled before logging in~~ Add functionality to Enable/Disable users Feb 26, 2025

adombeck reviewed Feb 26, 2025

View reviewed changes

shiv-tyagi force-pushed the user-disable branch from 7208138 to 33ddd8c Compare March 4, 2025 18:25

shiv-tyagi added 3 commits March 5, 2025 00:05

db: add Disabled property to UserRow struct

a7b3feb

users: add method to check if a user is disabled

986c9ec

pam: check if the user is disabled before creating session

91f2d51

shiv-tyagi added 5 commits March 5, 2025 00:05

pam: add test case Error_when_user_is_disabled while selecting broker

4dba7b3

internal/users: implement logic to enable/disable user

62b4a91

This also adds tests for the same.

nss: add API methods to enable/disable user

c2c03fe

nss: add tests for DisableUser/EnableUser API methods

cd5ae64

create authctl cli tool

5152313

shiv-tyagi force-pushed the user-disable branch from 33ddd8c to 5152313 Compare March 4, 2025 18:56

	// Throw an error if the user trying to authenticate already exists in cache and is disabled
	// Throw an error if the user trying to authenticate already exists in the database and is disabled

	usr, err := m.cache.UserByName(username)
	u, err := m.cache.UserByName(username)

		// We don't care about the output of gpasswd in this test, but we still need to mock it.
		_ = localgroupstestutils.SetupGPasswdMock(t, "empty.group")

		rpc DisablePasswd(DisablePasswdRequest) returns (Empty);
		rpc EnablePasswd(EnablePasswdRequest) returns (Empty);

	fmt.Fprintf(os.Stderr, "Error while executing authctl '%s'\n", err)
	fmt.Fprintln(os.Stderr, err.Error())

	Short: "Disable a user to log in",
	Short: "Disable a user managed by authd",

	Short: "Commands retaled to working with users",
	Short: "Commands related to users",

Add functionality to Enable/Disable users #782

Are you sure you want to change the base?

Add functionality to Enable/Disable users #782

Conversation

shiv-tyagi commented Feb 7, 2025 • edited Loading

shiv-tyagi commented Feb 7, 2025

codecov-commenter commented Feb 7, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adombeck commented Feb 10, 2025

adombeck commented Feb 10, 2025

adombeck commented Feb 10, 2025

shiv-tyagi commented Feb 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adombeck commented Feb 18, 2025

shiv-tyagi commented Feb 18, 2025

shiv-tyagi commented Feb 19, 2025

shiv-tyagi commented Feb 26, 2025

adombeck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shiv-tyagi commented Mar 4, 2025

shiv-tyagi commented Feb 7, 2025 •

edited

Loading

codecov-commenter commented Feb 7, 2025 •

edited

Loading