auth: Add more tests for entity enrichment with entitlements #14973

gabrielmougard · 2025-02-13T10:01:23Z

This adds tests for the enrichment of the remaining LXD entities that are eligible. This also remove support for entitlement enrichment for instance<snapshot|backup> and volume<snapshot|backup>. These are managed by the parent instance/volume can_mange_<snapshots|backups>

test/suites/auth.sh

lxd/instance_backup.go

…uld not be enriched with entitlements Signed-off-by: Gabriel Mougard <[email protected]>

…p>, volume<snapshot|backup> entities Signed-off-by: Gabriel Mougard <[email protected]>

…lice of pointers Signed-off-by: Gabriel Mougard <[email protected]>

markylaing · 2025-02-20T09:42:47Z

test/suites/auth.sh

+    echo "Second check failed (or pool not found)!" >&2
+    false
+  fi
+


Storage pools aren't project specific. This should work without the project=default key value pair when adding the permissions, and without the project query parameter.

Also, why did you list all storage pools, rather than just querying for the one that permissions have been granted against?

Ok, removing this query param. As for the tests, I need to test both endpoints to see if entitlements are attached in both cases: the storage-pools/{name} one and the storage-pools/

test/suites/auth.sh

markylaing

I like the pattern you've created in the tests by listing all and using jq to make assertions. For me it's quite clear what's going on, but more comments on the test wouldn't hurt.

Additionally, the with-access-entitlements tests are getting quite long. Could you please move them into another bash function and call that?

gabrielmougard · 2025-02-20T10:11:24Z

@markylaing thanks for the review!

