User: Add uid colum to user table #81615
Conversation
ryantxu
added
no-backport
grafana-pr-automation
bot
added
area/backend
area/frontend
area/backend/db/migration
labels
grafana-pr-automation
bot
requested review from
a team and
Ukochka
and removed request for
a team
grafana-pr-automation
bot
requested review from
a team
and removed request for
a team
grafana-pr-automation
bot
requested review from
a team
and removed request for
a team
| })) | ||
|
|
||
| mg.AddMigration("Update uid column values for users", NewRawSQLMigration(""). | ||
| SQLite("UPDATE user SET uid=printf('i%09d',id) WHERE uid IS NULL;"). |
There was a problem hiding this comment.
better ideas for a prefix? u?
Eventually this will land in the service account like:
user:i000034, or api-key:i000024
There was a problem hiding this comment.
is this not going to expose the internal id's of users? meaning you could get information about how many users in an instance and so forth
@jmatosgrafana you have any comments on this?
There was a problem hiding this comment.
🤷🏻 that is what we currently do so it won't be worse than that.
It would be nicer if we could fill these in with values from our uid generator, but this lets us do the update in SQL, for sure open to better options!
There was a problem hiding this comment.
This kind of id is not a secret: if it leaks it doesn't really help to achieve authentication bypass and there are other ways to guess user ids (e.g. changing your email address)
That is why we put enumeration out of scope in our bug bounty program.
grafana-pr-automation
bot
requested review from
a team
and removed request for
a team
| Mysql("UPDATE user SET uid=concat('i',lpad(id,9,'0')) WHERE uid IS NULL;")) | ||
|
|
||
| mg.AddMigration("Add unique index user_org_id_uid", NewAddIndexMigration(userV2, &Index{ | ||
| Cols: []string{"org_id", "uid"}, Type: UniqueIndex, |
There was a problem hiding this comment.
I think the unique index needs to be on only uid. I am pretty sure org_id in user table is just used to determine last used org so this value will change.
What this will do is prevent two users with the same uid to be active in the same org at once
There was a problem hiding this comment.
is just used to determine last used org
🤯 good to know! so much mystery in the orgs
| @@ -140,6 +140,19 @@ func addUserMigrations(mg *Migrator) { | |||
| SQLite(migSQLITEisServiceAccountNullable). | |||
| Postgres("ALTER TABLE `user` ALTER COLUMN is_service_account DROP NOT NULL;"). | |||
| Mysql("ALTER TABLE user MODIFY is_service_account BOOLEAN DEFAULT 0;")) | |||
|
|
|||
| mg.AddMigration("Add uid column to user", NewAddColumnMigration(userV2, &Column{ | |||
| Name: "uid", Type: DB_NVarchar, Length: 40, Nullable: false, Default: "", | |||
There was a problem hiding this comment.
As I mentioned in slack I think this has to be nullable, otherwise this would block new users if an instance has to be rolled back to a previous version
grafana-pr-automation
bot
requested review from
a team
and removed request for
a team
ryantxu
requested review from
mildwonkey,
undef1nd,
suntala,
Clarity-89,
L-M-K-B,
rwwiv,
JacobsonMT,
yuri-tceretian,
grobinson-grafana,
kalleep and
eleijonmarck
and removed request for
a team
| mg.AddMigration("Update uid column values for users", NewRawSQLMigration(""). | ||
| SQLite("UPDATE user SET uid=printf('u%09d',id) WHERE uid IS NULL;"). | ||
| Postgres("UPDATE `user` SET uid='u' || lpad('' || id::text,9,'0') WHERE uid IS NULL;"). | ||
| Mysql("UPDATE user SET uid=concat('u',lpad(id,9,'0')) WHERE uid IS NULL;")) |
There was a problem hiding this comment.
I think instead of running this as a one time migration we should run it on every start up for a couple of minors. Then we would handle a upgrade downgrade scenario better. With a one time migration any users added after a downgrade would not have a uid
There was a problem hiding this comment.
👍 with the unique index it will cause the DB to error out anyway on downgrade 😢
We might need have no unique index for a bit as well
EDIT: sqlite, mysql,postgres works ok with nullable unique index by default. mssql seems to be the odd one out
|
@Jguer -- i will merge this now, and add a new PR that fills in any possibly missing UIDs that could happen with a downgrade issue |
Similar to #66920 (and we should have done it then 😢)
This adds a UID table to the user; we can not depend on the internal auto-increment ids when we move to a multi-tenant model.
This PR takes the first step of adding UID for all users. It does not yet replace the widespread usage of internal id, and is not even able to pass it to the UI just yet.
Next/soon: the ID tokens should use UID not ID: https://github.com/grafana/grafana/blob/v10.3.1/pkg/services/authn/identity.go#L20