Add model last login to domain #17332

Aflynn50 · 2024-05-02T15:33:51Z

Juju stores the time each user last logged in to each model. It previously did this via a collection in mongo. This PR adds a table to the access service that tracks the users last logins.

The last login column is removed from the user table and the users last login is instead calculated by selecting the most recent login by that user to any model from the new table.

Currently we are double writing the last login to mongo and to DQLite but a subsequent PR will remove the need to do that by replacing all the remaining last login calls.

The tests for ListModels in the model-manager are currently commented out so this functionality is tests. I had a go at fixing the tests but realised that ListModels itself does not work as expected and fixing that is out of scope of this PR. I have coordinated with @tlm and he will land a patch for this soon.

Checklist

Code style: imports ordered, good names, simple structure, etc
Comments saying why design decisions were made
Go unit tests, with comments saying what you're testing

QA steps

The QA for this PR is a little difficult since the login and permissions stories are currently in flux. Specifically, when a is granted access to a model and logs in they do not see the model when they do juju models (this is likely because of the changes by @tlm that move ListModels to use state). There is also the issue that adding a model and accessing it does not trigger the expected login code and therefore, new models added do not have their last login updated.

The extent of the QA possible right now is:

$ juju bootstrap lxd test-controller
$ juju models
Controller: test-controller

Model       Cloud/Region  Type  Status     Machines  Units  Access  Last connection
controller  lxd/default   lxd   available         1      1  admin   just now

In the future, it should be possible to add a model and see that the last connection is just now. For the moment it says never connected.

$ juju add-model default
$ juju models
Controller: test-controller

Model       Cloud/Region  Type  Status     Machines  Units  Access  Last connection
controller  lxd/default   lxd   available         1      1  admin   just now
default*    lxd/default   lxd   available         0      -  admin   never connected

It should also be possible to see the last logins of other users, however listing the models of other users is currently not working. You should be able to do:

$ juju add-user test-user
$ juju change-user-password test-user
$ juju grant test-user admin default
$ juju models --user test-user
// Or instead of listing the models as the admin, you could do
$ juju logout 
$ juju login -u test-user -c test-controller
$ juju models

Links

Jira card: JUJU-5835

manadart · 2024-05-02T16:40:39Z

internal/worker/httpserverargs/authenticator.go

