Skip to content

Add system user with special uuid #2428

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.

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

Closed
wants to merge 3 commits into from

Conversation

AbhineetJain
Copy link
Contributor

This PR adds a system user to assign an initiator for auto start/stop workspace builds.

Subtasks

  • create a system user migration
  • create a system user in the fake database
  • use the system user as initiator for auto start/stop builds
  • add unit test
  • update get user queries to filter out the system user

@AbhineetJain AbhineetJain requested a review from a team June 16, 2022 18:25
Copy link
Member

@Emyrk Emyrk left a comment

Choose a reason for hiding this comment

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

Minor comments

Copy link
Member

@Emyrk Emyrk left a comment

Choose a reason for hiding this comment

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

I am ok with the system user coming from a migration 👍

@AbhineetJain
Copy link
Contributor Author

Addresses the addition of a system user as discussed in #2029.

@AbhineetJain AbhineetJain enabled auto-merge (squash) June 16, 2022 18:42
users;
users
WHERE
id != '11111111-1111-1111-1111-111111111111';
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe this is a valid uuid, it needs some certain bits set. See https://github.com/google/uuid/blob/master/version4.go#L53

Copy link
Member

Choose a reason for hiding this comment

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

TIL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! Updated it to c0de2b07-0000-4000-A000-000000000000.

@AbhineetJain AbhineetJain disabled auto-merge June 16, 2022 18:43
@AbhineetJain AbhineetJain force-pushed the abhineetjain/2029-add-system-user branch from 23e3aac to e5dfd1c Compare June 16, 2022 19:15
Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

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

This change allows the system user to own workspaces, create organizations, and perform other actions... like a normal user.

If the primary purpose for this change is to show that "Coder" initiated an action in the build-log, have we considered dropping the foreign key on workspace builds for initiator and using a special UUID there? Or adding another column?

Code changes like GetActualUserCount feel very jarring to me. What do you mean by actual user count? Isn't the total number of users the actual user count? I think this is likely to introduce bugs as we move forward. I've already regretted having default resources inside a database that are required for a system to function.

"golang.org/x/xerrors"
)

var SystemUserID uuid.UUID = uuid.MustParse("c0de2b07-0000-4000-A000-000000000000")
Copy link
Member

Choose a reason for hiding this comment

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

Let's add a comment here. Why was this UUID chosen? Why not uuid.Nil?

Copy link
Member

Choose a reason for hiding this comment

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

uuid.Nil already has meaning in the db. We use it as a null in search filters since sqlc didn't support nullable args at the time. (It does as of like a week ago, I made a bug report on it as I ran into some issues using sqlc.narg)

Comment on lines +1 to +12
INSERT INTO
users (
id,
email,
username,
hashed_password,
created_at,
updated_at,
rbac_roles
)
VALUES
('c0de2b07-0000-4000-A000-000000000000', 'system@coder.com', 'system', '', NOW(), NOW(), '{}');
Copy link
Member

Choose a reason for hiding this comment

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

Should this user's username be coder? Seems a lot more friendly.

@Emyrk
Copy link
Member

Emyrk commented Jun 16, 2022

This change allows the system user to own workspaces, create organizations, and perform other actions... like a normal user.

It can but no one can log into it because we don't support empty string passwords in the /login route. So the user can only be controlled by backend actions.

If the primary purpose for this change is to show that "Coder" initiated an action in the build-log, have we considered dropping the foreign key on workspace builds for initiator and using a special UUID there? Or adding another column?

Dropping the foreign key relation would be unfortunate. This foreign key relation is going to prevent orphaned resources in the DB in the future. I personally prefer db constraints where possible to ensure clean data relations, otherwise when you do db.UserByID(ctx, build.InitiatorID) you will always get an error if the data model is messed up. Then all functions/endpoints that use this fail.

Foreign key relations ensure our expected relations can be expected. I'd rather put a null field then a special ID that we just need to know what it is, because now we need to do a

if id == database.SystemUser.ID {
  user = database.SystemUser
} else {
  user, err = db.UserByID(ctx, id)
}

If anything I'd prefer another column for a reason. The user being the initiator is still kinda strange...

Code changes like GetActualUserCount feel very jarring to me. What do you mean by actual user count? Isn't the total number of users the actual user count? I think this is likely to introduce bugs as we move forward. I've already regretted having default resources inside a database that are required for a system to function.

I don't think it is that hard to maintain. Our user queries by default never return the system user, so it can only be queried by ID. UserCount is something that is critical to licensing. Outside of that, UserCount is really going to be a niche use case like the "FirstUser" behavior. We can call it GetLicensedUsers or something in that regard. We can maybe even just mark the system user as some special status that is not active to omit it from all queries by default.


The question is either we introduce system user in the data model and keep the data model clean, or we keep system user in the application logic.

Both have costs and can introduce bugs imo. I think engineers are likely to take the data model for granted and assume user_id fields can always be fetched.

@AbhineetJain
Copy link
Contributor Author

AbhineetJain commented Jun 16, 2022

@kylecarbs @Emyrk How about we keep a non-null initiator_id and a non-null reason column? The following combinations can arise:

Initiator Reason
initiator_id Reason.User
owner_id Reason.Autostart
owner_id Reason.Autostop

In case of Reason.Autostart and Reason.Autostop, we ignore the initiator field when displaying it in the UI, and use the initiator field for the other case - the member could be admin too (or whoever the user may possibly be sharing workspaces with).

This way, we also avoid having any system user or special UUID scenarios.

@AbhineetJain
Copy link
Contributor Author

Closing this PR. #2438 has the changes suggested above.

@github-actions github-actions bot deleted the abhineetjain/2029-add-system-user branch February 4, 2023 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants