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.

Sign up for GitHub

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

8224 report required permissions when authorization fails #8314

Merged
merged 18 commits into from
Nov 12, 2024
Merged

8224 report required permissions when authorization fails #8314

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
f1c6393
deny works
ItamarYuran Oct 28, 2024
88175ae
reporting missing permissions
ItamarYuran Oct 28, 2024
2242218
works
ItamarYuran Oct 28, 2024
b1f9e3a
fixed errors
ItamarYuran Oct 28, 2024
11aa10a
fixed esti
ItamarYuran Oct 28, 2024
ee2986a
enhanced, very enhanced
ItamarYuran Oct 29, 2024
d04f675
crossing fingaz
ItamarYuran Oct 29, 2024
e883b3c
using string.contains
ItamarYuran Oct 29, 2024
5da4f66
with changes no tests
ItamarYuran Oct 31, 2024
54dc6aa
service
ItamarYuran Nov 3, 2024
743f818
yesh tests
ItamarYuran Nov 3, 2024
4a054c6
basic changes
ItamarYuran Nov 4, 2024
803d4a3
minor changes
ItamarYuran Nov 4, 2024
758d910
simpler, returning first missing
ItamarYuran Nov 5, 2024
7b304cc
simpler, no mock
ItamarYuran Nov 5, 2024
7992c4b
final tunings
ItamarYuran Nov 10, 2024
060083f
no empty lines
ItamarYuran Nov 10, 2024
2c50f4a
now using parm
ItamarYuran Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions contrib/auth/acl/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

allowed, _ := auth.CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies, perm)

if allowed != auth.CheckAllow {
return &auth.AuthorizationResponse{
Expand Down
9 changes: 5 additions & 4 deletions esti/auth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"net/http"
"slices"
"strings"
"testing"
"time"

Expand Down Expand Up @@ -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()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

t.Fatalf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message)
}
}
Expand All @@ -255,15 +256,15 @@ func TestRepoMetadata_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()) {
t.Errorf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message)
}
})
t.Run("delete", func(t *testing.T) {
resp, err := clt.DeleteRepositoryMetadataWithResponse(ctx, repo, apigen.DeleteRepositoryMetadataJSONRequestBody{Keys: []string{"foo"}})
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()) {
t.Errorf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message)
}
})
Expand All @@ -272,7 +273,7 @@ func TestRepoMetadata_Unauthorized(t *testing.T) {
resp, err := clt.GetRepositoryMetadataWithResponse(ctx, repo)
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()) {
t.Errorf("expected error message %q, got %q", auth.ErrInsufficientPermissions.Error(), resp.JSON401.Message)
}
})
Expand Down
57 changes: 43 additions & 14 deletions pkg/auth/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ type AuthorizationResponse struct {
Error error
}

type NeededPermissions struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Denied []model.Statement
Unauthorized []permissions.Node
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

// CheckResult - the final result for the authorization is accepted only if it's CheckAllow
type CheckResult int

Expand Down Expand Up @@ -918,13 +923,13 @@ func (a *APIAuthService) Authorize(ctx context.Context, req *AuthorizationReques
if err != nil {
return nil, err
}

allowed := CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies)
perm := &NeededPermissions{}
allowed, permissions := CheckPermissions(ctx, req.RequiredPermissions, req.Username, policies, perm)

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

Expand Down Expand Up @@ -1147,8 +1152,27 @@ func NewAPIAuthServiceWithClient(client ClientWithResponsesInterface, externalPr
}, nil
}

func CheckPermissions(ctx context.Context, node permissions.Node, username string, policies []*model.Policy) CheckResult {
func (n *NeededPermissions) String() string {
if len(n.Denied) != 0 {
deniedStr := "denied from:\n"
for _, statement := range n.Denied {
deniedStr += strings.Join(statement.Action, "\n")
}
return deniedStr
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. 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.
  2. 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.

}
if len(n.Unauthorized) != 0 {
permStr := "lacking permissions for:\n"
for _, node := range n.Unauthorized {
permStr += node.Permission.Action
}
return permStr
}
return ""
}

func CheckPermissions(ctx context.Context, node permissions.Node, username string, policies []*model.Policy, perm *NeededPermissions) (CheckResult, *NeededPermissions) {
allowed := CheckNeutral
hasPermission := false
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

switch node.Type {
case permissions.NodeTypeNode:
// check whether the permission is allowed, denied or natural (not allowed and not denied)
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Was thinking the same, agree with @arielshaqed 100%

return CheckDeny, perm
} else {
hasPermission = true
allowed = CheckAllow
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

allowed = CheckAllow
}
}
}
if !hasPermission {
perm.Unauthorized = append(perm.Unauthorized, node)
}

case permissions.NodeTypeOr:
// returns:
// Allowed - at least one of the permissions is allowed and no one is denied
// Denied - one of the permissions is Deny
// Natural - otherwise
for _, node := range node.Nodes {
result := CheckPermissions(ctx, node, username, policies)
result, perm := CheckPermissions(ctx, node, username, policies, perm)
if result == CheckDeny {
return CheckDeny
return CheckDeny, perm
}
if allowed != CheckAllow {
allowed = result
Expand All @@ -1194,18 +1223,18 @@ func CheckPermissions(ctx context.Context, node permissions.Node, username strin
// Denied - one of the permissions is Deny
// Natural - otherwise
for _, node := range node.Nodes {
result := CheckPermissions(ctx, node, username, policies)
result, perm := CheckPermissions(ctx, node, username, policies, perm)
if result == CheckNeutral || result == CheckDeny {
return result
return result, perm
}
}
return CheckAllow
return CheckAllow, perm

default:
logging.FromContext(ctx).Error("unknown permission node type")
return CheckDeny
return CheckDeny, perm
}
return allowed
return allowed, perm
}

func interpolateUser(resource string, username string) string {
Expand Down
Loading