…ceOnly.` Signed-off-by: Gabriel Mougard <[email protected]>

Signed-off-by: Gabriel Mougard <[email protected]>

markylaing

Just one more thing - can you please move the with-access-entitlements tests to another bash function and call that? Thanks.

Signed-off-by: Gabriel Mougard <[email protected]>

tomponline · 2025-02-24T16:55:13Z

@markylaing @gabrielmougard is this ready for final review now?

gabrielmougard · 2025-02-24T16:57:40Z

yes (I addressed @markylaing latest comment regarding moving the tests in their own function)

markylaing · 2025-02-24T17:31:42Z

test/suites/auth.sh

+  lxc auth group permission add test-group server admin
+  lxc auth group permission add test-group server viewer
+  lxc auth group permission add test-group server project_manager
+  [ "$(lxc_remote query "oidc:/1.0?with-access-entitlements=admin,viewer,project_manager" | jq -r '.access_entitlements | sort | @csv')" = '"admin","project_manager","viewer"' ]
 }


Missing newline at the end of the file.

markylaing · 2025-02-24T17:34:10Z

test/suites/auth.sh

  lxc auth identity group add oidc/[email protected] test-group
-  # Create a new test project, add some entitlements on it and check that these are reflected in the 'access_entitlements' field returned from the API.
-  lxc project create test-project
-  lxc auth group permission add test-group project test-project can_view
-  lxc auth group permission add test-group project test-project can_edit
-  lxc auth group permission add test-group project test-project can_delete

-  # Check the created project entitlements given a list of candidate entitlements (some are wrong: `can_create_instances` and `can_create_networks`. These should not be returned).
-  [ "$(lxc_remote query "oidc:/1.0/projects/test-project?recursion=1&with-access-entitlements=can_view,can_edit,can_delete,can_create_instances,can_create_networks" | jq -r '.access_entitlements | sort | @csv')" = '"can_delete","can_edit","can_view"' ]
-  lxc project delete test-project
-
-  # Repeat the same test for other entity types.
-  # Instance
-  ensure_import_testimage
-  lxc init testimage test-foo
-  lxc auth group permission add test-group instance test-foo can_view project=default
-  lxc auth group permission add test-group instance test-foo can_edit project=default
-  lxc auth group permission add test-group instance test-foo can_delete project=default
-  [ "$(lxc_remote query "oidc:/1.0/instances/test-foo?project=default&recursion=1&with-access-entitlements=can_view,can_edit,can_delete,can_exec" | jq -r '.access_entitlements | sort | @csv')" = '"can_delete","can_edit","can_view"' ]
-  lxc delete test-foo -f
-
-  # Storage volume
-  # Storage volume entitlements test
-  pool_name="$(lxc storage list -f csv | cut -d, -f1)"
-  lxc storage volume create "${pool_name}" test-volume
-  lxc auth group permission add test-group storage_volume test-volume can_view project=default pool="${pool_name}" type=custom
-  lxc auth group permission add test-group storage_volume test-volume can_edit project=default pool="${pool_name}" type=custom
-  lxc auth group permission add test-group storage_volume test-volume can_delete project=default pool="${pool_name}" type=custom
-  [ "$(lxc_remote query "oidc:/1.0/storage-pools/${pool_name}/volumes/custom/test-volume?project=default&recursion=1&with-access-entitlements=can_view,can_edit,can_delete,can_manage_backups,can_manage_snapshots" | jq -r '.access_entitlements | sort | @csv')" = '"can_delete","can_edit","can_view"' ]
-  lxc storage volume delete "${pool_name}" test-volume
+  entities_enrichment_with_entitlements


You can move this to just below line 220 and then you won't need to re-add the OIDC user to the test group on line 290.

markylaing

Just a couple of nits that can be cleaned up in a follow up.

@gabrielmougard

Permissions for auth groups and certificates were not being deleted properly by the triggers. This caused dangling permissions to be logged when new tests were added by @gabrielmougard in #14973. More details on the trigger changes can be found in the commit messages. I've also added a regex to `deps/panic-checker` to check for this log line and fail any future tests if it is found again. I've run the tests with this addition to the test suite before and after adding the fix to ensure it works and catches the log message. Note that these dangling permissions are *not* a security issue because they reference entities by their ID. If they are dangling, it means they are no longer referencing anything and therefore not granting access. They also can never reference another new entity, since all of our database IDs are set to auto-increment.

gabrielmougard force-pushed the fix/continue-entitlement-tests branch from adf22a4 to 62c1c61 Compare February 13, 2025 13:13

github-actions bot added the Documentation Documentation needs updating label Feb 13, 2025

gabrielmougard force-pushed the fix/continue-entitlement-tests branch 3 times, most recently from 351ce70 to 42954b9 Compare February 13, 2025 15:47

github-advanced-security bot found potential problems Feb 13, 2025

View reviewed changes

gabrielmougard force-pushed the fix/continue-entitlement-tests branch 3 times, most recently from 57bb795 to 25c7335 Compare February 13, 2025 17:08

github-advanced-security bot found potential problems Feb 13, 2025

View reviewed changes

test/suites/auth.sh Fixed Show fixed Hide fixed

test/suites/auth.sh Fixed Show fixed Hide fixed

test/suites/auth.sh Fixed Show fixed Hide fixed

gabrielmougard force-pushed the fix/continue-entitlement-tests branch 2 times, most recently from 8026848 to dfc1e69 Compare February 13, 2025 18:35

github-actions bot removed the Documentation Documentation needs updating label Feb 13, 2025

gabrielmougard force-pushed the fix/continue-entitlement-tests branch from dfc1e69 to ed4ab9e Compare February 13, 2025 18:44

github-advanced-security bot found potential problems Feb 13, 2025

View reviewed changes

test/suites/auth.sh Fixed Show fixed Hide fixed

gabrielmougard force-pushed the fix/continue-entitlement-tests branch 7 times, most recently from 12eaf11 to 077ebe8 Compare February 14, 2025 11:07

github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Feb 14, 2025

gabrielmougard force-pushed the fix/continue-entitlement-tests branch 5 times, most recently from 3a2a3f1 to 0c270f7 Compare February 14, 2025 13:36

github-advanced-security bot found potential problems Feb 14, 2025

View reviewed changes

test/suites/auth.sh Fixed Show fixed Hide fixed

test/suites/auth.sh Fixed Show fixed Hide fixed

tomponline reviewed Feb 18, 2025

View reviewed changes

lxd/instance_backup.go Show resolved Hide resolved

gabrielmougard force-pushed the fix/continue-entitlement-tests branch from f3f4a03 to 812d005 Compare February 18, 2025 11:51

gabrielmougard added 3 commits February 18, 2025 13:19

shared/api: instance<snapshot|backup> and volume<snapshot|backup> sho…

5dcea00

…uld not be enriched with entitlements Signed-off-by: Gabriel Mougard <[email protected]>

lxd: disable enrichment with entitlements for instance<snapshot|backu…

bde6f60

…p>, volume<snapshot|backup> entities Signed-off-by: Gabriel Mougard <[email protected]>

lxd: resultMap for network zones and network acls should be a s…

9ee50a8

…lice of pointers Signed-off-by: Gabriel Mougard <[email protected]>

gabrielmougard force-pushed the fix/continue-entitlement-tests branch from 812d005 to af7d62a Compare February 18, 2025 12:21

markylaing reviewed Feb 20, 2025

View reviewed changes

test/suites/auth.sh Outdated Show resolved Hide resolved

markylaing reviewed Feb 20, 2025

View reviewed changes

test/suites/auth.sh Outdated Show resolved Hide resolved

markylaing reviewed Feb 20, 2025

View reviewed changes

test/suites/auth.sh Outdated Show resolved Hide resolved

markylaing requested changes Feb 20, 2025

View reviewed changes

gabrielmougard force-pushed the fix/continue-entitlement-tests branch from af7d62a to 000f97a Compare February 21, 2025 16:20

gabrielmougard added 2 commits February 21, 2025 17:22

lxd: fix warning `SA1019: req.ContainerOnly is deprecated: Use Instan…

