8224 report required permissions when authorization fails #8314

ItamarYuran · 2024-10-28T12:36:19Z

Closes #(8224)

Change Description

The added code helps the user understand why they are not authorized to perform a certain action.

Background

Up until now in case a user had no permissions or was denied of some action, they would only get a 401 unauthorized message, making it hard to understand what is missing in order to perform that action.

The enhancement will report all denied actions in case there are any, or if there aren't any - report missing permissions.

Testing Details

Changes were not tested yet, no existing test got broken

Additional info

github-actions · 2024-10-28T12:43:35Z

E2E Test Results - Quickstart

github-actions · 2024-10-28T12:43:44Z

E2E Test Results - DynamoDB Local - Local Block Adapter

arielshaqed

Nice! Can you please show what outputs we can expect? Or is that in the next commit with tests etc.?

arielshaqed · 2024-10-29T14:52:11Z

pkg/auth/service.go

+		deniedStr := "denied from:\n"
+		for _, statement := range n.Denied {
+			deniedStr += strings.Join(statement.Action, "\n")
+		}
+		return deniedStr


Not sure how this code can successfully format deniedStr if len(n.Denied) > 0. There will be no newline after the last row of the Action list on each statement.

It will probably be easier to use strings.Builder. This is actually an io.Writer, so you can just write to it using the normal operations. Here's a playground.

arielshaqed · 2024-10-29T14:54:53Z

pkg/auth/service.go

 	allowed := CheckNeutral
+	hasPermission := false


This var is used only in case permissions.NoteTypeNode. So please keep it in scope there: you could just put that entire case inside curly brackets, and that var in there. Or extract it to another func, or even go full OO and make each of these a method. Right now the code confuses me about what might happen with this variable in other cases.

arielshaqed · 2024-10-29T14:57:19Z

pkg/auth/service.go

@@ -1165,23 +1189,28 @@ func CheckPermissions(ctx context.Context, node permissions.Node, username strin

 					if stmt.Effect == model.StatementEffectDeny {
 						// this is a "Deny" and it takes precedence
-						return CheckDeny
+						perm.Denied = append(perm.Denied, stmt)


This is confusing: I take perm as an input, now I modify it and return it. So I've changed my input, but also returned it as an output.

I think either make perm into an object, pass it by pointer, and keep state on it, or explicitly handle the lists.

Was thinking the same, agree with @arielshaqed 100%

ItamarYuran · 2024-11-03T14:36:27Z

The current code returns:

All denied actions in a policy in case there are any
First missing permission in case there is no relevant denied action

arielshaqed

Thanks!

Final stretch: the actual message. The current message is too detailed and as a result needs too many lines. Let's start by playing safely and never report anything other than action names. Reporting an entire statement gives a wealth of information that might not visible to the user - such as permissions of other users.

arielshaqed · 2024-11-04T08:17:24Z

esti/auth_test.go

@@ -234,7 +235,7 @@ func TestCreateRepo_Unauthorized(t *testing.T) {
 	})
 	require.NoError(t, err)
 	require.NotNil(t, resp.JSON401)
-	if resp.JSON401.Message != auth.ErrInsufficientPermissions.Error() {
+	if !strings.Contains(resp.JSON401.Message, auth.ErrInsufficientPermissions.Error()) {


Can we test here (and below) that we got an expected value? Obviously doing so makes sense only after we finalize the message contents. So it is fine to open an issue to improve these checks in this test file.

arielshaqed · 2024-11-04T08:21:18Z

pkg/api/controller_test.go

+				},
+			},
+			username: "user1",
+			expected: "lacking permissions for:\nfs:CreateRepository",


I would expect it to report both missing permissions. Reporting only one is fine, but then the message is surprising - it says "permissions" when it only ever means a single permissions.

arielshaqed · 2024-11-04T08:22:20Z

pkg/auth/service.go


 	if allowed != CheckAllow {
 		return &AuthorizationResponse{
 			Allowed: false,
-			Error:   ErrInsufficientPermissions,
+			Error:   fmt.Errorf("%w\n%s", ErrInsufficientPermissions, perm.String()),


Looking at the results in the tests, I prefer without the newline, error messages are rarely >1 line.

Suggested change

Error: fmt.Errorf("%w\n%s", ErrInsufficientPermissions, perm.String()),

Error: fmt.Errorf("%w: %s", ErrInsufficientPermissions, perm.String()),

arielshaqed · 2024-11-04T08:23:21Z

pkg/auth/service.go

+				fmt.Fprintln(w, action)
+			}
+		}
+		return strings.TrimSpace(w.String())


It is quite rare to need to TrimSpace on a string you created.

arielshaqed · 2024-11-04T08:25:12Z

pkg/auth/service.go

+		fmt.Fprintln(w, "denied from:")
+		for _, statement := range n.Denied {
+			for _, action := range statement.Action {
+				fmt.Fprintln(w, action)
+			}
+		}
+		return strings.TrimSpace(w.String())


I think you don't even need a strings.Builder here:

Suggested change

fmt.Fprintln(w, "denied from:")

for _, statement := range n.Denied {

for _, action := range statement.Action {

fmt.Fprintln(w, action)

}

}

return strings.TrimSpace(w.String())

return fmt.Sprintf("denied permission to %s", strings.Join(n.Denied, ", "))

The same of course in the next section.

arielshaqed · 2024-11-04T08:26:21Z

pkg/auth/service.go

+		}
+		return strings.TrimSpace(w.String())
+	}
+	return strings.TrimSpace(w.String())


