-
Notifications
You must be signed in to change notification settings - Fork 936
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments
There was a problem hiding this 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 👍
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.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL
There was a problem hiding this comment.
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
.
23e3aac
to
e5dfd1c
Compare
There was a problem hiding this 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") |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
)
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.
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.
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