6912759

…ceOnly.` Signed-off-by: Gabriel Mougard <[email protected]>

doc/rest-api: Refresh swagger YAML

5981ad9

Signed-off-by: Gabriel Mougard <[email protected]>

gabrielmougard force-pushed the fix/continue-entitlement-tests branch from 000f97a to 7fa1d7e Compare February 21, 2025 16:22

tomponline requested a review from markylaing February 21, 2025 16:58

markylaing requested changes Feb 24, 2025

View reviewed changes

gabrielmougard force-pushed the fix/continue-entitlement-tests branch from 4f90416 to e851b13 Compare February 24, 2025 08:36

test/auth: More tests for entity enrichment with entitlements

b0a63b9

Signed-off-by: Gabriel Mougard <[email protected]>

gabrielmougard force-pushed the fix/continue-entitlement-tests branch from e851b13 to b0a63b9 Compare February 24, 2025 09:14

gabrielmougard requested a review from markylaing February 24, 2025 15:29

markylaing reviewed Feb 24, 2025

View reviewed changes

markylaing approved these changes Feb 24, 2025

View reviewed changes

tomponline merged commit ead711f into canonical:main Feb 24, 2025
29 checks passed

markylaing mentioned this pull request Feb 26, 2025

Auth: Permission deletion trigger bug #15077

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

auth: Add more tests for entity enrichment with entitlements #14973

auth: Add more tests for entity enrichment with entitlements #14973

gabrielmougard commented Feb 13, 2025 •

edited

Loading

markylaing Feb 20, 2025

gabrielmougard Feb 21, 2025

markylaing left a comment

gabrielmougard commented Feb 20, 2025

markylaing left a comment

tomponline commented Feb 24, 2025

gabrielmougard commented Feb 24, 2025

markylaing Feb 24, 2025

markylaing Feb 24, 2025

markylaing left a comment

auth: Add more tests for entity enrichment with entitlements #14973

auth: Add more tests for entity enrichment with entitlements #14973

Conversation

gabrielmougard commented Feb 13, 2025 • edited Loading

markylaing Feb 20, 2025

Choose a reason for hiding this comment

gabrielmougard Feb 21, 2025

Choose a reason for hiding this comment

markylaing left a comment

Choose a reason for hiding this comment

gabrielmougard commented Feb 20, 2025

markylaing left a comment

Choose a reason for hiding this comment

tomponline commented Feb 24, 2025

gabrielmougard commented Feb 24, 2025

markylaing Feb 24, 2025

Choose a reason for hiding this comment

markylaing Feb 24, 2025

Choose a reason for hiding this comment

markylaing left a comment

Choose a reason for hiding this comment

gabrielmougard commented Feb 13, 2025 •

edited

Loading