Add system user with special uuid #2428
Conversation
|
Addresses the addition of a system user as discussed in #2029. |
coderd/database/queries/users.sql
Outdated
| users; | ||
| users | ||
| WHERE | ||
| id != '11111111-1111-1111-1111-111111111111'; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Thank you! Updated it to c0de2b07-0000-4000-A000-000000000000.
23e3aac to
e5dfd1c
Compare
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
Let's add a comment here. Why was this UUID chosen? Why not uuid.Nil?
There was a problem hiding this comment.
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)
| 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(), '{}'); |
There was a problem hiding this comment.
Should this user's username be coder? Seems a lot more friendly.
It can but no one can log into it because we don't support empty string passwords in the
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 Foreign key relations ensure our expected relations can be expected. I'd rather put 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
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 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 |
|
@kylecarbs @Emyrk How about we keep a non-null
In case of This way, we also avoid having any |
|
Closing this PR. #2438 has the changes suggested above. |
This PR adds a system user to assign an initiator for auto start/stop workspace builds.
Subtasks