AuthZ: Extend /api/search to work with self-contained permissions #70749

mgyongyosi · 2023-06-27T13:30:40Z

What is this feature?
This PR extends the accessControlDashboardPermissionFilter (used by the /api/search endpoint) to be capable of handling users authenticated by the Extended JWT client (in which the SignedInUser's permissions are populated from the entitlement claim of the access token) and these changes make sure that the search result only contains dashboards and folders that the SignedInUser has access to.

Besides, this PR adds a new field to the SignedInUser called AuthenticatedBy which stores the name of the Auth Client that authenticated the current user.

Why do we need this feature?
The dashboard filters should handle the case when the permissions are only set in the SignedInUser's Permissions field and this field is the source of truth for the users' permission which is the case for Extended JWT Client.

Used by the experimental external service communication feature (which is using OAuth2 in the background).

Who is this feature for?

[Add information on what kind of user the feature is for.]

Which issue(s) does this PR fix?:

Fixes #

Special notes for your reviewer:
If the user has the following Permissions:

{ "action": "dashboards:read", "scope": "dashboards:uid:d5485b09-0b3b-47d0-a353-ec6035e1cfdf" },
{ "action": "dashboards:read", "scope": "folders:uid:ce0d1d61-7043-4fb0-9fbc-b5d15963fce0"},
{ "action": "folders:read", "scope": "folders:uid:ba1313d4-d404-48ec-a088-2eea38e4bd29"},

Then the generated SQL will be

nestedFolders = on

WITH RECURSIVE RecQry0 AS (
    SELECT
        uid,
        parent_uid,
        org_id
    FROM
        folder
    WHERE
        uid IN ('ce0d1d61-7043-4fb0-9fbc-b5d15963fce0')
    UNION
    ALL
    SELECT
        f.uid,
        f.parent_uid,
        f.org_id
    FROM
        folder f
        INNER JOIN RecQry0 r ON f.parent_uid = r.uid
        and f.org_id = r.org_id
),
RecQry1 AS (
    SELECT
        uid,
        parent_uid,
        org_id
    FROM
        folder
    WHERE
        uid IN ('ba1313d4-d404-48ec-a088-2eea38e4bd29')
    UNION
    ALL
    SELECT
        f.uid,
        f.parent_uid,
        f.org_id
    FROM
        folder f
        INNER JOIN RecQry1 r ON f.parent_uid = r.uid
        and f.org_id = r.org_id
)
SELECT
    dashboard.id,
    dashboard.uid,
    dashboard.title,
    dashboard.slug,
    dashboard_tag.term,
    dashboard.is_folder,
    dashboard.folder_id,
    folder.uid AS folder_uid,
    folder.slug AS folder_slug,
    folder.title AS folder_title
FROM
    (
        SELECT
            dashboard.id
        FROM
            dashboard
        WHERE
            (
                (
                    dashboard.uid IN ('d5485b09-0b3b-47d0-a353-ec6035e1cfdf')
                    AND NOT dashboard.is_folder
                )
                OR (
                    dashboard.folder_id IN (
                        SELECT
                            d.id
                        FROM
                            dashboard as d
                        WHERE
                            d.uid IN (
                                SELECT
                                    uid
                                FROM
                                    RecQry0
                            )
                    )
                    AND NOT dashboard.is_folder
                )
                OR (
                    dashboard.uid IN (
                        SELECT
                            uid
                        FROM
                            RecQry1
                    )
                    AND dashboard.is_folder
                )
            )
            AND dashboard.org_id = 1
        ORDER BY
            dashboard.title ASC
        LIMIT
            1000 OFFSET 0
    ) AS ids
    INNER JOIN dashboard ON ids.id = dashboard.id
    LEFT OUTER JOIN dashboard AS folder ON folder.id = dashboard.folder_id
    LEFT OUTER JOIN dashboard_tag ON dashboard.id = dashboard_tag.dashboard_id
ORDER BY
    dashboard.title ASC

nestedFolders = off

SELECT
    dashboard.id,
    dashboard.uid,
    dashboard.title,
    dashboard.slug,
    dashboard_tag.term,
    dashboard.is_folder,
    dashboard.folder_id,
    folder.uid AS folder_uid,
    folder.slug AS folder_slug,
    folder.title AS folder_title
FROM
    (
        SELECT
            dashboard.id
        FROM
            dashboard
        WHERE
            (
                (
                    dashboard.uid IN ('d5485b09-0b3b-47d0-a353-ec6035e1cfdf')
                    AND NOT dashboard.is_folder
                )
                OR (
                    dashboard.folder_id IN (
                        SELECT
                            d.id
                        FROM
                            dashboard as d
                        WHERE
                            d.uid IN ('ce0d1d61-7043-4fb0-9fbc-b5d15963fce0')
                    )
                    AND NOT dashboard.is_folder
                )
                OR (
                    dashboard.uid IN ('ba1313d4-d404-48ec-a088-2eea38e4bd29')
                    AND dashboard.is_folder
                )
            )
            AND dashboard.org_id = 1
        ORDER BY
            dashboard.title ASC
        LIMIT
            1000 OFFSET 0
    ) AS ids
    INNER JOIN dashboard ON ids.id = dashboard.id
    LEFT OUTER JOIN dashboard AS folder ON folder.id = dashboard.folder_id
    LEFT OUTER JOIN dashboard_tag ON dashboard.id = dashboard_tag.dashboard_id
ORDER BY
    dashboard.title ASC

Please check that:

It works as expected from a user's perspective.
If this is a pre-GA feature, it is behind a feature toggle.
The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

…s-empty-role

* clean up, changes to the search

* Refactor and simplify code

…s-empty-role

IevaVasiljeva

Some tiny comments about tests, but I also have questions about the query logic. Let's catch up at some point to talk it through.

pkg/services/authn/clients/grafana_test.go

pkg/services/login/authinfo.go

pkg/services/sqlstore/permissions/dashboard_test.go

papagian · 2023-07-06T08:52:38Z

hi @mgyongyosi
I'm working on measuring and refactoring the search query for mitigating performance issues.
from browsing the changes, I get that the only change in the accessControlDashboardPermissionFilter is the additional check for useSelfContainedPermissions and the additional implementation when useSelfContainedPermissions=true.
is that correct?

mgyongyosi · 2023-07-06T09:21:22Z

hi @mgyongyosi I'm working on measuring and refactoring the search query for mitigating performance issues. from browsing the changes, I get that the only change in the accessControlDashboardPermissionFilter is the additional check for useSelfContainedPermissions and the additional implementation when useSelfContainedPermissions=true. is that correct?

@papagian Correct, but based on @kalleep's feedback, I'm gonna change the useSelfContainedPermissions=true part a little bit to use the accesscontrol.Filter method instead of having my own implementation (I still need to test it whether it works with the Filter method, I'll let you know), but I won't change the part when useSelfContainedPermissions=false!

IevaVasiljeva · 2023-07-11T09:06:00Z

pkg/services/sqlstore/permissions/dashboard.go

+	return result
+}
+
+func parseArguments(actions []string, user *user.SignedInUser, scopePrefix string) []interface{} {


I think this logic would list all the dashboards that user has any of the required permissions on, while the old logic was listing dashboards that user has all of the required permissions on. Or am I misunderstanding something?

Thanks! I investigated this and you are right! I pushed a fix + added tests for this scenario!

…s-empty-role

gamab

LGTM 👍
I left a couple of comments/suggestions, the cyclomatic complexity of the buildClause function is what worries me most. I'd consider splitting it 🤔

pkg/services/authn/authn.go

pkg/services/authn/clients/grafana.go

pkg/services/user/model.go

pkg/services/sqlstore/permissions/dashboard.go

pkg/services/sqlstore/permissions/dashboard_test.go

gamab

LGTM 👍
I left a couple of comments/suggestions, the cyclomatic complexity of the buildClause function is what worries me most. I'd consider splitting it 🤔

Co-authored-by: Gabriel MABILLE <[email protected]>

IevaVasiljeva

Nice, looks good to me now! The permission filter clause function is pretty mahoosive now, so I think it would be good to try to refactor it. But let's leave that for a separate PR.

Jguer · 2023-07-12T09:27:37Z

pkg/services/authn/authnimpl/sync/user_sync.go

@@ -184,7 +184,7 @@ func (s *UserSync) upsertAuthConnection(ctx context.Context, userID int64, ident
 	if createConnection {


Hey @mgyongyosi will this change start to create user_auth entries for basic auth and api key auth? This might be problematic but not an active issue.

Most important would be for sessions to not set auth modules as they should take the 'last authenticated' method

Nope, because the SyncHook is only executed when the ClientParams.SyncUser is set to true and it's set to true for only external auth.

works for me, thanks for the confirmation

Jguer

https://github.com/grafana/grafana/pull/70749/files#r1260889415

IevaVasiljeva · 2023-07-12T09:29:12Z

Haha, I realised that my comment pretty much repeated what @gamab said 🙈 But it looks like we all agree that this could use improvement. I've made an issue for it, so that we don't forget.

…0749) * Search sql filter draft, unfinished * Search works for empty roles * Add current AuthModule to SignedInUser * clean up, changes to the search * Use constant prefixes * Change AuthModule to AuthenticatedBy * Add tests for using the permissions from the SignedInUser * Refactor and simplify code * Fix sql generation for pg and mysql * Fixes, clean up * Add test for empty permission list * Fix * Fix any vs all in case of edit permission * Update pkg/services/authn/authn.go Co-authored-by: Gabriel MABILLE <[email protected]> * Update pkg/services/sqlstore/permissions/dashboard_test.go Co-authored-by: Gabriel MABILLE <[email protected]> * Fixes, changes based on the review --------- Co-authored-by: Gabriel MABILLE <[email protected]>

mgyongyosi added 5 commits June 22, 2023 18:17

Search sql filter draft, unfinished

4823c39

Search works for empty roles

05d5f35

Merge remote-tracking branch 'origin/main' into mgyongyosi/sql-filter…

718ab87

…s-empty-role

Add current AuthModule to SignedInUser

0d5f520

* clean up, changes to the search

Use constant prefixes

79a0c87

mgyongyosi added this to the 10.1.x milestone Jun 27, 2023

grafanabot added the area/backend label Jun 27, 2023

mgyongyosi changed the title ~~AuthZ: Extend /api/search to work with self-contained permissions~~ WIP AuthZ: Extend /api/search to work with self-contained permissions Jun 27, 2023

mgyongyosi added the area/auth/rbac Grafana role-based access control label Jun 27, 2023

mgyongyosi added 2 commits June 29, 2023 10:33

Change AuthModule to AuthenticatedBy

33fe1df

Add tests for using the permissions from the SignedInUser

dd1c27b

* Refactor and simplify code

mgyongyosi requested review from Jguer and IevaVasiljeva June 29, 2023 15:04

mgyongyosi marked this pull request as ready for review July 4, 2023 07:50

mgyongyosi requested review from a team as code owners July 4, 2023 07:50

mgyongyosi requested review from mildwonkey, suntala, yangkb09 and kalleep and removed request for a team July 4, 2023 07:50

mgyongyosi added add to changelog no-backport Skip backport of PR labels Jul 4, 2023

Merge remote-tracking branch 'origin/main' into mgyongyosi/sql-filter…

041130b

…s-empty-role

mgyongyosi changed the title ~~WIP AuthZ: Extend /api/search to work with self-contained permissions~~ AuthZ: Extend /api/search to work with self-contained permissions Jul 4, 2023

Fix sql generation for pg and mysql

c70f128

mgyongyosi requested a review from gamab July 5, 2023 09:09

IevaVasiljeva reviewed Jul 5, 2023

View reviewed changes

pkg/services/authn/clients/grafana_test.go Outdated Show resolved Hide resolved

pkg/services/login/authinfo.go Show resolved Hide resolved

pkg/services/sqlstore/permissions/dashboard_test.go Show resolved Hide resolved

Fixes, clean up

0ba0f85

mgyongyosi force-pushed the mgyongyosi/sql-filters-empty-role branch from b97ab89 to 0ba0f85 Compare July 6, 2023 15:23

mgyongyosi added 2 commits July 10, 2023 13:28

Add test for empty permission list

891d8b0

Fix

b71b447

mgyongyosi requested review from papagian and IevaVasiljeva July 10, 2023 11:59

IevaVasiljeva reviewed Jul 11, 2023

View reviewed changes

mgyongyosi added 3 commits July 11, 2023 11:26

Merge remote-tracking branch 'origin/main' into mgyongyosi/sql-filter…

f7699cb

…s-empty-role

Fix any vs all in case of edit permission

d9b7446

Merge remote-tracking branch 'origin/main' into mgyongyosi/sql-filter…

0e23175

…s-empty-role

gamab approved these changes Jul 11, 2023

View reviewed changes

mgyongyosi and others added 3 commits July 11, 2023 14:43

Update pkg/services/authn/authn.go

58217e1

Co-authored-by: Gabriel MABILLE <[email protected]>

Update pkg/services/sqlstore/permissions/dashboard_test.go

eca220a

Co-authored-by: Gabriel MABILLE <[email protected]>

Fixes, changes based on the review

3652ecc

mgyongyosi requested a review from IevaVasiljeva July 11, 2023 13:47

IevaVasiljeva approved these changes Jul 12, 2023

View reviewed changes

Jguer reviewed Jul 12, 2023

View reviewed changes

Jguer approved these changes Jul 12, 2023

View reviewed changes

mgyongyosi merged commit 5efc338 into main Jul 12, 2023

mgyongyosi deleted the mgyongyosi/sql-filters-empty-role branch July 12, 2023 10:31

papagian mentioned this pull request Jul 31, 2023

Nested folders: Fix search query for empty self-contained permissions #72620

Closed

3 tasks

ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023

This was referenced Aug 2, 2023

Nested folders: Fix search query for empty self-contained permissions #72727

Merged

[v10.1.x] Nested folders: Fix search query for empty self-contained permissions #72733

Merged

chenrui333 mentioned this pull request Aug 24, 2023

grafana 10.1.0 Homebrew/homebrew-core#140321

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AuthZ: Extend /api/search to work with self-contained permissions #70749

AuthZ: Extend /api/search to work with self-contained permissions #70749

mgyongyosi commented Jun 27, 2023 •

edited

Loading

IevaVasiljeva left a comment

papagian commented Jul 6, 2023

mgyongyosi commented Jul 6, 2023 •

edited

Loading

IevaVasiljeva Jul 11, 2023

mgyongyosi Jul 11, 2023

gamab left a comment

gamab left a comment

IevaVasiljeva left a comment

Jguer Jul 12, 2023

mgyongyosi Jul 12, 2023

Jguer Jul 12, 2023

Jguer left a comment

IevaVasiljeva commented Jul 12, 2023

		@@ -184,7 +184,7 @@ func (s *UserSync) upsertAuthConnection(ctx context.Context, userID int64, ident
		if createConnection {

AuthZ: Extend /api/search to work with self-contained permissions #70749

AuthZ: Extend /api/search to work with self-contained permissions #70749

Conversation

mgyongyosi commented Jun 27, 2023 • edited Loading

IevaVasiljeva left a comment

Choose a reason for hiding this comment

papagian commented Jul 6, 2023

mgyongyosi commented Jul 6, 2023 • edited Loading

IevaVasiljeva Jul 11, 2023

Choose a reason for hiding this comment

mgyongyosi Jul 11, 2023

Choose a reason for hiding this comment

gamab left a comment

Choose a reason for hiding this comment

gamab left a comment

Choose a reason for hiding this comment

IevaVasiljeva left a comment

Choose a reason for hiding this comment

Jguer Jul 12, 2023

Choose a reason for hiding this comment

mgyongyosi Jul 12, 2023

Choose a reason for hiding this comment

Jguer Jul 12, 2023

Choose a reason for hiding this comment

Jguer left a comment

Choose a reason for hiding this comment

IevaVasiljeva commented Jul 12, 2023

mgyongyosi commented Jun 27, 2023 •

edited

Loading

mgyongyosi commented Jul 6, 2023 •

edited

Loading