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.

Sign up for GitHub

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

Add functionality to Enable/Disable users #782

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Add functionality to Enable/Disable users #782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7bcc727
db: add Disabled property to UserRow struct
shiv-tyagi Feb 7, 2025
5a0c8ea
users: add method to check if a user is disabled
shiv-tyagi Feb 7, 2025
7db3b90
pam: check if the user is disabled before creating session
shiv-tyagi Feb 7, 2025
54b8833
pam: add test case Error_when_user_is_disabled while selecting broker
shiv-tyagi Feb 15, 2025
7c8e8f0
internal/users: implement logic to enable/disable user
shiv-tyagi Feb 18, 2025
f6ad608
nss: add API methods to enable/disable user
shiv-tyagi Feb 18, 2025
7061140
nss: add tests for DisableUser/EnableUser API methods
shiv-tyagi Feb 19, 2025
85c7275
create authctl cli tool
shiv-tyagi Feb 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions cmd/authctl/main.go
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to have integration tests for this, but if you don't have the time it would be nice if you could open an issue so that we can track it.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you want to see tested in these integration tests?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to have tests which start authd and then use authctl and check that it works as expected. That would be an end-to-end test which we don't have a framework for yet. I'm not sure if makes sense to have something in between the tests we already have and this end-to-end test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like to have tests which start authd and then use authctl and check that it works as expected.

Exactly what I mean, and what it should be done IMHO in an integration test, similar to the PAM ones.

Basically, adding a new (VHS based, sorry) test that:

  • Authenticates with an user
  • Disables it
  • Tries authentication again that must fail
  • Enables it back
  • It should be in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you point me to the examples of existing integration tests in the code? I will use them as a reference to write new ones.

Copy link
Collaborator

@3v1n0 3v1n0 Mar 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

❯ sudo gdbus call --system --dest org.freedesktop.Accounts --object-path /org/freedesktop/Accounts/User1001 --method org.freedesktop.Accounts.User.SetLocked true

❯ gdbus call --system --dest org.freedesktop.Accounts --object-path /org/freedesktop/Accounts --method org.freedesktop.Accounts.ListCachedUsers
([objectpath '/org/freedesktop/Accounts/User1000', '/org/freedesktop/Accounts/User1001'],)

But indeed that's marked as locked and so we filtering it out in gdm...

However... What lock does is that eventually calls usermod -L -- $USER that indeed would fail for authd users, and so the user wouldn't never be marked as locked either.

In fact:

$ sudo gdbus call --system --dest org.freedesktop.Accounts --object-path /org/freedesktop/Accounts/User65692 --method org.freedesktop.Accounts.User.SetLocked true
Errore: GDBus.Error:org.freedesktop.Accounts.Error.Failed: running '/usr/sbin/usermod' failed: Processo figlio uscito con codice 6

Thus it wouldn't be enough... Account service has support for vendor overrides, but I'm unsure if they can help either

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If integration tests are not a blocker to merge this work, can we create another issue for that?

Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// Package main implements Cobra commands for management operations on authd.
package main

import (
"fmt"
"os"

"github.com/spf13/cobra"
"github.com/ubuntu/authd/cmd/authctl/user"
)

const cmdName = "authctl"

var rootCmd = &cobra.Command{
Use: fmt.Sprintf("%s COMMAND", cmdName),
Short: "CLI tool to interact with authd",
Long: "authctl is a CLI tool which can be used to interact with authd.",
Args: cobra.NoArgs,
Run: func(cmd *cobra.Command, args []string) {},
}

func init() {
rootCmd.AddCommand(user.UserCmd)
}

func main() {
if err := rootCmd.Execute(); err != nil {
fmt.Fprintln(os.Stderr, err.Error())
os.Exit(1)
}
}
41 changes: 41 additions & 0 deletions cmd/authctl/user/disable.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package user

import (
"context"
"fmt"
"os"

"github.com/spf13/cobra"
"github.com/ubuntu/authd/internal/consts"
"github.com/ubuntu/authd/internal/proto/authd"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"
)

// disableCmd is a command to disable a user.
var disableCmd = &cobra.Command{
Use: "disable",
Short: "Disable a user managed by authd",
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
fmt.Printf("Disabling user %q\n", args[0])

authdSocket := os.Getenv("AUTHD_SOCKET")
if authdSocket == "" {
authdSocket = "unix://" + consts.DefaultSocketPath
}

conn, err := grpc.NewClient(authdSocket, grpc.WithTransportCredentials(insecure.NewCredentials()))
if err != nil {
return err
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also check for the HealthCheck to be around? See pam adapter model but it can be simpler here.

Copy link
Contributor

Choose a reason for hiding this comment

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

What benefit would that provide here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, do not make the request when the socket is there but not ready (so in a socket-activated scenario) or in tests where the daemon may take some time to show up.

Copy link
Contributor

@adombeck adombeck Mar 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Mmh, I don't really understand how calling the Health.Check RPC method instead of the NSS.DisableUser RPC method would help us when the daemon is not ready. Both have a timeout and if the daemon is ready before the timeout then they succeed, otherwise they fail.

My understanding of the gRPC health checks is that they are useful for two things: monitoring the status of gRPC services and choosing a healthy service from a list of services. Both don't apply to our use case.

client := authd.NewNSSClient(conn)
_, err = client.DisableUser(context.Background(), &authd.DisableUserRequest{Name: args[0]})
if err != nil {
return err
}

return nil
},
}
41 changes: 41 additions & 0 deletions cmd/authctl/user/enable.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package user

import (
"context"
"fmt"
"os"

"github.com/spf13/cobra"
"github.com/ubuntu/authd/internal/consts"
"github.com/ubuntu/authd/internal/proto/authd"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"
)

// enableCmd is a command to enable a user.
var enableCmd = &cobra.Command{
Use: "enable",
Short: "Enable a user managed by authd",
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
fmt.Printf("Enabling user %q\n", args[0])

authdSocket := os.Getenv("AUTHD_SOCKET")
if authdSocket == "" {
authdSocket = "unix://" + consts.DefaultSocketPath
}

conn, err := grpc.NewClient(authdSocket, grpc.WithTransportCredentials(insecure.NewCredentials()))
if err != nil {
return err
}

client := authd.NewNSSClient(conn)
_, err = client.EnableUser(context.Background(), &authd.EnableUserRequest{Name: args[0]})
if err != nil {
return err
}

return nil
},
}
19 changes: 19 additions & 0 deletions cmd/authctl/user/user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Package user provides utilities for managing user operations.
package user

import (
"github.com/spf13/cobra"
)

// UserCmd is a command to perform user-related operations.
var UserCmd = &cobra.Command{
Use: "user",
Short: "Commands related to users",
Args: cobra.NoArgs,
Run: func(cmd *cobra.Command, args []string) {},
}

func init() {
UserCmd.AddCommand(disableCmd)
UserCmd.AddCommand(enableCmd)
}
Loading
Loading