public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

AuthN: Use typed namespace id inside authn package (#86048)

Browse Source 
* authn: Use typed namespace id inside package
This commit is contained in:
Karl Persson
2024-04-24 09:57:34 +02:00
committed by GitHub
parent 804c726413
commit 0fa983ad8e
36 changed files with 189 additions and 203 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/services/authn/authnimpl/sync/oauth_token_sync.go
+3 -4
View File
@@ -34,9 +34,8 @@ type OAuthTokenSync struct {
}
func (s *OAuthTokenSync) SyncOauthTokenHook(ctx context.Context, identity *authn.Identity, _ *authn.Request) error {
namespace, _ := identity.GetNamespacedID()
// only perform oauth token check if identity is a user
if namespace != authn.NamespaceUser {
if !identity.ID.IsNamespace(authn.NamespaceUser) {
return nil
}
@@ -50,8 +49,8 @@ func (s *OAuthTokenSync) SyncOauthTokenHook(ctx context.Context, identity *authn
return nil
}
_, err, _ := s.singleflightGroup.Do(identity.ID, func() (interface{}, error) {
s.log.Debug("Singleflight request for OAuth token sync", "key", identity.ID)
_, err, _ := s.singleflightGroup.Do(identity.ID.String(), func() (interface{}, error) {
s.log.Debug("Singleflight request for OAuth token sync", "key", identity.ID.String())
// FIXME: Consider using context.WithoutCancel instead of context.Background after Go 1.21 update
updateCtx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
pkg/services/authn/authnimpl/sync/oauth_token_sync_test.go
+5 -5
View File
@@ -41,17 +41,17 @@ func TestOAuthTokenSync_SyncOAuthTokenHook(t *testing.T) {
tests := []testCase{
{
desc: "should skip sync when identity is not a user",
identity: &authn.Identity{ID: "service-account:1"},
identity: &authn.Identity{ID: authn.MustParseNamespaceID("service-account:1")},
expectTryRefreshTokenCalled: false,
},
{
desc: "should skip sync when identity is a user but is not authenticated with session token",
identity: &authn.Identity{ID: "user:1"},
identity: &authn.Identity{ID: authn.MustParseNamespaceID("user:1")},
expectTryRefreshTokenCalled: false,
},
{
desc: "should invalidate access token and session token if token refresh fails",
identity: &authn.Identity{ID: "user:1", SessionToken: &auth.UserToken{}, AuthenticatedBy: login.AzureADAuthModule},
identity: &authn.Identity{ID: authn.MustParseNamespaceID("user:1"), SessionToken: &auth.UserToken{}, AuthenticatedBy: login.AzureADAuthModule},
expectHasEntryCalled: true,
expectedTryRefreshErr: errors.New("some err"),
expectTryRefreshTokenCalled: true,
@@ -62,7 +62,7 @@ func TestOAuthTokenSync_SyncOAuthTokenHook(t *testing.T) {
},
{
desc: "should refresh the token successfully",
identity: &authn.Identity{ID: "user:1", SessionToken: &auth.UserToken{}, AuthenticatedBy: login.AzureADAuthModule},
identity: &authn.Identity{ID: authn.MustParseNamespaceID("user:1"), SessionToken: &auth.UserToken{}, AuthenticatedBy: login.AzureADAuthModule},
expectHasEntryCalled: false,
expectTryRefreshTokenCalled: true,
expectInvalidateOauthTokensCalled: false,
@@ -70,7 +70,7 @@ func TestOAuthTokenSync_SyncOAuthTokenHook(t *testing.T) {
},
{
desc: "should not invalidate the token if the token has already been refreshed by another request (singleflight)",
identity: &authn.Identity{ID: "user:1", SessionToken: &auth.UserToken{}, AuthenticatedBy: login.AzureADAuthModule},
identity: &authn.Identity{ID: authn.MustParseNamespaceID("user:1"), SessionToken: &auth.UserToken{}, AuthenticatedBy: login.AzureADAuthModule},
expectHasEntryCalled: true,
expectTryRefreshTokenCalled: true,
expectInvalidateOauthTokensCalled: false,
pkg/services/authn/authnimpl/sync/org_sync.go
+8 -11
View File
@@ -8,7 +8,6 @@ import (
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/auth/identity"
"github.com/grafana/grafana/pkg/services/authn"
"github.com/grafana/grafana/pkg/services/org"
"github.com/grafana/grafana/pkg/services/user"
@@ -35,15 +34,14 @@ func (s *OrgSync) SyncOrgRolesHook(ctx context.Context, id *authn.Identity, _ *a
ctxLogger := s.log.FromContext(ctx).New("id", id.ID, "login", id.Login)
namespace, identifier := id.GetNamespacedID()
if namespace != authn.NamespaceUser {
ctxLogger.Warn("Failed to sync org role, invalid namespace for identity", "namespace", namespace)
if !id.ID.IsNamespace(authn.NamespaceUser) {
ctxLogger.Warn("Failed to sync org role, invalid namespace for identity", "namespace", id.ID.Namespace())
return nil
}
userID, err := identity.IntIdentifier(namespace, identifier)
userID, err := id.ID.ParseInt()
if err != nil {
ctxLogger.Warn("Failed to sync org role, invalid ID for identity", "namespace", namespace, "err", err)
ctxLogger.Warn("Failed to sync org role, invalid ID for identity", "namespace", id.ID.Namespace(), "err", err)
return nil
}
@@ -139,15 +137,14 @@ func (s *OrgSync) SetDefaultOrgHook(ctx context.Context, currentIdentity *authn.
ctxLogger := s.log.FromContext(ctx)
namespace, identifier := currentIdentity.GetNamespacedID()
if namespace != identity.NamespaceUser {
ctxLogger.Debug("Skipping default org sync, not a user", "namespace", namespace)
if !currentIdentity.ID.IsNamespace(authn.NamespaceUser) {
ctxLogger.Debug("Skipping default org sync, not a user", "namespace", currentIdentity.ID.Namespace())
return
}
userID, err := identity.IntIdentifier(namespace, identifier)
userID, err := currentIdentity.ID.ParseInt()
if err != nil {
ctxLogger.Debug("Skipping default org sync, invalid ID for identity", "id", currentIdentity.ID, "namespace", namespace, "err", err)
ctxLogger.Debug("Skipping default org sync, invalid ID for identity", "id", currentIdentity.ID, "namespace", currentIdentity.ID.Namespace(), "err", err)
return
}
pkg/services/authn/authnimpl/sync/org_sync_test.go
+10 -10
View File
@@ -75,7 +75,7 @@ func TestOrgSync_SyncOrgRolesHook(t *testing.T) {
args: args{
ctx: context.Background(),
id: &authn.Identity{
ID: "user:1",
ID: authn.MustParseNamespaceID("user:1"),
Login: "test",
Name: "test",
Email: "test",
@@ -91,7 +91,7 @@ func TestOrgSync_SyncOrgRolesHook(t *testing.T) {
},
},
wantID: &authn.Identity{
ID: "user:1",
ID: authn.MustParseNamespaceID("user:1"),
Login: "test",
Name: "test",
Email: "test",
@@ -137,7 +137,7 @@ func TestOrgSync_SetDefaultOrgHook(t *testing.T) {
{
name: "should set default org",
defaultOrgSetting: 2,
identity: &authn.Identity{ID: "user:1"},
identity: &authn.Identity{ID: authn.MustParseNamespaceID("user:1")},
setupMock: func(userService *usertest.MockService, orgService *orgtest.FakeOrgService) {
userService.On("SetUsingOrg", mock.Anything, mock.MatchedBy(func(cmd *user.SetUsingOrgCommand) bool {
return cmd.UserID == 1 && cmd.OrgID == 2
@@ -147,7 +147,7 @@ func TestOrgSync_SetDefaultOrgHook(t *testing.T) {
{
name: "should skip setting the default org when default org is not set",
defaultOrgSetting: -1,
identity: &authn.Identity{ID: "user:1"},
identity: &authn.Identity{ID: authn.MustParseNamespaceID("user:1")},
},
{
name: "should skip setting the default org when identity is nil",
@@ -157,28 +157,28 @@ func TestOrgSync_SetDefaultOrgHook(t *testing.T) {
{
name: "should skip setting the default org when input err is not nil",
defaultOrgSetting: 2,
identity: &authn.Identity{ID: "user:1"},
identity: &authn.Identity{ID: authn.MustParseNamespaceID("user:1")},
inputErr: fmt.Errorf("error"),
},
{
name: "should skip setting the default org when identity is not a user",
defaultOrgSetting: 2,
identity: &authn.Identity{ID: "service-account:1"},
identity: &authn.Identity{ID: authn.MustParseNamespaceID("service-account:1")},
},
{
name: "should skip setting the default org when user id is not valid",
defaultOrgSetting: 2,
identity: &authn.Identity{ID: "user:invalid"},
identity: &authn.Identity{ID: authn.MustParseNamespaceID("user:invalid")},
},
{
name: "should skip setting the default org when user is not allowed to use the configured default org",
defaultOrgSetting: 3,
identity: &authn.Identity{ID: "user:1"},
identity: &authn.Identity{ID: authn.MustParseNamespaceID("user:1")},
},
{
name: "should skip setting the default org when validateUsingOrg returns error",
defaultOrgSetting: 2,
identity: &authn.Identity{ID: "user:1"},
identity: &authn.Identity{ID: authn.MustParseNamespaceID("user:1")},
setupMock: func(userService *usertest.MockService, orgService *orgtest.FakeOrgService) {
orgService.ExpectedError = fmt.Errorf("error")
},
@@ -186,7 +186,7 @@ func TestOrgSync_SetDefaultOrgHook(t *testing.T) {
{
name: "should skip the hook when the user org update was unsuccessful",
defaultOrgSetting: 2,
identity: &authn.Identity{ID: "user:1"},
identity: &authn.Identity{ID: authn.MustParseNamespaceID("user:1")},
setupMock: func(userService *usertest.MockService, orgService *orgtest.FakeOrgService) {
userService.On("SetUsingOrg", mock.Anything, mock.Anything).Return(fmt.Errorf("error"))
},
pkg/services/authn/authnimpl/sync/rbac_sync.go
+2 -4
View File
@@ -6,7 +6,6 @@ import (
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/auth/identity"
"github.com/grafana/grafana/pkg/services/authn"
"github.com/grafana/grafana/pkg/services/login"
"github.com/grafana/grafana/pkg/services/org"
@@ -98,13 +97,12 @@ func (s *RBACSync) SyncCloudRoles(ctx context.Context, ident *authn.Identity, r
return nil
}
namespace, id := ident.GetNamespacedID()
if namespace != authn.NamespaceUser {
if !ident.ID.IsNamespace(authn.NamespaceUser) {
s.log.FromContext(ctx).Debug("Skip syncing cloud role", "id", ident.ID)
return nil
}
userID, err := identity.IntIdentifier(namespace, id)
userID, err := ident.ID.ParseInt()
if err != nil {
return err
}
pkg/services/authn/authnimpl/sync/rbac_sync_test.go
+7 -7
View File
@@ -24,14 +24,14 @@ func TestRBACSync_SyncPermission(t *testing.T) {
testCases := []testCase{
{
name: "enriches the identity successfully when SyncPermissions is true",
identity: &authn.Identity{ID: "user:2", OrgID: 1, ClientParams: authn.ClientParams{SyncPermissions: true}},
identity: &authn.Identity{ID: authn.MustParseNamespaceID("user:2"), OrgID: 1, ClientParams: authn.ClientParams{SyncPermissions: true}},
expectedPermissions: []accesscontrol.Permission{
{Action: accesscontrol.ActionUsersRead},
},
},
{
name: "does not load the permissions when SyncPermissions is false",
identity: &authn.Identity{ID: "user:2", OrgID: 1, ClientParams: authn.ClientParams{SyncPermissions: true}},
identity: &authn.Identity{ID: authn.MustParseNamespaceID("user:2"), OrgID: 1, ClientParams: authn.ClientParams{SyncPermissions: true}},
expectedPermissions: []accesscontrol.Permission{
{Action: accesscontrol.ActionUsersRead},
},
@@ -65,7 +65,7 @@ func TestRBACSync_SyncCloudRoles(t *testing.T) {
desc: "should call sync when authenticated with grafana com and has viewer role",
module: login.GrafanaComAuthModule,
identity: &authn.Identity{
ID: authn.NamespacedID(authn.NamespaceUser, 1),
ID: authn.MustNewNamespaceID(authn.NamespaceUser, 1),
OrgID: 1,
OrgRoles: map[int64]org.RoleType{1: org.RoleViewer},
},
@@ -76,7 +76,7 @@ func TestRBACSync_SyncCloudRoles(t *testing.T) {
desc: "should call sync when authenticated with grafana com and has editor role",
module: login.GrafanaComAuthModule,
identity: &authn.Identity{
ID: authn.NamespacedID(authn.NamespaceUser, 1),
ID: authn.MustNewNamespaceID(authn.NamespaceUser, 1),
OrgID: 1,
OrgRoles: map[int64]org.RoleType{1: org.RoleEditor},
},
@@ -87,7 +87,7 @@ func TestRBACSync_SyncCloudRoles(t *testing.T) {
desc: "should call sync when authenticated with grafana com and has admin role",
module: login.GrafanaComAuthModule,
identity: &authn.Identity{
ID: authn.NamespacedID(authn.NamespaceUser, 1),
ID: authn.MustNewNamespaceID(authn.NamespaceUser, 1),
OrgID: 1,
OrgRoles: map[int64]org.RoleType{1: org.RoleAdmin},
},
@@ -98,7 +98,7 @@ func TestRBACSync_SyncCloudRoles(t *testing.T) {
desc: "should not call sync when authenticated with grafana com and has invalid role",
module: login.GrafanaComAuthModule,
identity: &authn.Identity{
ID: authn.NamespacedID(authn.NamespaceUser, 1),
ID: authn.MustNewNamespaceID(authn.NamespaceUser, 1),
OrgID: 1,
OrgRoles: map[int64]org.RoleType{1: org.RoleType("something else")},
},
@@ -109,7 +109,7 @@ func TestRBACSync_SyncCloudRoles(t *testing.T) {
desc: "should not call sync when not authenticated with grafana com",
module: login.LDAPAuthModule,
identity: &authn.Identity{
ID: authn.NamespacedID(authn.NamespaceUser, 1),
ID: authn.MustNewNamespaceID(authn.NamespaceUser, 1),
OrgID: 1,
OrgRoles: map[int64]org.RoleType{1: org.RoleAdmin},
},
pkg/services/authn/authnimpl/sync/user_sync.go
+1 -1
View File
@@ -394,7 +394,7 @@ func (s *UserSync) lookupByOneOf(ctx context.Context, params login.UserLookupPar
// syncUserToIdentity syncs a user to an identity.
// This is used to update the identity with the latest user information.
func syncUserToIdentity(usr *user.User, id *authn.Identity) {
id.ID = authn.NamespacedID(authn.NamespaceUser, usr.ID)
id.ID = authn.NewNamespaceIDUnchecked(authn.NamespaceUser, usr.ID)
id.Login = usr.Login
id.Email = usr.Email
id.Name = usr.Name
pkg/services/authn/authnimpl/sync/user_sync_test.go
+10 -19
View File
@@ -110,7 +110,6 @@ func TestUserSync_SyncUserHook(t *testing.T) {
args: args{
ctx: context.Background(),
id: &authn.Identity{
ID: "",
Login: "test",
Name: "test",
Email: "test",
@@ -124,7 +123,6 @@ func TestUserSync_SyncUserHook(t *testing.T) {
},
wantErr: false,
wantID: &authn.Identity{
ID: "",
Login: "test",
Name: "test",
Email: "test",
@@ -146,7 +144,6 @@ func TestUserSync_SyncUserHook(t *testing.T) {
args: args{
ctx: context.Background(),
id: &authn.Identity{
ID: "",
Login: "test",
Name: "test",
Email: "test",
@@ -161,7 +158,7 @@ func TestUserSync_SyncUserHook(t *testing.T) {
},
wantErr: false,
wantID: &authn.Identity{
ID: "user:1",
ID: authn.MustParseNamespaceID("user:1"),
Login: "test",
Name: "test",
Email: "test",
@@ -185,7 +182,6 @@ func TestUserSync_SyncUserHook(t *testing.T) {
args: args{
ctx: context.Background(),
id: &authn.Identity{
ID: "",
Login: "test",
Name: "test",
Email: "test",
@@ -200,7 +196,7 @@ func TestUserSync_SyncUserHook(t *testing.T) {
},
wantErr: false,
wantID: &authn.Identity{
ID: "user:1",
ID: authn.MustParseNamespaceID("user:1"),
Login: "test",
Name: "test",
Email: "test",
@@ -224,7 +220,6 @@ func TestUserSync_SyncUserHook(t *testing.T) {
args: args{
ctx: context.Background(),
id: &authn.Identity{
ID: "",
AuthID: "2032",
AuthenticatedBy: "oauth",
Login: "test",
@@ -241,7 +236,7 @@ func TestUserSync_SyncUserHook(t *testing.T) {
},
wantErr: false,
wantID: &authn.Identity{
ID: "user:1",
ID: authn.MustParseNamespaceID("user:1"),
AuthID: "2032",
AuthenticatedBy: "oauth",
Login: "test",
@@ -267,7 +262,6 @@ func TestUserSync_SyncUserHook(t *testing.T) {
args: args{
ctx: context.Background(),
id: &authn.Identity{
ID: "",
Login: "test",
Name: "test",
Email: "test",
@@ -294,7 +288,6 @@ func TestUserSync_SyncUserHook(t *testing.T) {
args: args{
ctx: context.Background(),
id: &authn.Identity{
ID: "",
Login: "test_create",
Name: "test_create",
IsGrafanaAdmin: ptrBool(true),
@@ -314,7 +307,7 @@ func TestUserSync_SyncUserHook(t *testing.T) {
},
wantErr: false,
wantID: &authn.Identity{
ID: "user:2",
ID: authn.MustParseNamespaceID("user:2"),
Login: "test_create",
Name: "test_create",
Email: "test_create",
@@ -342,7 +335,6 @@ func TestUserSync_SyncUserHook(t *testing.T) {
args: args{
ctx: context.Background(),
id: &authn.Identity{
ID: "",
Login: "test_mod",
Name: "test_mod",
Email: "test_mod",
@@ -360,7 +352,7 @@ func TestUserSync_SyncUserHook(t *testing.T) {
},
wantErr: false,
wantID: &authn.Identity{
ID: "user:3",
ID: authn.MustParseNamespaceID("user:3"),
Login: "test_mod",
Name: "test_mod",
Email: "test_mod",
@@ -386,7 +378,6 @@ func TestUserSync_SyncUserHook(t *testing.T) {
args: args{
ctx: context.Background(),
id: &authn.Identity{
ID: "",
Login: "test",
Name: "test",
Email: "test_mod@test.com",
@@ -405,7 +396,7 @@ func TestUserSync_SyncUserHook(t *testing.T) {
},
wantErr: false,
wantID: &authn.Identity{
ID: "user:3",
ID: authn.MustParseNamespaceID("user:3"),
Login: "test",
Name: "test",
Email: "test_mod@test.com",
@@ -455,7 +446,7 @@ func TestUserSync_FetchSyncedUserHook(t *testing.T) {
{
desc: "should skip hook when identity is not a user",
req: &authn.Request{},
identity: &authn.Identity{ID: "apikey:1", ClientParams: authn.ClientParams{FetchSyncedUser: true}},
identity: &authn.Identity{ID: authn.MustParseNamespaceID("api-key:1"), ClientParams: authn.ClientParams{FetchSyncedUser: true}},
},
}
@@ -479,7 +470,7 @@ func TestUserSync_EnableDisabledUserHook(t *testing.T) {
{
desc: "should skip if correct flag is not set",
identity: &authn.Identity{
ID: authn.NamespacedID(authn.NamespaceUser, 1),
ID: authn.MustNewNamespaceID(authn.NamespaceUser, 1),
IsDisabled: true,
ClientParams: authn.ClientParams{EnableUser: false},
},
@@ -488,7 +479,7 @@ func TestUserSync_EnableDisabledUserHook(t *testing.T) {
{
desc: "should skip if identity is not a user",
identity: &authn.Identity{
ID: authn.NamespacedID(authn.NamespaceAPIKey, 1),
ID: authn.MustNewNamespaceID(authn.NamespaceAPIKey, 1),
IsDisabled: true,
ClientParams: authn.ClientParams{EnableUser: true},
},
@@ -497,7 +488,7 @@ func TestUserSync_EnableDisabledUserHook(t *testing.T) {
{
desc: "should enabled disabled user",
identity: &authn.Identity{
ID: authn.NamespacedID(authn.NamespaceUser, 1),
ID: authn.MustNewNamespaceID(authn.NamespaceUser, 1),
IsDisabled: true,
ClientParams: authn.ClientParams{EnableUser: true},
},