Skip to content
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

System: Access: migrate Users and Groups to MVC/API #8046

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

AdSchellevis
Copy link
Member

Initial draft for #7904

@AdSchellevis AdSchellevis added the feature Adding new functionality label Nov 7, 2024
@AdSchellevis AdSchellevis self-assigned this Nov 7, 2024
@AdSchellevis AdSchellevis marked this pull request as draft November 7, 2024 21:01
@Monviech
Copy link
Member

Monviech commented Nov 8, 2024

Things I notice while testing (not complete, will be edited)

/ui/auth/user

  • Pressing "Create and Download API Key for this user" does not refresh the "ApiKeys" bootgrid
  • The "Users" bootgrid could show more items in its table, initially hidden (e.g. E-Mail address or Comment)

/ui/auth/groups

  • "Username" should probably be "Group Name"

/ui/auth/priv

  • Sorting the bootgrid by "Users" or "Groups" does not seem to work as expected. E.g., when "page-diagnostics-ping - Groups 1", I would expect it to appear below "page-all - Groups 1" when Groups is sorted. But it does not.
  • Create a group in /ui/auth/group that has a name like test_group, do not give it any settings, privileges, users.
    Go to /ui/auth/priv, edited the all-pages privilege and add test_group to it (additionally) and save. The WebGUI stops working with "ERR_TOO_MANY_REDIRECTS", also root login does not work in serial console after a reboot.

Here is a diff for the last issue:

root@OPNsense:/conf/backup # diff -u config-1731060712.6972.xml config-1731069336.335.xml
--- config-1731060712.6972.xml	2024-11-08 11:11:52.701110000 +0100
+++ config-1731069336.335.xml	2024-11-08 13:35:36.339304000 +0100
@@ -206,21 +206,32 @@
     <optimization>normal</optimization>
     <hostname>OPNsense</hostname>
     <domain>localdomain</domain>
-    <group>
+    <group uuid="23e64452-52ff-4469-9e80-b97bcd18709b">
+      <gid>1999</gid>
       <name>admins</name>
-      <description>System Administrators</description>
       <scope>system</scope>
-      <gid>1999</gid>
+      <description>System Administrators</description>
+      <priv/>
       <member>0</member>
-      <priv>page-all</priv>
     </group>

@AdSchellevis
Copy link
Member Author

@Monviech first batch of changes pushed in f6b2a4f, the redirect issue I still have to investigate.

@Monviech
Copy link
Member

Thank you.

I think the main issue is not the Redirect error, its that when saving any "ID" in "System: Access: Privileges", the priv is stripped.

E.g. after saving ID page-dhcp-kea-v4 this is the config.xml diff:

root@OPNsense:/conf/backup # diff -u /conf/config.xml ./config-1731507909.2196.xml
--- /conf/config.xml	2024-11-13 15:25:16.313978000 +0100
+++ ./config-1731507909.2196.xml	2024-11-13 15:25:09.222917000 +0100
@@ -325,7 +325,7 @@
       <comment/>
       <email/>
       <apikeys/>
-      <priv>page-dhcp-kea-v4</priv>
+      <priv/>
       <language/>
       <descr/>
     </user>
@@ -843,8 +843,8 @@
   </widgets>
   <revision>
     <username>[email protected]</username>
-    <description>/api/auth/priv/set_item/page-dhcp-kea-v4 made changes</description>
-    <time>1731507916.3107</time>
+    <description>/api/auth/user/add/ made changes</description>
+    <time>1731507909.2196</time>
   </revision>
   <Deciso>
     <Unbound>

When doing this with the page-all ID, all privileges are stripped and as root you are immediately thrown out of the WebGUI.

Thats what I expect from the evidence.

@AdSchellevis
Copy link
Member Author

@Monviech ok, this 496191c should be the cause of the redirect issue.

This did trigger me about being able to update the admins group, if I'm not mistaking you could drop these in the old software as well, but when you do, you might render the root user "useless", which might be something we should guard against.

@Monviech
Copy link
Member

@AdSchellevis Hey this worked, the privilege is still there after saving page-all. Really nice work thanks.

