Add functionality to Enable/Disable users

[shiv-tyagi]

trafficstars

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]

@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]

Codecov Report

:x: Patch coverage is 76.78571% with 26 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 87.90%. Comparing base (229aa56) to head (3585468). :warning: Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/authctl/main.go	30.76%	9 Missing :warning:
internal/users/db/migration.go	83.33%	4 Missing :warning:
cmd/authctl/user/user.go	72.72%	3 Missing :warning:
cmd/authctl/user/lock.go	71.42%	2 Missing :warning:
cmd/authctl/user/unlock.go	71.42%	2 Missing :warning:
internal/services/pam/pam.go	75.00%	2 Missing :warning:
internal/services/user/user.go	85.71%	2 Missing :warning:
internal/users/db/update.go	80.00%	2 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #782      +/-   ##
==========================================
- Coverage   88.10%   87.90%   -0.21%     
==========================================
  Files          85       89       +4     
  Lines        6013     6125     +112     
  Branches      111      111              
==========================================
+ Hits         5298     5384      +86     
- Misses        659      685      +26     
  Partials       56       56              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[adombeck]

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]

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]

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]

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 :)

[adombeck]

I rebased on main and resolved the conflicts caused by https://github.com/ubuntu/authd/pull/779.

[shiv-tyagi]

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]

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]

@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.

[shiv-tyagi]

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

[adombeck]

Looks great, thanks for addressing the comments, @shiv-tyagi! I just added one more comment. Once you resolved that, I plan to merge this and create follow-up tasks for adding documentation and actually shipping the tool as part of the authd package.

[adombeck]

Rebased on main, resolved conflicts and amended the commits which add the new API methods to add them to the new user service instead of the NSS service.

[shiv-tyagi]

Rebased on main, resolved conflicts and amended the commits which add the new API methods to add them to the new user service instead of the NSS service.

Thanks @adombeck! I was about the pick the rebasing part myself, but you were fast :) I just went to see if there is anything left to be done from my side, looks like there isn't.

[shiv-tyagi]

Hey @adombeck @3v1n0, Is there anything pending from my side which is blocking this? Nothing is perfect I believe, even if there is some polishing left, we can always do follow up PRs I think. Even the renaming of the commands could have been done in another PR I think. If we keep waiting, things would always keep coming up.

[3v1n0]

We're about to prepare a new release, so once that lands we can open the gates to main again.

I would add tests to this branch though, but don't worry I'll handle them myself.

[adombeck]

Hi @shiv-tyagi! No, this is not blocked by anything from your side. It's blocked by us being busy with the new release, and by the fact that more important things kept popping up. I also hope that we can merge this soon.

Even the renaming of the commands could have been done in another PR I think.

I disagree on that. Once this has been merged to main, it will be part of the next release, which could in theory happen any time. Changing the names of the commands is a breaking change (i.e. if the command is used in a script, that script will break), which I would like to avoid once it's released.

[adombeck]

Rebased on main

[adombeck]

I added a schema migration to add the 'locked" column to the database. @3v1n0 could you review these commits?

[adombeck]

I added shell completion scripts for authctl.

[adombeck]

I added integration tests, fixed error messages, and did various other small fixes and improvements.

@3v1n0 Could you review my commits?

[3v1n0]

@adombeck I added a simple lock/unlock test using SSH, for more just follow the example in https://github.com/ubuntu/authd/pull/782/commits/a298b8577abaa0c1b0672046e7c5127ea13e5d4f

[adombeck]

Rebased on main and resolved the conflicts caused by https://github.com/ubuntu/authd/pull/993

[shiv-tyagi]

Thanks @adombeck for all the work you did.

Any update on this @3v1n0?

[adombeck]

Thanks @adombeck for all the work you did.

Any update on this @3v1n0?

@3v1n0 is out of office until August, and I will be out of office soon until the second week of August, so I don't expect things to move forward until then. I'm sorry this is taking so long! It seems the issue wasn't such a good candidate after all, because there was a lot more work and iterations of reviews involved than I first anticipated :/

[shiv-tyagi]

No worries @adombeck. I just keep bugging to make sure this doesn't get lost. Once this one is merged, I plan to add more functionality to it or find another issue to work on.

[shiv-tyagi]

@3v1n0 Sorry for bugging you again. Can you look into this?

[shiv-tyagi]

Meanwhile, @adombeck Can you please point me to a good beginner friendly issue_? I might go on holidays soon. So I was looking for something to contribute to.

[adombeck]

Meanwhile, @adombeck Can you please me to a good beginner friendly issue_? I might go on holidays soon. So I was looking for something to contribute to.

@shiv-tyagi: I looked through a lot of old issues and identified two which seem like relatively small changes:

• https://github.com/ubuntu/authd/issues/957 As you can see on the issue, we haven't finished discussing that one yet. However, I expect that we want to implement that and that the code change will be small (but require adding/changing test cases, which will be a bit more effort, but you already have some experience with that now).

• https://github.com/ubuntu/authd/issues/726 That's about adding a new setting (we haven't decided the name yet, maybe disable_local_password) for the broker config file to disable local password support. I expect this to require more code changes than the other issue, but it's still relatively straightforward when you look at how other settings are passed and used throughout the code. Again, the main part of the work will be to add tests. This one also requires documentation (the new setting must be explained on https://documentation.ubuntu.com/authd/stable/howto/configure-authd/), but that could be done as a follow-up, and not necessarily by you.

[adombeck]

Many thanks for the great contribution, @shiv-tyagi!