@@ -34,7 +35,7 @@ type UserService interface {
 GetUserByName(ctx context.Context, name string) (coreuser.User, error)
 // UpdateLastLogin updates the last login time for the user with the
 // given name.
- UpdateLastLogin(ctx context.Context, name string) error
+ UpdateLastLogin(ctx context.Context, modelUUID coremodel.UUID, name string) error


Given that it's the access (user) service, put the name first as the most relevant argument.

Good point, changed for this and for LastModelLogin as well

manadart · 2024-05-02T16:55:46Z

domain/schema/controller.go

+SELECT time AS last_login, user_uuid
+FROM model_last_login 
+GROUP BY user_uuid
+ORDER BY time DESC LIMIT 1;


This won't work. It limits the whole query to one row.

If you treat the max as a dummy value, you can still select time, because you're grouping by the user.

SELECT time AS last_login, user_uuid, MAX(time) as _.

Ah nice, changed to that

hmlanigan · 2024-05-08T17:47:11Z

@Aflynn50 please run the (cd tests ; ./main.sh user) tests with this work. They are failing in CI and I believe this PR will fix them.

tlm · 2024-05-23T18:49:46Z

Hey @Aflynn50,

Thanks for the pickup on the bug. I have a had a look at the code and I think the bug has always been there I just manage to re-implement the bug in DQlite perfectly.

Under the way it use to be for state we called ModelBasicInfoForUser passing in the requested user tag to run the query against and also isAdmin which is a bool for the currently logged in user and not that of the user we are necessarily querying about. For this example we will say the logged in user is Bob who is an admin and Bob is querying about Joes models.
See:

juju/apiserver/facades/client/modelmanager/modelmanager.go

Line 769 in 5be5c43

modelInfos, err := m.state.ModelBasicInfoForUser(userTag, m.isAdmin)

ModelBasicInfoForUser calls ModelBasicInfoForUser https://github.com/juju/juju/blob/5be5c4376d052fdaf34b1da802dec7a8148a40ab/state/modeluser.go#L210 which looks at the isAdmin flag. and decides what query to run. Now because Bob is an admin we will get all models and return them even though we have asked about only Joes models.

Take a look at the 3.6 branch and see if you can confirm that this was already a bug we had and I have just managed to replicate it in Dqlite?

Assuming the above is correct I think we just need to decide what the logic should be and I am happy to throw in the change.

tlm

Thanks @Aflynn50, this is a very nice PR!

tlm · 2024-05-23T18:51:49Z

apiserver/admin_test.go

@@ -967,7 +968,7 @@ func (s *loginSuite) TestLoginUpdatesLastLoginAndConnection(c *gc.C) {
 model := s.ControllerModel(c)
 modelUser, err := model.State().UserAccess(names.NewUserTag("bobbrown"), model.ModelTag())
 c.Assert(err, jc.ErrorIsNil)
- when, err := model.LastModelConnection(modelUser.UserTag)
+ when, err := userService.LastModelLogin(context.Background(), modelUser.UserTag.Name(), coremodel.UUID(model.UUID()))


Small nit..... I am wondering if the arg order need to be swapped around here to be ctx, model id, user id. To align with how we are asking the question. i.e For this model when did this user last login.

I checked with Joe about this, we decided that the user should go first because this is on the user service. I see your point though, I did have it the other way round originally

tlm · 2024-05-23T18:54:57Z

domain/access/errors/errors.go

+
+ // UserNeverConnectedToModel describes an error that occurs if a user has
+ // never connected to a model.
+ UserNeverConnectedToModel = errors.ConstError("user never connected to model")


Please disregard if you don't agree. I am not sure the "connected" part here make a lot of sense. Connected isn't a term we currently use for Juju to describe model access and could be confusing we we are trying to rationalise what connected means.

Maybe simply "accessed" is what we are after?

Accessed sounds better, I'll change it. I copied this over from state where there were two confusingly similar concepts of a user logging in and a user connecting to a model. This PR replaces them with just the concept of login so this should go as well.

Could there be any problem with changing the wording of this error w.r.t. public APIs? I assume its something we can change with the move to 4.0 if there was but it would be good to know if changes to error messages like this could require changes in external clients.

No we will be fine changing wording. A lot of errors strings from deep in Juju end up a user errors unfortunately. Time and determination will fix this.

tlm · 2024-05-23T18:56:03Z

apiserver/facades/client/controller/controller.go

 if err != nil {
- if !state.IsNeverConnectedError(err) {
+ if !errors.Is(err, accesserrors.UserNeverConnectedToModel) {


I left a comment below around the term "connected" but I see why you chose it. To match what we already had.

tlm · 2024-05-23T18:56:41Z

apiserver/facades/client/controller/controller.go

@@ -404,9 +409,9 @@ func (c *ControllerAPI) AllModels(ctx context.Context) (params.UserModelList, er
 },
 }

- lastConn, err := model.LastModelConnection(c.apiUser)
+ lastConn, err := c.accessService.LastModelLogin(ctx, c.apiUser.Name(), coremodel.UUID(model.UUID()))
 if err != nil {


nit..... We can combine these to if checks together and remove a line of indentation.

Combining them would change the behavior. In the case where the error is UserNeverAccessedModel then combining them would cause it to be set to lastConn which it is not currently.

I'll change this clause to make it more explicit in setting lastConn to nil because right now its a bit obfuscated.

tlm · 2024-05-23T18:59:03Z

apiserver/stateauthenticator/context.go

 // given name.
- UpdateLastLogin(ctx context.Context, name string) error
+ UpdateLastModelLogin(ctx context.Context, name string, modelUUID coremodel.UUID) error


Same comments as below for fetching. I think the argument order needs to be swapped here.

See reply on LastModelLogin for why the user name is first.

tlm · 2024-05-23T19:00:27Z

domain/access/service/service.go

+ // - accesserrors.UserNotFound: When the user cannot be found.
+ // - accesserrors.UserNeverConnectedToModel: If there is no record of the user
+ // connecting to the model.
+ LastModelLogin(context.Context, string, coremodel.UUID) (time.Time, error)


There should probably exist a modelerrors.NotFound in this list for LastModelLogin ?

Currently the error returned in that case would be UserNeverAccessedModel. I've just changed it now though to do a check for a missing model in the case that it gets an ErrorNoRows from the database so that we return the correct error in that case. I've added it now to this comment too.

tlm · 2024-05-23T19:05:21Z

domain/access/state/user.go

 creator.name AS &dbUser.created_by_name
 FROM v_user_auth u
 LEFT JOIN user AS creator 
 ON u.created_by_uuid = creator.uuid
+ LEFT JOIN v_user_last_login AS ull 


Because this is a left join now what happens when last_login is null or empty string. Do we default off to the zero value of time?

We do yea. I'm not changing the behavior from before here, though whether its right is a different matter.
IMO the field on user should really be a pointer to time.Time. I'll check with the others if we should change this.

Just discussed this with the team. They're happy just leaving it as a non pointer and doing checkers for the 0 unix time rather than risking nil pointer errors.

tlm · 2024-05-23T19:08:38Z

domain/access/state/user.go

+ // The foreign key constrain may be triggered if the user or the
+ // model does not exist. However, the user must exist or the
+ // uuidForName query would have failed, so it must be the model.
+ return modelerrors.NotFound


Can we please add more context to this error?

fmt.Errorf("updating last model login for user %q on model %q: %w")

tlm · 2024-05-23T19:10:10Z

domain/access/state/user.go

+// - accesserrors.UserNotFound: When the user cannot be found.
+// - accesserrors.UserNeverConnectedToModel: If there is no record of the user
+// connecting to the model.
+func (st *UserState) LastModelLogin(ctx context.Context, name string, modelUUID coremodel.UUID) (time.Time, error) {


Random question and thought. Currently we don't check to see if the model actually exists? Should we use a left join and do the work so we can be explicit and strict or no?

Happy to go either way. Just asking the question :)

Yea this is a good point. I've added a check in the case that we get no rows from the query to model last login. This way we're not doing the extra work when we don't need to but we are ensuring that it exists when it should.

tlm · 2024-05-23T19:11:29Z

domain/schema/controller.go

+CREATE TABLE model_last_login (
+ model_uuid TEXT NOT NULL,
+ user_uuid TEXT NOT NULL,
+ time TIMESTAMP,


Should this be NOT NULL?

Good point, added a not null.

I had left it as nullable because ideally we would actually insert a row in here on user creation (with a null timestamp) and use a update to add last logins. As it is we use an upsert to add the row in. I discussed this with Joe and right now we don't have a centralised place where we add users so we're not really ready to switch to doing that.

manadart

Some comments.

domain/access/service/user.go

manadart · 2024-05-28T13:51:55Z

apiserver/facades/client/modelmanager/listmodelsummaries_test.go

+ gomock.Any(),
+ "admin",
+ coremodel.UUID(s.st.ModelUUID()),
+ ).Return(time.Time{}, accesserrors.UserNeverAccessedModel)


it doesn't seem like you're ever testing the scenario where a valid time is returned.

True, I've added an actual value to TestListModelSummaries to test it there and reverted the change that removes it here. The others I have left as UserNeverAccessedModel.

apiserver/facades/client/modelmanager/modelmanager_test.go

manadart · 2024-05-28T13:57:28Z

domain/access/state/user.go

-WHERE uuid = $M.uuid`
+SELECT (u.uuid, u.name, u.display_name, u.created_by_uuid, u.created_at, ull.last_login, u.disabled) AS (&dbUser.*)
+FROM v_user_auth u
+LEFT JOIN v_user_last_login ull ON u.uuid = ull.user_uuid


Indent the join so it's under v_user_last_login.

manadart · 2024-05-28T13:57:37Z

domain/access/state/user.go

-AND removed = false`
+SELECT (u.uuid, u.name, u.display_name, u.created_by_uuid, u.created_at, ull.last_login, u.disabled) AS (&dbUser.*)
+FROM v_user_auth u
+LEFT JOIN v_user_last_login ull ON u.uuid = ull.user_uuid


Indent here too.

manadart · 2024-05-28T13:59:01Z

domain/access/state/user.go

+ return errors.Annotate(err, "preparing activation key deletion query")
+ }
+
+ deleteLastLoginStmt, err := st.Prepare("DELETE FROM model_last_login WHERE user_uuid = $M.uuid", m)


Given that deletion is a soft delete, and we haven't actually removed the record, there's no need to delete these entries. They might even prove useful for auditing purposes.

Ok, I'll remove this deletion

manadart · 2024-05-28T14:00:01Z

domain/access/state/user.go

+ }
+
+ getLastModelLoginTime := `
+ SELECT time AS &loginTime.time


Should be hard to the left, with vertical justification after the keywords.

Stop updating the mongoDB LastModelConnection Add checks to ensure password and activation were deleted by RemoveUser. The fact that RemoveUser deleted these values was previously untested.

manadart · 2024-06-03T09:32:32Z

/build

manadart

Thanks for seeing all the changes through.

I was able to create a new user/model and verify directly against the DB that last login times are written as appropriate.

manadart · 2024-06-03T09:56:16Z

/merge

Aflynn50 changed the title ~~The model last login to domain~~ Add model last login to domain May 2, 2024

Aflynn50 force-pushed the just-update-last-login branch from 743ffe6 to f0298b0 Compare May 2, 2024 15:44

manadart reviewed May 2, 2024

View reviewed changes

hpidcock added the 4.0 label May 3, 2024

Aflynn50 force-pushed the just-update-last-login branch 4 times, most recently from b38813e to f5afdb4 Compare May 3, 2024 10:30

Aflynn50 mentioned this pull request May 3, 2024

Use model access #17323

Merged

3 tasks

Aflynn50 force-pushed the just-update-last-login branch 2 times, most recently from 56d12ee to be5ff3a Compare May 3, 2024 13:15

hpidcock added the has merge conflicts label May 6, 2024

Aflynn50 force-pushed the just-update-last-login branch 2 times, most recently from 651f249 to 4ba4774 Compare May 9, 2024 12:27

hpidcock removed the has merge conflicts label May 13, 2024

Aflynn50 force-pushed the just-update-last-login branch 3 times, most recently from 4222b9d to d2c97d8 Compare May 22, 2024 18:41

Aflynn50 requested a review from manadart May 23, 2024 11:58

tlm reviewed May 23, 2024

View reviewed changes

manadart reviewed May 28, 2024

View reviewed changes

Aflynn50 force-pushed the just-update-last-login branch from ed10424 to e310c98 Compare May 29, 2024 09:16

Aflynn50 requested a review from manadart May 29, 2024 12:51

hpidcock added the has merge conflicts label May 30, 2024

Add model_last_login table, update UpdateLastLogin and add ModeLastLogin

94c60ba

Stop updating the mongoDB LastModelConnection Add checks to ensure password and activation were deleted by RemoveUser. The fact that RemoveUser deleted these values was previously untested.

Aflynn50 force-pushed the just-update-last-login branch from 2568328 to 94c60ba Compare May 31, 2024 14:37

manadart removed the has merge conflicts label Jun 3, 2024

manadart approved these changes Jun 3, 2024

View reviewed changes

jujubot merged commit 9c849a8 into juju:main Jun 3, 2024
15 of 17 checks passed

Add model last login to domain #17332

Add model last login to domain #17332

Conversation

Aflynn50 commented May 2, 2024 • edited

Checklist

QA steps

Links

manadart May 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manadart May 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmlanigan commented May 8, 2024

tlm commented May 23, 2024

tlm 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

Aflynn50 May 28, 2024 • edited

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

manadart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Aflynn50 May 28, 2024 • edited

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

manadart commented Jun 3, 2024

manadart left a comment

Choose a reason for hiding this comment

manadart commented Jun 3, 2024

Aflynn50 commented May 2, 2024 •

edited

manadart May 2, 2024 •

edited

manadart May 2, 2024 •

edited

Aflynn50 May 28, 2024 •

edited

Aflynn50 May 28, 2024 •

edited