This returns "". If that is not helpful, please return some constant string like "not allowed".

arielshaqed · 2024-11-04T08:29:33Z

pkg/auth/service.go

+	Denied       []model.Statement
+	Unauthorized []permissions.Node


These are a bit too informative IMO. Let's just keep the action name here? Otherwise I learn details of the policy including denied paths that I might not have known existed!

Suggested change

Denied []model.Statement

Unauthorized []permissions.Node

// Denied is a list of actions the user was denied for the attempt.

Denied []string

// Unauthorized is a list of actions the user did not have for the attempt.

Unauthorized []string

Isan-Rivkin · 2024-11-04T12:01:59Z

pkg/auth/service.go

+	return strings.TrimSpace(w.String())
+}
+
+func CheckPermissions(ctx context.Context, node permissions.Node, username string, policies []*model.Policy, perm *NeededPermissions) CheckResult {


nit: variable name perm is too generic.
Can you use something more explicit?
for example: audit, denyAudit, permAudit, history etc...
So that in the future when extending, debugging or just reading the code we can immediately understand what it is used for.

Isan-Rivkin · 2024-11-04T12:03:31Z

pkg/auth/service.go

@@ -1165,23 +1189,28 @@ func CheckPermissions(ctx context.Context, node permissions.Node, username strin

 					if stmt.Effect == model.StatementEffectDeny {
 						// this is a "Deny" and it takes precedence
-						return CheckDeny
+						perm.Denied = append(perm.Denied, stmt)


Was thinking the same, agree with @arielshaqed 100%

Isan-Rivkin · 2024-11-04T12:04:57Z

pkg/auth/service.go

@@ -44,6 +44,11 @@ type AuthorizationResponse struct {
 	Error   error
 }

+type NeededPermissions struct {


rename, MissingPermissions or PermissionsAudit or DenyHistory w/e would be more appropriate?

Isan-Rivkin · 2024-11-04T12:06:02Z

contrib/auth/acl/service.go

@@ -895,8 +895,8 @@ func (s *AuthService) Authorize(ctx context.Context, req *auth.AuthorizationRequ
 	if err != nil {
 		return nil, err
 	}
-
-	allowed := auth.CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies)
+	perm := &auth.NeededPermissions{}


sorry, Im confused what are you doing with this variable?

ItamarYuran · 2024-11-05T09:34:46Z

Thank you for your reviews!
These changes will return all denied permissions, only the ones that regard the request (not all permission in the policy)
Same goes for missing permissions, all missing one will return. In case of denied + missing permissions only denied ones return.

arielshaqed

Very elegant, thanks! I think this will be a great improvement in quality of life for lakeFS admins who need to configure permissions.

Just a note about phrasing one of the error messages; please decide if it will improve things and pull either way.

arielshaqed · 2024-11-05T12:16:25Z

pkg/api/controller_test.go

+		}, /////////////////////////////////////////////////////////////////
+		{


Please avoid adding filler comments. They are pleasant for different people at different widths, they are never consistent, and they end up not appearing everywhere.

Suggested change

}, /////////////////////////////////////////////////////////////////

{

}, {

arielshaqed · 2024-11-10T09:28:36Z

pkg/auth/service.go

+		return fmt.Sprintf("denied permission to %s", strings.Join(n.Denied, ","))
+	}
+	if len(n.Unauthorized) != 0 {
+		return fmt.Sprintf("missing permission to %s", strings.Join(n.Unauthorized, ","))


l.935 will cause a message that says "insufficient permissions: missing permission to ...". That's a bit wordy, so perhaps

Suggested change

return fmt.Sprintf("missing permission to %s", strings.Join(n.Unauthorized, ","))

return fmt.Sprintf("not allowed to %s", strings.Join(n.Unauthorized, ","))

An added benefit is that it uses the same word "allow" from the policy language.

arielshaqed · 2024-11-10T09:30:21Z

pkg/auth/service.go

+					} else {
+						hasPermission = true
+						allowed = CheckAllow


Suggested change

} else {

hasPermission = true

allowed = CheckAllow

hasPermission = true

allowed = CheckAllow

is closer to the previous version.

We prefer to use an "early return" style: errors return immediately - like l. 1188 does; successes keep going with no "else" and no need to indent.

Isan-Rivkin

single comment regarding ACL authorize check. its not used there.
Don't want to block PTAL

Isan-Rivkin · 2024-11-12T14:19:25Z

contrib/auth/acl/service.go

@@ -895,8 +895,8 @@ func (s *AuthService) Authorize(ctx context.Context, req *auth.AuthorizationRequ
 	if err != nil {
 		return nil, err
 	}
-
-	allowed := auth.CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies)
+	perm := &auth.MissingPermissions{}


Going back to prev question - what are you doing with perm after auth.CheckPermissions ? I don't see it being used.

ItamarYuran added 3 commits October 28, 2024 13:35

deny works

f1c6393

reporting missing permissions

88175ae

works

2242218

ItamarYuran linked an issue Oct 28, 2024 that may be closed by this pull request

Report required permission(s) when authorization fails #8224

Closed

ItamarYuran added the exclude-changelog PR description should not be included in next release changelog label Oct 28, 2024

ItamarYuran requested review from Isan-Rivkin and arielshaqed October 28, 2024 12:36

ItamarYuran marked this pull request as draft October 28, 2024 12:53

ItamarYuran added 5 commits October 28, 2024 17:28

fixed errors

b1f9e3a

fixed esti

11aa10a

enhanced, very enhanced

ee2986a

crossing fingaz

d04f675

using string.contains

e883b3c

ItamarYuran marked this pull request as ready for review October 29, 2024 12:11

arielshaqed reviewed Oct 29, 2024

View reviewed changes

ItamarYuran added 3 commits October 31, 2024 14:07

with changes no tests

5da4f66

service

54dc6aa

yesh tests

743f818

ItamarYuran requested a review from arielshaqed November 3, 2024 15:22

arielshaqed reviewed Nov 4, 2024

View reviewed changes

basic changes

4a054c6

Isan-Rivkin requested changes Nov 4, 2024

View reviewed changes

minor changes

803d4a3

ItamarYuran force-pushed the 8224-report-required-permissions-when-authorization-fails branch from 539918d to 704af7d Compare November 4, 2024 17:08

ItamarYuran requested review from arielshaqed and Isan-Rivkin November 5, 2024 09:34

simpler, returning first missing

758d910

ItamarYuran force-pushed the 8224-report-required-permissions-when-authorization-fails branch from 972dc58 to 758d910 Compare November 5, 2024 14:02

simpler, no mock

7b304cc

arielshaqed approved these changes Nov 10, 2024

View reviewed changes

ItamarYuran added 2 commits November 10, 2024 11:47

final tunings

7992c4b

no empty lines

060083f

Isan-Rivkin approved these changes Nov 12, 2024

View reviewed changes

now using parm

2c50f4a

ItamarYuran merged commit 3d0db30 into master Nov 12, 2024
38 checks passed

ItamarYuran deleted the 8224-report-required-permissions-when-authorization-fails branch November 12, 2024 16:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8224 report required permissions when authorization fails #8314

8224 report required permissions when authorization fails #8314

ItamarYuran commented Oct 28, 2024 •

edited by arielshaqed

Loading

github-actions bot commented Oct 28, 2024 •

edited

Loading

github-actions bot commented Oct 28, 2024

arielshaqed left a comment

arielshaqed Oct 29, 2024

arielshaqed Oct 29, 2024

arielshaqed Oct 29, 2024

Isan-Rivkin Nov 4, 2024

ItamarYuran commented Nov 3, 2024

arielshaqed left a comment

arielshaqed Nov 4, 2024

arielshaqed Nov 4, 2024

arielshaqed Nov 4, 2024

arielshaqed Nov 4, 2024

arielshaqed Nov 4, 2024

arielshaqed Nov 4, 2024

arielshaqed Nov 4, 2024

Isan-Rivkin Nov 4, 2024

Isan-Rivkin Nov 4, 2024

Isan-Rivkin Nov 4, 2024

Isan-Rivkin Nov 4, 2024

ItamarYuran commented Nov 5, 2024

arielshaqed left a comment

arielshaqed Nov 5, 2024

arielshaqed Nov 10, 2024

arielshaqed Nov 10, 2024

Isan-Rivkin left a comment

Isan-Rivkin Nov 12, 2024

	Error: fmt.Errorf("%w\n%s", ErrInsufficientPermissions, perm.String()),
	Error: fmt.Errorf("%w: %s", ErrInsufficientPermissions, perm.String()),

-	Denied       []model.Statement
-	Unauthorized []permissions.Node
+	// Denied is a list of actions the user was denied for the attempt.
+	Denied       []string
+	// Unauthorized is a list of actions the user did not have for the attempt.
+	Unauthorized []string

		}, /////////////////////////////////////////////////////////////////
		{

	}, /////////////////////////////////////////////////////////////////
	{
	}, {

	return fmt.Sprintf("missing permission to %s", strings.Join(n.Unauthorized, ","))
	return fmt.Sprintf("not allowed to %s", strings.Join(n.Unauthorized, ","))

8224 report required permissions when authorization fails #8314

8224 report required permissions when authorization fails #8314

Conversation

ItamarYuran commented Oct 28, 2024 • edited by arielshaqed Loading

Change Description

Background

Testing Details

Additional info

github-actions bot commented Oct 28, 2024 • edited Loading

E2E Test Results - Quickstart

github-actions bot commented Oct 28, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ItamarYuran commented Nov 3, 2024

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ItamarYuran commented Nov 5, 2024

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Isan-Rivkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ItamarYuran commented Oct 28, 2024 •

edited by arielshaqed

Loading

github-actions bot commented Oct 28, 2024 •

edited

Loading