Test locking and unlocking a user after DB migration #1072
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This also adds tests for the same.
That's the default behavior of cobra, which was overridden with an empty Run function.
Print the `user` command before the `help` and `completion` commands.
That function depends on columns that might not exist yet, if the column is only added by a later migration.
Z_ForTests_CreateDBFromYAML creates the database with the current schema, but we want to test migrating a database with an old schema. We don't want to commit a binary database, so we create it from a SQLite dump instead. To produce the SQLite dumps, I applied this patch and ran the tests: diff --git a/internal/users/db/testutils.go b/internal/users/db/testutils.go index af7fe35..5f16a63a8 100644 --- a/internal/users/db/testutils.go +++ b/internal/users/db/testutils.go @@ -10,6 +10,7 @@ import ( "fmt" "io" "os" + "os/exec" "path/filepath" "sort" @@ -201,6 +202,14 @@ func createDBFromYAMLReader(r io.Reader, destDir string) (err error) { } log.Debug(context.Background(), "Database created") + + cmd := exec.Command("sqlite3", db.path, ".dump") + out, err := cmd.CombinedOutput() + if err != nil { + return fmt.Errorf("failed to dump database: %w, output: %s", err, string(out)) + } + fmt.Printf("XXX: Database dump:\n%s", string(out)) + return nil }
We added a new schema migration, so the schema version is now 2.
The testdata is for a specific schema migration, which is now encoded in the filepath.
These were created by running:
go run ./cmd/authctl/main.go completion bash > ./shell-completion/bash/authctl
go run ./cmd/authctl/main.go completion zsh > ./shell-completion/zsh/_authctl
go run ./cmd/authctl/main.go completion fish > ./shell-completion/fish/authctl.fish
We ship the shell completion scripts with the Debian package now, so there is no need for the user to generate their own scripts, and we can avoid cluttering the usage message with that command.
Might be useful for users who want to use authctl in scripts.
Except if the error is related to argument parsing.
Apparently, everything after the first space is not used anywhere, so we can just omit it.
I don't know why, but specifying "Args: cobra.NoArgs" without a Run or RunE function has this effect.
I would expect a function called RunDaemon to start the daemon and block until it has finished running. StartDaemon makes it clear that the function returns once the daemon was started.
The BuildDaemon() function is only called to build the daemon for integration tests with the example broker. Lets avoid passing the same arguments everywhere. Also renames the function to BuildDaemonWithExampleBroker.
Everywhere we called StartDaemon(), we registered the cleanup of the daemon process via t.Cleanup(). Let's avoid the duplicate code by inlining the cleanup registration into StartDaemon().
Lets be more consistent with other command-line tools which control system services, like systemctl and machinectl, and not print any output if the command succeeds (for example like `systemctl start`/`systemctl stop` or `machinectl kill`).
When trying to log in with a locked user, the following error message is
printed:
$ su [email protected]
can't select broker: error PermissionDenied from server: can't start authentication transaction: rpc error: code = PermissionDenied desc = user [email protected] is locked
The "can't start authentication transaction" part doesn't add any
valuable information to the error message, which is already too long.
By omitting it, we also avoid that the gRPC status is formatted to a
string containing the error code, which would duplicate information
which the caller adds to the error message.
With this commit, the error message is simplified to:
can't select broker: error PermissionDenied from server: user [email protected] is locked
Following up on the error message printed when trying to log in as a
locked user, we can further improve the message:
$ su [email protected]
can't select broker: error PermissionDenied from server: user [email protected] is locked
by omitting the "can't select broker:" part, which doesn't add any
useful information.
With this commit, the error message is simplified to:
error PermissionDenied from server: user [email protected] is locked
Further improve the error message printed when trying to log in with a
locked user from:
error PermissionDenied from server: user [email protected] is locked
to
permission denied: user [email protected] is locked
Co-Authored-By: Adrian Dombeck <[email protected]>
Same as pam_unix, we now avoid leaking to unauthenticated users whether a user account is locked. That's achieved by only checking if the user is locked after they successfully authenticated to the broker, aborting login in that case. That has the side effect that locked users can still refresh the token and user info which the broker stores on disk. This commit also implements the same improvements to the "permission denied: user is locked" error message which were already implemented in the SelectBroker method. I'm not reverting the changes to the SelectBroker method because I think they are still improvements, even if they are not relevant for this specific error message anymore.
I'm adding a test case which doesn't modify the local groups file, so the backup file doesn't exist.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As requested in #782 (comment)
This is based on #1046, please review and merge after that
UDENG-7903