-
Notifications
You must be signed in to change notification settings - Fork 282
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
Store roles in database #5134
base: main
Are you sure you want to change the base?
Store roles in database #5134
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.
@lippserd Was letzte Schema? :)
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.
Please show sample queries of how roles of a sample user would be loaded. Also note that CTEs can be used.
I'm unsure of the icingaweb_permission
and icingaweb_restriction
tables. They seem to have zero to no benefit and only make things more complicated. What was your idea behind these tables?
Is ROW_FORMAT=DYNAMIC
really needed everywhere? I don't think so.
Please increase all name columns to 256. The SAM-Account-Name attribute can be that long.
WITH RECURSIVE rl(id, parent_id, unrestricted) AS (
SELECT id, parent_id, unrestricted FROM icingaweb_role
WHERE all_users OR all_groups
-- I guess an index over (all_users, all_groups) would be nice, wouldn't it?
OR id IN (SELECT role_id FROM icingaweb_role_user WHERE user_name=?)
-- Shall I swap icingaweb_role_user's PK's columns for this query by user_name?
OR id IN (SELECT role_id FROM icingaweb_role_group WHERE group_name IN (?, ?, ?))
-- Shall I swap icingaweb_role_group's PK's columns for this query by group_name?
UNION
-- This even handles relationship cycles. 😎
SELECT id, parent_id, unrestricted FROM icingaweb_role
WHERE id IN (SELECT parent_id FROM rl)
)
SELECT id, unrestricted FROM rl; SELECT name FROM icingaweb_permission WHERE id IN (
SELECT permission_id FROM icingaweb_role_permission WHERE role_id IN (?, ?, ?)
) SELECT (SELECT name FROM icingaweb_restriction WHERE id=irr.restriction_id) name, filter
FROM icingaweb_role_restriction irr WHERE role_id IN (?, ?, ?)
The same idea as behind Icinga DB's action_url and notes_url: deduplication of probably frequently used strings.
I thought this is our new guideline as Icinga DB uses this literally everywhere. (
It's length is actually limited to 20:
And my length limit was just inspired from the existing username columns which are btw. 254 long to contain eMail addresses. |
Btw.: Distros supporting CTEsvia MariaDB 10.2.1+
|
Now as I see it: It IS literally everywhere, even the damn simple table icingaweb_user of this schema. Let's keep it for consistency. |
No. It is not needed. And consistency is no reason to add it. New code should reflect how it's done properly and not how it was done ages ago. |
Red Hat states something else. |
That's what I wanted to express, sorry for the misunderstanding. Btw. Ubuntu 20.04 also has MySQL 8 and RHEL 7 has PostgreSQL (almost forgot!) 9.2. v8.4 has CTEs. |
It seems the default, so no – it's indeed not needed. |
WITH RECURSIVE rl(id, parent_id, unrestricted) AS (
SELECT id, parent_id, unrestricted FROM icingaweb_role
WHERE id IN (SELECT role_id FROM icingaweb_role_user WHERE user_name IN ('*', ?))
OR id IN (SELECT role_id FROM icingaweb_role_group WHERE group_name IN ('*', ?, ?, ?))
UNION -- This even handles relationship cycles. 😎
SELECT id, parent_id, unrestricted FROM icingaweb_role
WHERE id IN (SELECT parent_id FROM rl)
)
SELECT id, unrestricted FROM rl; |
48511e9
to
33a5a58
Compare
If this is an old schema, CREATE TABLE `icingaweb_role`(
`name` varchar(64) COLLATE utf8_unicode_ci NOT NULL,
`public` tinyint(1) NOT NULL DEFAULT 0,
`ctime` timestamp NULL DEFAULT NULL,
`mtime` timestamp NULL DEFAULT NULL ON UPDATE CURRENT_TIMESTAMP,
PRIMARY KEY (`name`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;
CREATE TABLE `icingaweb_role_permission`(
`role_name` varchar(64) COLLATE utf8_unicode_ci NOT NULL,
`name` varchar(255) COLLATE utf8_unicode_ci NOT NULL,
`ctime` timestamp NULL DEFAULT NULL,
`mtime` timestamp NULL DEFAULT NULL ON UPDATE CURRENT_TIMESTAMP,
PRIMARY KEY (`role_name`,`name`),
CONSTRAINT `fk_icingaweb_role_permission_icingaweb_role` FOREIGN KEY (`role_name`) REFERENCES `icingaweb_role` (`name`)
ON UPDATE CASCADE ON DELETE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8;
CREATE TABLE `icingaweb_role_restriction`(
`role_name` varchar(64) COLLATE utf8_unicode_ci NOT NULL,
`name` varchar(255) COLLATE utf8_unicode_ci NOT NULL,
`restriction` text NOT NULL,
`ctime` timestamp NULL DEFAULT NULL,
`mtime` timestamp NULL DEFAULT NULL ON UPDATE CURRENT_TIMESTAMP,
PRIMARY KEY (`role_name`,`name`),
CONSTRAINT `fk_icingaweb_role_restriction_icingaweb_role` FOREIGN KEY (`role_name`) REFERENCES `icingaweb_role` (`name`)
ON UPDATE CASCADE ON DELETE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8;
CREATE TABLE `icingaweb_user_role`(
`username` varchar(64) COLLATE utf8_unicode_ci NOT NULL,
`role_name` varchar(64) COLLATE utf8_unicode_ci NOT NULL,
`ctime` timestamp NULL DEFAULT NULL,
`mtime` timestamp NULL DEFAULT NULL ON UPDATE CURRENT_TIMESTAMP,
PRIMARY KEY (`username`,`role_name`),
CONSTRAINT `fk_icingaweb_user_role_icingaweb_role` FOREIGN KEY (`role_name`) REFERENCES `icingaweb_role` (`name`)
ON UPDATE CASCADE ON DELETE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8;
CREATE TABLE `icingaweb_group_role`(
`group_name` varchar(64) COLLATE utf8_unicode_ci NOT NULL,
`role_name` varchar(64) COLLATE utf8_unicode_ci NOT NULL,
`ctime` timestamp NULL DEFAULT NULL,
`mtime` timestamp NULL DEFAULT NULL ON UPDATE CURRENT_TIMESTAMP,
PRIMARY KEY (`group_name`,`role_name`),
CONSTRAINT `fk_icingaweb_group_role_icingaweb_role` FOREIGN KEY (`role_name`) REFERENCES `icingaweb_role` (`name`)
ON UPDATE CASCADE ON DELETE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8; we can
|
Our Docker entry point already handles users in DB, so roles wouldn't be a problem. But the others already can't manage users or groups. Roles would be just yet another blind point (unless you let Ansible do INSERT). I think we need a more general strategy. Like a bunch of CLI commands (in the setup mod? or even a new mod "cfgmgmt"?) which manage stuff stored in the IW2 DB. |
Meeting resultsThe comrades already populate the admin user (and role), so that you can login at all, but only initially not to fiddle around in the db, so status quo here is OK. Setup mod. CLI commands would be very nice, but -in our opinion- for all db stuff incl. user/groups. A separate (not small) project IMAO. A cherry on the cake would be a per-row flag (again for all db stuff, separate construction area) for managed-externally, so that users can't change those Puppet-deployed stuff temporarily and get mad because it disappears again. Question to @lippserd(?): when will the default roles backend change? |
…rate them This prevents the admin from breaking Icinga Web with one checkbox.
TODO