@AdSchellevis
Copy link
Member Author

@Monviech thanks for testing, we're getting close to something mergable :)

@Monviech
Copy link
Member

Monviech commented Nov 13, 2024

@AdSchellevis I clicked some more and it happened again:

root@OPNsense:~/core # diff -u /conf/backup/config-1731508914.3057.xml /conf/config.xml
--- /conf/backup/config-1731508914.3057.xml	2024-11-13 15:41:54.309977000 +0100
+++ /conf/config.xml	2024-11-13 15:42:02.848899000 +0100
@@ -211,7 +211,7 @@
       <name>admins</name>
       <scope>system</scope>
       <description>System Administrators</description>
-      <priv>page-all,page-dhcp-kea-v4</priv>
+      <priv>page-dhcp-kea-v4</priv>
       <member>0</member>
     </group>
     <user uuid="fa02bcc0-5955-403f-b79c-bb7891cf27f1">
@@ -843,8 +843,8 @@
   </widgets>
   <revision>
     <username>[email protected]</username>
-    <description>/api/auth/priv/set_item/page-diagnostics-arptable made changes</description>
-    <time>1731508914.3057</time>
+    <description>/api/auth/priv/set_item/page-dhcp-kea-v4 made changes</description>
+    <time>1731508922.8457</time>
   </revision>
   <Deciso>
     <Unbound>

I have added the admin group to page-dhcp-kea-v4 and saved. Afterwards I removed the admin group from page-dhcp-kea-v4 and saved.

The above is the diff what happend afterwards.

Adding and removing the admin group from any privilege ID will cause this.

@AdSchellevis
Copy link
Member Author

@Monviech I'll put it on my list to test this as well, sounds like we still have an issue somewhere

@AdSchellevis
Copy link
Member Author

I'll force push the change, stupid me:
image

Copy link
Member

@Monviech Monviech left a comment

Choose a reason for hiding this comment

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

I'm okay with this. Everything I tried seems to do the correct thing now.

Thank you :)

…mma separated member lists.

If we convert groups to a model, we will switch the nested <member> tags into comma separated fields, e.g.

	<member>1</member>
	<member>12</member>

will convert to:

	<member>1,12</member>

using this commit we support both for areas where these are being read.
* add initial boilerplate
* unpack `<priv/>` field on first access
* unpack '<apikeys/>' field on first access and implement key actions into ApiKeyField
* add apikey grid in user management view
* change isset() to !empty() for users disabled flag in backend code
* move user atributes into dialog
* hook PrivField type to \OPNsense\Core\ACL()
* refactor Auth/API to use new User class
* otp seed logic  with simple api call to generate new seeds and some JS glue for the frontend
* uid autonumber field
* language selector using get_locale_list() via configd (cached)
* add StoreB64Field field for authorizedkeys so we can keep the field contents backwards compatible.
* ExpiresField for custom date parsing, supporting previous input formats as well.
* group membership using a volatile custom field type, controller is responisble for persisting the configuration data to avoid entanglement between models
* add button which links to most likely user certs (based on commonname), to avoid all sorts of magic to reflect certs back into the usermanager.
* add getUserPrivs() to model so we can fetch a full list of privs for a user
* show user icons, long this might be less relevant
* add addApiKeyAction() to create a new api key for a user (by name)
* download new api key from user view
* implement hashing when setting a new (or scrabled) password
* use new "auth sync user" event to trigger local user db changes
* in API authenticator keep createKey and dropKey as stubs to the new model implementation
* prevent removal of "system" users (root)
* hook ACL and Menu
* add Group administration using the same logic as users
* cleanup unused
* add System: Access: Privileges to manage and change user and group privileges
review comments from @Monviech

* "Create and Download API Key for this user" refresh apikeys bootgrid
* "Users" bootgrid, add some columns
* rename "Username" to "Group Name" in group edit
* Disable sorting the bootgrid by "Users" and "Groups" as these are aggregated/formatted columns
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Adding new functionality
Development

Successfully merging this pull request may close these issues.

2 participants