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

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

Merged
merged 18 commits into from
Jul 12, 2023
Merged

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
4823c39
Search sql filter draft, unfinished
mgyongyosi Jun 22, 2023
05d5f35
Search works for empty roles
mgyongyosi Jun 26, 2023
718ab87
Merge remote-tracking branch 'origin/main' into mgyongyosi/sql-filter…
mgyongyosi Jun 27, 2023
0d5f520
Add current AuthModule to SignedInUser
mgyongyosi Jun 27, 2023
79a0c87
Use constant prefixes
mgyongyosi Jun 27, 2023
33fe1df
Change AuthModule to AuthenticatedBy
mgyongyosi Jun 29, 2023
dd1c27b
Add tests for using the permissions from the SignedInUser
mgyongyosi Jun 29, 2023
041130b
Merge remote-tracking branch 'origin/main' into mgyongyosi/sql-filter…
mgyongyosi Jul 4, 2023
c70f128
Fix sql generation for pg and mysql
mgyongyosi Jul 4, 2023
0ba0f85
Fixes, clean up
mgyongyosi Jul 6, 2023
891d8b0
Add test for empty permission list
mgyongyosi Jul 10, 2023
b71b447
Fix
mgyongyosi Jul 10, 2023
f7699cb
Merge remote-tracking branch 'origin/main' into mgyongyosi/sql-filter…
mgyongyosi Jul 11, 2023
d9b7446
Fix any vs all in case of edit permission
mgyongyosi Jul 11, 2023
0e23175
Merge remote-tracking branch 'origin/main' into mgyongyosi/sql-filter…
mgyongyosi Jul 11, 2023
58217e1
Update pkg/services/authn/authn.go
mgyongyosi Jul 11, 2023
eca220a
Update pkg/services/sqlstore/permissions/dashboard_test.go
mgyongyosi Jul 11, 2023
3652ecc
Fixes, changes based on the review
mgyongyosi Jul 11, 2023
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
42 changes: 22 additions & 20 deletions pkg/services/authn/authn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,9 +200,9 @@ type Identity struct {
Email string
// IsGrafanaAdmin is true if the entity is a Grafana admin.
IsGrafanaAdmin *bool
// AuthModule is the name of the external system. For example, "auth_ldap" or "auth_saml".
// Empty if the identity is provided by Grafana.
AuthModule string
// AuthenticatedBy is the name of authentication client that used to authenticate the current Identity.
mgyongyosi marked this conversation as resolved.
Show resolved Hide resolved
// For example, "password", "apikey", "auth_ldap" or "auth_azuread".
AuthenticatedBy string
// AuthId is the unique identifier for the entity in the external system.
// Empty if the identity is provided by Grafana.
AuthID string
Expand Down Expand Up @@ -269,6 +269,7 @@ func (i *Identity) SignedInUser() *user.SignedInUser {
Login: i.Login,
Name: i.Name,
Email: i.Email,
AuthModule: i.AuthenticatedBy,
OrgCount: i.OrgCount,
IsGrafanaAdmin: isGrafanaAdmin,
IsAnonymous: i.IsAnonymous,
Expand All @@ -294,7 +295,7 @@ func (i *Identity) ExternalUserInfo() login.ExternalUserInfo {
_, id := i.NamespacedID()
return login.ExternalUserInfo{
OAuthToken: i.OAuthToken,
AuthModule: i.AuthModule,
AuthModule: i.AuthenticatedBy,
AuthId: i.AuthID,
UserId: id,
Email: i.Email,
Expand All @@ -308,23 +309,24 @@ func (i *Identity) ExternalUserInfo() login.ExternalUserInfo {
}

// IdentityFromSignedInUser creates an identity from a SignedInUser.
func IdentityFromSignedInUser(id string, usr *user.SignedInUser, params ClientParams) *Identity {
func IdentityFromSignedInUser(id string, usr *user.SignedInUser, params ClientParams, authenticatedBy string) *Identity {
return &Identity{
ID: id,
OrgID: usr.OrgID,
OrgName: usr.OrgName,
OrgRoles: map[int64]org.RoleType{usr.OrgID: usr.OrgRole},
Login: usr.Login,
Name: usr.Name,
Email: usr.Email,
OrgCount: usr.OrgCount,
IsGrafanaAdmin: &usr.IsGrafanaAdmin,
IsDisabled: usr.IsDisabled,
HelpFlags1: usr.HelpFlags1,
LastSeenAt: usr.LastSeenAt,
Teams: usr.Teams,
ClientParams: params,
Permissions: usr.Permissions,
ID: id,
OrgID: usr.OrgID,
OrgName: usr.OrgName,
OrgRoles: map[int64]org.RoleType{usr.OrgID: usr.OrgRole},
Login: usr.Login,
Name: usr.Name,
Email: usr.Email,
AuthenticatedBy: authenticatedBy,
OrgCount: usr.OrgCount,
IsGrafanaAdmin: &usr.IsGrafanaAdmin,
IsDisabled: usr.IsDisabled,
HelpFlags1: usr.HelpFlags1,
LastSeenAt: usr.LastSeenAt,
Teams: usr.Teams,
ClientParams: params,
Permissions: usr.Permissions,
}
}

Expand Down
26 changes: 13 additions & 13 deletions pkg/services/authn/authnimpl/sync/user_sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,27 +76,27 @@ func (s *UserSync) SyncUserHook(ctx context.Context, id *authn.Identity, _ *auth
// Does user exist in the database?
usr, userAuth, errUserInDB := s.getUser(ctx, id)
if errUserInDB != nil && !errors.Is(errUserInDB, user.ErrUserNotFound) {
s.log.FromContext(ctx).Error("Failed to fetch user", "error", errUserInDB, "auth_module", id.AuthModule, "auth_id", id.AuthID)
s.log.FromContext(ctx).Error("Failed to fetch user", "error", errUserInDB, "auth_module", id.AuthenticatedBy, "auth_id", id.AuthID)
return errSyncUserInternal.Errorf("unable to retrieve user")
}

if errors.Is(errUserInDB, user.ErrUserNotFound) {
if !id.ClientParams.AllowSignUp {
s.log.FromContext(ctx).Warn("Failed to create user, signup is not allowed for module", "auth_module", id.AuthModule, "auth_id", id.AuthID)
s.log.FromContext(ctx).Warn("Failed to create user, signup is not allowed for module", "auth_module", id.AuthenticatedBy, "auth_id", id.AuthID)
return errUserSignupDisabled.Errorf("%w", login.ErrSignupNotAllowed)
}

// create user
var errCreate error
usr, errCreate = s.createUser(ctx, id)
if errCreate != nil {
s.log.FromContext(ctx).Error("Failed to create user", "error", errCreate, "auth_module", id.AuthModule, "auth_id", id.AuthID)
s.log.FromContext(ctx).Error("Failed to create user", "error", errCreate, "auth_module", id.AuthenticatedBy, "auth_id", id.AuthID)
return errSyncUserInternal.Errorf("unable to create user")
}
} else {
// update user
if errUpdate := s.updateUserAttributes(ctx, usr, id, userAuth); errUpdate != nil {
s.log.FromContext(ctx).Error("Failed to update user", "error", errUpdate, "auth_module", id.AuthModule, "auth_id", id.AuthID)
s.log.FromContext(ctx).Error("Failed to update user", "error", errUpdate, "auth_module", id.AuthenticatedBy, "auth_id", id.AuthID)
return errSyncUserInternal.Errorf("unable to update user")
}
}
Expand Down Expand Up @@ -174,7 +174,7 @@ func (s *UserSync) EnableDisabledUserHook(ctx context.Context, identity *authn.I
}

func (s *UserSync) upsertAuthConnection(ctx context.Context, userID int64, identity *authn.Identity, createConnection bool) error {
if identity.AuthModule == "" {
if identity.AuthenticatedBy == "" {
return nil
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

works for me, thanks for the confirmation

return s.authInfoService.SetAuthInfo(ctx, &login.SetAuthInfoCommand{
UserId: userID,
AuthModule: identity.AuthModule,
AuthModule: identity.AuthenticatedBy,
AuthId: identity.AuthID,
OAuthToken: identity.OAuthToken,
})
Expand All @@ -194,13 +194,13 @@ func (s *UserSync) upsertAuthConnection(ctx context.Context, userID int64, ident
return s.authInfoService.UpdateAuthInfo(ctx, &login.UpdateAuthInfoCommand{
UserId: userID,
AuthId: identity.AuthID,
AuthModule: identity.AuthModule,
AuthModule: identity.AuthenticatedBy,
OAuthToken: identity.OAuthToken,
})
}

func (s *UserSync) updateUserAttributes(ctx context.Context, usr *user.User, id *authn.Identity, userAuth *login.UserAuth) error {
if errProtection := s.userProtectionService.AllowUserMapping(usr, id.AuthModule); errProtection != nil {
if errProtection := s.userProtectionService.AllowUserMapping(usr, id.AuthenticatedBy); errProtection != nil {
return errUserProtection.Errorf("user mapping not allowed: %w", errProtection)
}
// sync user info
Expand Down Expand Up @@ -286,8 +286,8 @@ func (s *UserSync) createUser(ctx context.Context, id *authn.Identity) (*user.Us

func (s *UserSync) getUser(ctx context.Context, identity *authn.Identity) (*user.User, *login.UserAuth, error) {
// Check auth info fist
if identity.AuthID != "" && identity.AuthModule != "" {
query := &login.GetAuthInfoQuery{AuthId: identity.AuthID, AuthModule: identity.AuthModule}
if identity.AuthID != "" && identity.AuthenticatedBy != "" {
query := &login.GetAuthInfoQuery{AuthId: identity.AuthID, AuthModule: identity.AuthenticatedBy}
authInfo, errGetAuthInfo := s.authInfoService.GetAuthInfo(ctx, query)

if errGetAuthInfo != nil && !errors.Is(errGetAuthInfo, user.ErrUserNotFound) {
Expand All @@ -307,7 +307,7 @@ func (s *UserSync) getUser(ctx context.Context, identity *authn.Identity) (*user
// if the user connected to user auth does not exist try to clean it up
if errors.Is(errGetByID, user.ErrUserNotFound) {
if err := s.authInfoService.DeleteUserAuthInfo(ctx, authInfo.UserId); err != nil {
s.log.FromContext(ctx).Error("Failed to clean up user auth", "error", err, "auth_module", identity.AuthModule, "auth_id", identity.AuthID)
s.log.FromContext(ctx).Error("Failed to clean up user auth", "error", err, "auth_module", identity.AuthenticatedBy, "auth_id", identity.AuthID)
}
}
}
Expand All @@ -322,8 +322,8 @@ func (s *UserSync) getUser(ctx context.Context, identity *authn.Identity) (*user
var userAuth *login.UserAuth
// Special case for generic oauth: generic oauth does not store authID,
// so we need to find the user first then check for the userAuth connection by module and userID
if identity.AuthModule == login.GenericOAuthModule {
query := &login.GetAuthInfoQuery{AuthModule: identity.AuthModule, UserId: usr.ID}
if identity.AuthenticatedBy == login.GenericOAuthModule {
query := &login.GetAuthInfoQuery{AuthModule: identity.AuthenticatedBy, UserId: usr.ID}
userAuth, err = s.authInfoService.GetAuthInfo(ctx, query)
if err != nil && !errors.Is(err, user.ErrUserNotFound) {
return nil, nil, err
Expand Down
66 changes: 33 additions & 33 deletions pkg/services/authn/authnimpl/sync/user_sync_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,12 +268,12 @@ func TestUserSync_SyncUserHook(t *testing.T) {
args: args{
ctx: context.Background(),
id: &authn.Identity{
ID: "",
AuthID: "2032",
AuthModule: "oauth",
Login: "test",
Name: "test",
Email: "test",
ID: "",
AuthID: "2032",
AuthenticatedBy: "oauth",
Login: "test",
Name: "test",
Email: "test",
ClientParams: authn.ClientParams{
SyncUser: true,
LookUpParams: login.UserLookupParams{
Expand All @@ -286,13 +286,13 @@ func TestUserSync_SyncUserHook(t *testing.T) {
},
wantErr: false,
wantID: &authn.Identity{
ID: "user:1",
AuthID: "2032",
AuthModule: "oauth",
Login: "test",
Name: "test",
Email: "test",
IsGrafanaAdmin: ptrBool(false),
ID: "user:1",
AuthID: "2032",
AuthenticatedBy: "oauth",
Login: "test",
Name: "test",
Email: "test",
IsGrafanaAdmin: ptrBool(false),
ClientParams: authn.ClientParams{
SyncUser: true,
LookUpParams: login.UserLookupParams{
Expand All @@ -313,12 +313,12 @@ func TestUserSync_SyncUserHook(t *testing.T) {
args: args{
ctx: context.Background(),
id: &authn.Identity{
ID: "",
Login: "test",
Name: "test",
Email: "test",
AuthModule: "oauth",
AuthID: "2032",
ID: "",
Login: "test",
Name: "test",
Email: "test",
AuthenticatedBy: "oauth",
AuthID: "2032",
ClientParams: authn.ClientParams{
SyncUser: true,
LookUpParams: login.UserLookupParams{
Expand All @@ -341,13 +341,13 @@ func TestUserSync_SyncUserHook(t *testing.T) {
args: args{
ctx: context.Background(),
id: &authn.Identity{
ID: "",
Login: "test_create",
Name: "test_create",
IsGrafanaAdmin: ptrBool(true),
Email: "test_create",
AuthModule: "oauth",
AuthID: "2032",
ID: "",
Login: "test_create",
Name: "test_create",
IsGrafanaAdmin: ptrBool(true),
Email: "test_create",
AuthenticatedBy: "oauth",
AuthID: "2032",
ClientParams: authn.ClientParams{
SyncUser: true,
AllowSignUp: true,
Expand All @@ -362,13 +362,13 @@ func TestUserSync_SyncUserHook(t *testing.T) {
},
wantErr: false,
wantID: &authn.Identity{
ID: "user:2",
Login: "test_create",
Name: "test_create",
Email: "test_create",
AuthModule: "oauth",
AuthID: "2032",
IsGrafanaAdmin: ptrBool(true),
ID: "user:2",
Login: "test_create",
Name: "test_create",
Email: "test_create",
AuthenticatedBy: "oauth",
AuthID: "2032",
IsGrafanaAdmin: ptrBool(true),
ClientParams: authn.ClientParams{
SyncUser: true,
AllowSignUp: true,
Expand Down
3 changes: 2 additions & 1 deletion pkg/services/authn/clients/api_key.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/services/apikey"
"github.com/grafana/grafana/pkg/services/authn"
"github.com/grafana/grafana/pkg/services/login"
"github.com/grafana/grafana/pkg/services/org"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/util"
Expand Down Expand Up @@ -80,7 +81,7 @@ func (s *APIKey) Authenticate(ctx context.Context, r *authn.Request) (*authn.Ide
return nil, err
}

return authn.IdentityFromSignedInUser(authn.NamespacedID(authn.NamespaceServiceAccount, usr.UserID), usr, authn.ClientParams{SyncPermissions: true}), nil
return authn.IdentityFromSignedInUser(authn.NamespacedID(authn.NamespaceServiceAccount, usr.UserID), usr, authn.ClientParams{SyncPermissions: true}, login.APIKeyAuthModule), nil
}

func (s *APIKey) getAPIKey(ctx context.Context, token string) (*apikey.APIKey, error) {
Expand Down
2 changes: 2 additions & 0 deletions pkg/services/authn/clients/api_key_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/grafana/grafana/pkg/services/apikey"
"github.com/grafana/grafana/pkg/services/apikey/apikeytest"
"github.com/grafana/grafana/pkg/services/authn"
"github.com/grafana/grafana/pkg/services/login"
"github.com/grafana/grafana/pkg/services/org"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/services/user/usertest"
Expand Down Expand Up @@ -88,6 +89,7 @@ func TestAPIKey_Authenticate(t *testing.T) {
ClientParams: authn.ClientParams{
SyncPermissions: true,
},
AuthenticatedBy: login.APIKeyAuthModule,
},
},
{
Expand Down
3 changes: 2 additions & 1 deletion pkg/services/authn/clients/ext_jwt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (

"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/services/authn"
"github.com/grafana/grafana/pkg/services/login"
"github.com/grafana/grafana/pkg/services/oauthserver"
"github.com/grafana/grafana/pkg/services/signingkeys"
"github.com/grafana/grafana/pkg/services/user"
Expand Down Expand Up @@ -100,7 +101,7 @@ func (s *ExtendedJWT) Authenticate(ctx context.Context, r *authn.Request) (*auth

signedInUser.Permissions[s.getDefaultOrgID()] = claims.Entitlements

return authn.IdentityFromSignedInUser(authn.NamespacedID(authn.NamespaceUser, signedInUser.UserID), signedInUser, authn.ClientParams{SyncPermissions: false}), nil
return authn.IdentityFromSignedInUser(authn.NamespacedID(authn.NamespaceUser, signedInUser.UserID), signedInUser, authn.ClientParams{SyncPermissions: false}, login.ExtendedJWTModule), nil
}

func (s *ExtendedJWT) Test(ctx context.Context, r *authn.Request) bool {
Expand Down
26 changes: 13 additions & 13 deletions pkg/services/authn/clients/ext_jwt_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,19 +150,19 @@ func TestExtendedJWT_Authenticate(t *testing.T) {
}
},
want: &authn.Identity{
OrgID: 1,
OrgCount: 0,
OrgName: "",
OrgRoles: map[int64]roletype.RoleType{1: roletype.RoleAdmin},
ID: "user:2",
Login: "johndoe",
Name: "John Doe",
Email: "[email protected]",
IsGrafanaAdmin: boolPtr(false),
AuthModule: "",
AuthID: "",
IsDisabled: false,
HelpFlags1: 0,
OrgID: 1,
OrgCount: 0,
OrgName: "",
OrgRoles: map[int64]roletype.RoleType{1: roletype.RoleAdmin},
ID: "user:2",
Login: "johndoe",
Name: "John Doe",
Email: "[email protected]",
IsGrafanaAdmin: boolPtr(false),
AuthenticatedBy: login.ExtendedJWTModule,
AuthID: "",
IsDisabled: false,
HelpFlags1: 0,
Permissions: map[int64]map[string][]string{
1: {
"dashboards:create": {
Expand Down
6 changes: 3 additions & 3 deletions pkg/services/authn/clients/grafana.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ func (c *Grafana) String() string {

func (c *Grafana) AuthenticateProxy(ctx context.Context, r *authn.Request, username string, additional map[string]string) (*authn.Identity, error) {
identity := &authn.Identity{
AuthModule: login.AuthProxyAuthModule,
AuthID: username,
AuthenticatedBy: login.AuthProxyAuthModule,
AuthID: username,
ClientParams: authn.ClientParams{
SyncUser: true,
SyncTeams: true,
Expand Down Expand Up @@ -109,7 +109,7 @@ func (c *Grafana) AuthenticatePassword(ctx context.Context, r *authn.Request, us
return nil, err
}

return authn.IdentityFromSignedInUser(authn.NamespacedID(authn.NamespaceUser, signedInUser.UserID), signedInUser, authn.ClientParams{SyncPermissions: true}), nil
return authn.IdentityFromSignedInUser(authn.NamespacedID(authn.NamespaceUser, signedInUser.UserID), signedInUser, authn.ClientParams{SyncPermissions: true}, login.PasswordAuthModule), nil
mgyongyosi marked this conversation as resolved.
Show resolved Hide resolved
}

func comparePassword(password, salt, hash string) bool {
Expand Down
Loading