Auth: Restore legacy behavior and add deprecation notice for empty org role in oauth (#55118)

* Auth: Add deprecation notice for empty org role

Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>

* fix recasts

* fix azure tests missing logger

* Adding test to gitlab oauth

* Covering more cases

* Cover more options

* Add role attributestrict check fail

* Adding one more edge case test

* Using legacy for gitlab

* Yet another edge case YAEC

* Reverting github oauth to legacy

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Not using token

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Nit.

* Adding warning in docs

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* add warning to generic oauth

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Be more precise

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Adding warning to github oauth

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Adding warning to gitlab oauth

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Adding warning to okta oauth

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Add docs about mapping to AzureAD

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Clarify oauth_skip_org_role_update_sync

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Nit.

* Nit on Azure AD

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Reorder docs index

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

* Fix typo

Co-authored-by: Jguer <joao.guerreiro@grafana.com>

Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>
Co-authored-by: gamab <gabi.mabs@gmail.com>
This commit is contained in:
Jo
2022-09-15 17:35:59 +02:00
committed by GitHub
co-authored by Gabriel MABILLE gamab
parent f1e8a528d1
commit 00e7324bf6
18 changed files with 320 additions and 77 deletions
+1 -1
View File
@@ -275,7 +275,7 @@ func (hs *HTTPServer) buildExternalUserInfo(token *oauth2.Token, userInfo *socia
}
if userInfo.Role != "" && !hs.Cfg.OAuthSkipOrgRoleUpdateSync {
rt := org.RoleType(userInfo.Role)
rt := userInfo.Role
if rt.IsValid() {
// The user will be assigned a role in either the auto-assigned organization or in the default one
var orgID int64
+8 -21
View File
@@ -68,10 +68,11 @@ func (s *SocialAzureAD) UserInfo(client *http.Client, token *oauth2.Token) (*Bas
return nil, ErrEmailNotFound
}
role, grafanaAdmin := claims.extractRoleAndAdmin(s.autoAssignOrgRole, s.roleAttributeStrict)
if role == "" {
role, grafanaAdmin := s.extractRoleAndAdmin(&claims)
if s.roleAttributeStrict && !role.IsValid() {
return nil, ErrInvalidBasicRole
}
logger.Debug("AzureAD OAuth: extracted role", "email", email, "role", role)
groups, err := extractGroups(client, claims, token)
@@ -94,7 +95,7 @@ func (s *SocialAzureAD) UserInfo(client *http.Client, token *oauth2.Token) (*Bas
Name: claims.Name,
Email: email,
Login: email,
Role: string(role),
Role: role,
IsGrafanaAdmin: isGrafanaAdmin,
Groups: groups,
}, nil
@@ -127,22 +128,12 @@ func (claims *azureClaims) extractEmail() string {
}
// extractRoleAndAdmin extracts the role from the claims and returns the role and whether the user is a Grafana admin.
func (claims *azureClaims) extractRoleAndAdmin(autoAssignRole string, strictMode bool) (org.RoleType, bool) {
func (s *SocialAzureAD) extractRoleAndAdmin(claims *azureClaims) (org.RoleType, bool) {
if len(claims.Roles) == 0 {
if strictMode {
return org.RoleType(""), false
}
return org.RoleType(autoAssignRole), false
}
roleOrder := []org.RoleType{
RoleGrafanaAdmin,
org.RoleAdmin,
org.RoleEditor,
org.RoleViewer,
return s.defaultRole(false), false
}
roleOrder := []org.RoleType{RoleGrafanaAdmin, org.RoleAdmin, org.RoleEditor, org.RoleViewer}
for _, role := range roleOrder {
if found := hasRole(claims.Roles, role); found {
if role == RoleGrafanaAdmin {
@@ -153,11 +144,7 @@ func (claims *azureClaims) extractRoleAndAdmin(autoAssignRole string, strictMode
}
}
if strictMode {
return org.RoleType(""), false
}
return org.RoleViewer, false
return s.defaultRole(false), false
}
func hasRole(roles []string, role org.RoleType) bool {
+14 -11
View File
@@ -5,7 +5,6 @@ import (
"encoding/json"
"net/http"
"net/http/httptest"
"reflect"
"strings"
"testing"
"time"
@@ -54,7 +53,7 @@ func TestSocialAzureAD_UserInfo(t *testing.T) {
ID: "1234",
},
fields: fields{
SocialBase: &SocialBase{autoAssignOrgRole: "Viewer"},
SocialBase: newSocialBase("azuread", &oauth2.Config{}, &OAuthInfo{}, "Viewer"),
},
want: &BasicUserInfo{
Id: "1234",
@@ -93,7 +92,7 @@ func TestSocialAzureAD_UserInfo(t *testing.T) {
ID: "1234",
},
fields: fields{
SocialBase: &SocialBase{autoAssignOrgRole: "Viewer"},
SocialBase: newSocialBase("azuread", &oauth2.Config{}, &OAuthInfo{}, "Viewer"),
},
want: &BasicUserInfo{
Id: "1234",
@@ -142,6 +141,9 @@ func TestSocialAzureAD_UserInfo(t *testing.T) {
},
{
name: "Only other roles",
fields: fields{
SocialBase: newSocialBase("azuread", &oauth2.Config{}, &OAuthInfo{}, "Viewer"),
},
claims: &azureClaims{
Email: "me@example.com",
PreferredUsername: "",
@@ -168,7 +170,7 @@ func TestSocialAzureAD_UserInfo(t *testing.T) {
ID: "1234",
},
fields: fields{
SocialBase: &SocialBase{autoAssignOrgRole: "Editor"},
SocialBase: newSocialBase("azuread", &oauth2.Config{}, &OAuthInfo{}, "Editor"),
},
want: &BasicUserInfo{
Id: "1234",
@@ -217,7 +219,7 @@ func TestSocialAzureAD_UserInfo(t *testing.T) {
},
{
name: "Grafana Admin but setting is disabled",
fields: fields{SocialBase: &SocialBase{allowAssignGrafanaAdmin: false}},
fields: fields{SocialBase: newSocialBase("azuread", &oauth2.Config{}, &OAuthInfo{AllowAssignGrafanaAdmin: false}, "Editor")},
claims: &azureClaims{
Email: "me@example.com",
PreferredUsername: "",
@@ -258,8 +260,9 @@ func TestSocialAzureAD_UserInfo(t *testing.T) {
},
},
{
name: "Grafana Admin and Editor roles in claim",
fields: fields{SocialBase: &SocialBase{allowAssignGrafanaAdmin: true}},
name: "Grafana Admin and Editor roles in claim",
fields: fields{SocialBase: newSocialBase("azuread",
&oauth2.Config{}, &OAuthInfo{AllowAssignGrafanaAdmin: true}, "")},
claims: &azureClaims{
Email: "me@example.com",
PreferredUsername: "",
@@ -297,7 +300,8 @@ func TestSocialAzureAD_UserInfo(t *testing.T) {
name: "Error if user is a member of allowed_groups",
fields: fields{
allowedGroups: []string{"foo", "bar"},
SocialBase: &SocialBase{autoAssignOrgRole: "Viewer"},
SocialBase: newSocialBase("azuread",
&oauth2.Config{}, &OAuthInfo{AllowAssignGrafanaAdmin: false}, "Viewer"),
},
claims: &azureClaims{
Email: "me@example.com",
@@ -443,9 +447,8 @@ func TestSocialAzureAD_UserInfo(t *testing.T) {
t.Errorf("UserInfo() error = %v, wantErr %v", err, tt.wantErr)
return
}
if !reflect.DeepEqual(got, tt.want) {
t.Errorf("UserInfo() got = %v, want %v", got, tt.want)
}
require.EqualValues(t, tt.want, got)
})
}
}
+6 -6
View File
@@ -144,15 +144,11 @@ func (s *SocialGenericOAuth) UserInfo(client *http.Client, token *oauth2.Token)
}
if userInfo.Role == "" {
role, grafanaAdmin := s.extractRoleAndAdmin(data.rawJSON, []string{})
role, grafanaAdmin := s.extractRoleAndAdmin(data.rawJSON, []string{}, true)
if role != "" {
if s.roleAttributeStrict && !role.IsValid() {
return nil, ErrInvalidBasicRole
}
s.log.Debug("Setting user info role from extracted role")
userInfo.Role = string(role)
userInfo.Role = role
if s.allowAssignGrafanaAdmin {
userInfo.IsGrafanaAdmin = &grafanaAdmin
}
@@ -170,6 +166,10 @@ func (s *SocialGenericOAuth) UserInfo(client *http.Client, token *oauth2.Token)
}
}
if s.roleAttributeStrict && !userInfo.Role.IsValid() {
return nil, ErrInvalidBasicRole
}
if userInfo.Email == "" {
var err error
userInfo.Email, err = s.FetchPrivateEmail(client)
+2 -1
View File
@@ -13,6 +13,7 @@ import (
"github.com/go-kit/log/level"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/services/org"
)
func newLogger(name string, lev string) log.Logger {
@@ -251,7 +252,7 @@ func TestUserInfoSearchesForEmailAndRole(t *testing.T) {
OAuth2Extra interface{}
RoleAttributePath string
ExpectedEmail string
ExpectedRole string
ExpectedRole org.RoleType
ExpectedGrafanaAdmin *bool
}{
{
+2 -2
View File
@@ -201,7 +201,7 @@ func (s *SocialGithub) UserInfo(client *http.Client, token *oauth2.Token) (*Basi
teams := convertToGroupList(teamMemberships)
role, grafanaAdmin := s.extractRoleAndAdmin(response.Body, teams)
role, grafanaAdmin := s.extractRoleAndAdmin(response.Body, teams, true)
if s.roleAttributeStrict && !role.IsValid() {
return nil, ErrInvalidBasicRole
}
@@ -216,7 +216,7 @@ func (s *SocialGithub) UserInfo(client *http.Client, token *oauth2.Token) (*Basi
Login: data.Login,
Id: fmt.Sprintf("%d", data.Id),
Email: data.Email,
Role: string(role),
Role: role,
Groups: teams,
IsGrafanaAdmin: isGrafanaAdmin,
}
+3 -3
View File
@@ -165,8 +165,8 @@ func TestSocialGitHub_UserInfo(t *testing.T) {
Groups: []string{"https://github.com/orgs/github/teams/justice-league", "@github/justice-league"},
},
},
{
name: "auto assign org role",
{ // Case that's going to change with Grafana 10
name: "No fallback to default org role (will change in Grafana 10)",
roleAttributePath: "",
userRawJSON: testGHUserJSON,
autoAssignOrgRole: "Editor",
@@ -176,7 +176,7 @@ func TestSocialGitHub_UserInfo(t *testing.T) {
Name: "monalisa octocat",
Email: "octocat@github.com",
Login: "octocat",
Role: "Editor",
Role: "",
Groups: []string{"https://github.com/orgs/github/teams/justice-league", "@github/justice-league"},
},
},
+3 -3
View File
@@ -89,7 +89,7 @@ func (s *SocialGitlab) GetGroupsPage(client *http.Client, url string) ([]string,
return fullPaths, next
}
func (s *SocialGitlab) UserInfo(client *http.Client, token *oauth2.Token) (*BasicUserInfo, error) {
func (s *SocialGitlab) UserInfo(client *http.Client, _ *oauth2.Token) (*BasicUserInfo, error) {
var data struct {
Id int
Username string
@@ -113,7 +113,7 @@ func (s *SocialGitlab) UserInfo(client *http.Client, token *oauth2.Token) (*Basi
groups := s.GetGroups(client)
role, grafanaAdmin := s.extractRoleAndAdmin(response.Body, groups)
role, grafanaAdmin := s.extractRoleAndAdmin(response.Body, groups, true)
if s.roleAttributeStrict && !role.IsValid() {
return nil, ErrInvalidBasicRole
}
@@ -129,7 +129,7 @@ func (s *SocialGitlab) UserInfo(client *http.Client, token *oauth2.Token) (*Basi
Login: data.Username,
Email: data.Email,
Groups: groups,
Role: string(role),
Role: role,
IsGrafanaAdmin: isGrafanaAdmin,
}
+159
View File
@@ -0,0 +1,159 @@
package social
import (
"net/http"
"net/http/httptest"
"strings"
"testing"
"github.com/grafana/grafana/pkg/services/org"
"github.com/stretchr/testify/require"
)
const (
apiURI = "/api/v4"
userURI = "/api/v4/user"
groupsURI = "/api/v4/groups"
gitlabAttrPath = `is_admin && 'GrafanaAdmin' || contains(groups[*], 'admins') && 'Admin' || contains(groups[*], 'editors') && 'Editor' || contains(groups[*], 'viewers') && 'Viewer'`
rootUserRespBody = `{"id":1,"username":"root","name":"Administrator","state":"active","email":"root@example.org","is_admin":true,"namespace_id":1}`
editorUserRespBody = `{"id":3,"username":"gitlab-editor","name":"Gitlab Editor","state":"active","email":"gitlab-editor@example.org","is_admin":false,"namespace_id":1}`
adminGroup = `{"id":4,"web_url":"http://grafana-gitlab.local/groups/admins","name":"Admins","path":"admins","project_creation_level":"developer","full_name":"Admins","full_path":"admins","created_at":"2022-09-13T19:38:04.891Z"}`
editorGroup = `{"id":5,"web_url":"http://grafana-gitlab.local/groups/editors","name":"Editors","path":"editors","project_creation_level":"developer","full_name":"Editors","full_path":"editors","created_at":"2022-09-13T19:38:15.074Z"}`
viewerGroup = `{"id":6,"web_url":"http://grafana-gitlab.local/groups/viewers","name":"Viewers","path":"viewers","project_creation_level":"developer","full_name":"Viewers","full_path":"viewers","created_at":"2022-09-13T19:38:25.777Z"}`
// serverAdminGroup = `{"id":7,"web_url":"http://grafana-gitlab.local/groups/serveradmins","name":"ServerAdmins","path":"serveradmins","project_creation_level":"developer","full_name":"ServerAdmins","full_path":"serveradmins","created_at":"2022-09-13T19:38:36.227Z"}`
)
func TestSocialGitlab_UserInfo(t *testing.T) {
provider := SocialGitlab{
SocialBase: &SocialBase{
log: newLogger("gitlab_oauth_test", "debug"),
},
}
type conf struct {
AllowAssignGrafanaAdmin bool
RoleAttributeStrict bool
AutoAssignOrgRole org.RoleType
}
tests := []struct {
Name string
Cfg conf
UserRespBody string
GroupsRespBody string
RoleAttributePath string
ExpectedLogin string
ExpectedEmail string
ExpectedRole org.RoleType
ExpectedGrafanaAdmin *bool
ExpectedError error
}{
{
Name: "Server Admin Allowed",
Cfg: conf{AllowAssignGrafanaAdmin: true},
UserRespBody: rootUserRespBody,
GroupsRespBody: "[" + strings.Join([]string{adminGroup, editorGroup, viewerGroup}, ",") + "]",
RoleAttributePath: gitlabAttrPath,
ExpectedLogin: "root",
ExpectedEmail: "root@example.org",
ExpectedRole: "Admin",
ExpectedGrafanaAdmin: trueBoolPtr(),
},
{ // Edge case, user in Viewer Group, Server Admin disabled but attribute path contains a condition for Server Admin => User has the Admin role
Name: "Server Admin Disabled",
Cfg: conf{AllowAssignGrafanaAdmin: false},
UserRespBody: rootUserRespBody,
GroupsRespBody: "[" + strings.Join([]string{viewerGroup}, ",") + "]",
RoleAttributePath: gitlabAttrPath,
ExpectedLogin: "root",
ExpectedEmail: "root@example.org",
ExpectedRole: "Admin",
},
{
Name: "Editor",
Cfg: conf{AllowAssignGrafanaAdmin: true},
UserRespBody: editorUserRespBody,
GroupsRespBody: "[" + strings.Join([]string{viewerGroup, editorGroup}, ",") + "]",
RoleAttributePath: gitlabAttrPath,
ExpectedLogin: "gitlab-editor",
ExpectedEmail: "gitlab-editor@example.org",
ExpectedRole: "Editor",
ExpectedGrafanaAdmin: falseBoolPtr(),
},
{ // Case that's going to change with Grafana 10
Name: "No fallback to default org role (will change in Grafana 10)",
Cfg: conf{AutoAssignOrgRole: org.RoleViewer},
UserRespBody: editorUserRespBody,
GroupsRespBody: "[" + strings.Join([]string{}, ",") + "]",
RoleAttributePath: gitlabAttrPath,
ExpectedLogin: "gitlab-editor",
ExpectedEmail: "gitlab-editor@example.org",
ExpectedRole: "",
},
{
Name: "Strict mode prevents fallback to default",
Cfg: conf{RoleAttributeStrict: true, AutoAssignOrgRole: org.RoleViewer},
UserRespBody: editorUserRespBody,
GroupsRespBody: "[" + strings.Join([]string{}, ",") + "]",
RoleAttributePath: gitlabAttrPath,
ExpectedError: ErrInvalidBasicRole,
},
{ // Edge case, no match, no strict mode and no fallback => User has an empty role
Name: "Fallback with no default will create a user with an empty role",
Cfg: conf{},
UserRespBody: editorUserRespBody,
GroupsRespBody: "[" + strings.Join([]string{}, ",") + "]",
RoleAttributePath: gitlabAttrPath,
ExpectedLogin: "gitlab-editor",
ExpectedEmail: "gitlab-editor@example.org",
ExpectedRole: "",
},
{ // Edge case, no attribute path with strict mode => User has an empty role
Name: "Strict mode with no attribute path",
Cfg: conf{RoleAttributeStrict: true, AutoAssignOrgRole: org.RoleViewer},
UserRespBody: editorUserRespBody,
GroupsRespBody: "[" + strings.Join([]string{editorGroup}, ",") + "]",
RoleAttributePath: "",
ExpectedError: ErrInvalidBasicRole,
},
}
for _, test := range tests {
provider.roleAttributePath = test.RoleAttributePath
provider.allowAssignGrafanaAdmin = test.Cfg.AllowAssignGrafanaAdmin
provider.autoAssignOrgRole = string(test.Cfg.AutoAssignOrgRole)
provider.roleAttributeStrict = test.Cfg.RoleAttributeStrict
t.Run(test.Name, func(t *testing.T) {
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusOK)
w.Header().Set("Content-Type", "application/json")
switch r.RequestURI {
case userURI:
_, err := w.Write([]byte(test.UserRespBody))
require.NoError(t, err)
case groupsURI:
_, err := w.Write([]byte(test.GroupsRespBody))
require.NoError(t, err)
default:
w.WriteHeader(http.StatusNotFound)
}
}))
provider.apiUrl = ts.URL + apiURI
actualResult, err := provider.UserInfo(ts.Client(), nil)
if test.ExpectedError != nil {
require.Equal(t, err, test.ExpectedError)
return
}
require.NoError(t, err)
require.Equal(t, test.ExpectedEmail, actualResult.Email)
require.Equal(t, test.ExpectedLogin, actualResult.Login)
require.Equal(t, test.ExpectedRole, actualResult.Role)
require.Equal(t, test.ExpectedGrafanaAdmin, actualResult.IsGrafanaAdmin)
})
}
}
+3 -2
View File
@@ -6,6 +6,7 @@ import (
"net/http"
"github.com/grafana/grafana/pkg/models"
"github.com/grafana/grafana/pkg/services/org"
"golang.org/x/oauth2"
)
@@ -44,7 +45,7 @@ func (s *SocialGrafanaCom) IsOrganizationMember(organizations []OrgRecord) bool
return false
}
func (s *SocialGrafanaCom) UserInfo(client *http.Client, token *oauth2.Token) (*BasicUserInfo, error) {
func (s *SocialGrafanaCom) UserInfo(client *http.Client, _ *oauth2.Token) (*BasicUserInfo, error) {
var data struct {
Id int `json:"id"`
Name string `json:"name"`
@@ -69,7 +70,7 @@ func (s *SocialGrafanaCom) UserInfo(client *http.Client, token *oauth2.Token) (*
Name: data.Name,
Login: data.Login,
Email: data.Email,
Role: data.Role,
Role: org.RoleType(data.Role),
}
if !s.IsOrganizationMember(data.Orgs) {
+2 -2
View File
@@ -80,7 +80,7 @@ func (s *SocialOkta) UserInfo(client *http.Client, token *oauth2.Token) (*BasicU
return nil, errMissingGroupMembership
}
role, grafanaAdmin := s.extractRoleAndAdmin(data.rawJSON, groups)
role, grafanaAdmin := s.extractRoleAndAdmin(data.rawJSON, groups, true)
if s.roleAttributeStrict && !role.IsValid() {
return nil, ErrInvalidBasicRole
}
@@ -95,7 +95,7 @@ func (s *SocialOkta) UserInfo(client *http.Client, token *oauth2.Token) (*BasicU
Name: claims.Name,
Email: email,
Login: email,
Role: string(role),
Role: role,
IsGrafanaAdmin: isGrafanaAdmin,
Groups: groups,
}, nil
+26 -8
View File
@@ -228,7 +228,7 @@ type BasicUserInfo struct {
Name string
Email string
Login string
Role string
Role org.RoleType
IsGrafanaAdmin *bool // nil will avoid overriding user's set server admin setting
Groups []string
}
@@ -312,13 +312,9 @@ type groupStruct struct {
Groups []string `json:"groups"`
}
func (s *SocialBase) extractRoleAndAdmin(rawJSON []byte, groups []string) (org.RoleType, bool) {
func (s *SocialBase) extractRoleAndAdmin(rawJSON []byte, groups []string, legacy bool) (org.RoleType, bool) {
if s.roleAttributePath == "" {
if s.autoAssignOrgRole != "" {
return org.RoleType(s.autoAssignOrgRole), false
}
return "", false
return s.defaultRole(legacy), false
}
role, err := s.searchJSONForStringAttr(s.roleAttributePath, rawJSON)
@@ -333,7 +329,29 @@ func (s *SocialBase) extractRoleAndAdmin(rawJSON []byte, groups []string) (org.R
}
}
return "", false
return s.defaultRole(legacy), false
}
// defaultRole returns the default role for the user based on the autoAssignOrgRole setting
// if legacy is enabled "" is returned indicating the previous role assignment is used.
func (s *SocialBase) defaultRole(legacy bool) org.RoleType {
if s.roleAttributeStrict {
s.log.Debug("RoleAttributeStrict is set, returning no role.")
return ""
}
if s.autoAssignOrgRole != "" && !legacy {
s.log.Debug("No role found, returning default.")
return org.RoleType(s.autoAssignOrgRole)
}
if legacy {
s.log.Warn("No valid role found. Skipping role sync. " +
"In Grafana 10, this will result in the user being assigned the default role and overriding manual assignment. " +
"If role sync is not desired, set oauth_skip_org_role_update_sync to false")
}
return ""
}
// match grafana admin role and translate to org role and bool.