Oidc: Improve package structure and add tests #782

internal/api/auth.go

@@ -1,12 +1,13 @@
 package api
 
 import (
+	"net/http"
+
 	"github.com/gin-gonic/gin"
 	"github.com/photoprism/photoprism/internal/entity"
 	"github.com/photoprism/photoprism/internal/oidc"
 	"github.com/photoprism/photoprism/internal/service"
 	"github.com/photoprism/photoprism/internal/session"
-	"net/http"
 )
 
 // GET /api/v1/auth/
@@ -22,12 +23,12 @@ func AuthEndpoints(router *gin.RouterGroup) {
 	}
 
 	router.GET("/auth/external", func(c *gin.Context) {
-		openIdConnect, _ := service.Oidc()
-		//if err := openIdConnect.IsAvailable(); err != nil {
-		//	c.Error(err)
-		//	callbackError(c, err.Error(), http.StatusInternalServerError)
-		//	return
-		//}
+		openIdConnect, err := service.Oidc()
+		if err != nil {
+			c.Error(err)
+			callbackError(c, err.Error(), http.StatusInternalServerError)
+			return
+		}
 
 		handle := openIdConnect.AuthUrlHandler()
 		handle(c.Writer, c.Request)

internal/entity/user_test.go

@@ -595,5 +595,32 @@ func TestCreateOrUpdateExternalUser(t *testing.T) {
 		assert.Nil(t, err2)
 		assert.Equal(t, "admin-02", user2.UserName)
 	})
+	t.Run("CreateOrUpdateExternalUser - update with roles", func(t *testing.T) {
+		u.PrimaryEmail = "gopher-new@example.com"
+		u.RoleAdmin = true
+		user, err := CreateOrUpdateExternalUser(u, true)
+		if err != nil {
+			t.Error(err)
+		}
+		assert.Equal(t, uid, user.UserUID)
+		assert.True(t, user.RoleAdmin)
+		u.RoleAdmin = false
+		user, err = CreateOrUpdateExternalUser(u, true)
+		if err != nil {
+			t.Error(err)
+		}
+		assert.Equal(t, uid, user.UserUID)
+		assert.False(t, user.RoleAdmin)
+	})
+	t.Run("CreateOrUpdateExternalUser - update with roles invalid", func(t *testing.T) {
+		u.PrimaryEmail = "gopher-new@example.com"
+		u.RoleAdmin = true
+		user, err := CreateOrUpdateExternalUser(u, false)
+		if err != nil {
+			t.Error(err)
+		}
+		assert.Equal(t, uid, user.UserUID)
+		assert.False(t, user.RoleAdmin)
+	})
 
 }

internal/oidc/client.go

@@ -154,54 +154,3 @@ func (c *Client) IsAvailable() error {
 	}
 	return nil
 }
-
-func UsernameFromUserInfo(userinfo oidc.UserInfo) (uname string) {
-	if len(userinfo.GetPreferredUsername()) >= 4 {
-		uname = userinfo.GetPreferredUsername()
-	} else if len(userinfo.GetNickname()) >= 4 {
-		uname = userinfo.GetNickname()
-	} else if len(userinfo.GetName()) >= 4 {
-		uname = strings.ReplaceAll(strings.ToLower(userinfo.GetName()), " ", "-")
-	} else if len(userinfo.GetEmail()) >= 4 {
-		uname = userinfo.GetEmail()
-	} else {
-		log.Error("oidc: no username found")
-	}
-	return uname
-}
-
-// HasRoleAdmin searches UserInfo claims for admin role.
-// Returns true if role is present or false if claim was found but no role in there.
-// Error will be returned if the role claim is not delivered at all.
-func HasRoleAdmin(userinfo oidc.UserInfo) (bool, error) {
-	claim := userinfo.GetClaim(OidcRoleClaim)
-	return claimContainsProp(claim, OidcAdminRole)
-}
-
-func claimContainsProp(claim interface{}, property string) (bool, error) {
-	switch t := claim.(type) {
-	case nil:
-		return false, errors.New("oidc: claim not found")
-	case []interface{}:
-		for _, value := range t {
-			res, err := claimContainsProp(value, property)
-			if err != nil {
-				return false, err
-			}
-			if res {
-				return res, nil
-			}
-		}
-		return false, nil
-	case interface{}:
-		if value, ok := t.(string); ok {
-			return value == property, nil
-		} else {
-			return false, errors.New("oidc: unexpected type")
-		}
-	//case string:
-	//	return t == property, nil
-	default:
-		return false, errors.New("oidc: unexpected type")
-	}
-}

internal/oidc/helper.go

@@ -0,0 +1,63 @@
+package oidc
+
+import (
+	"errors"
+	"strings"
+)
+
+type UserInfo interface {
+	GetPreferredUsername() string
+	GetNickname() string
+	GetName() string
+	GetEmail() string
+	GetClaim(key string) interface{}
+}
+
+func UsernameFromUserInfo(userinfo UserInfo) (username string) {
+	if len(userinfo.GetPreferredUsername()) >= 4 {
+		username = userinfo.GetPreferredUsername()
+	} else if len(userinfo.GetNickname()) >= 4 {
+		username = userinfo.GetNickname()
+	} else if len(userinfo.GetName()) >= 4 {
+		username = strings.ReplaceAll(strings.ToLower(userinfo.GetName()), " ", "-")
+	} else if len(userinfo.GetEmail()) >= 4 {
+		username = userinfo.GetEmail()
+	} else {
+		log.Error("oidc: no username found")
+	}
+	return username
+}
+
+// HasRoleAdmin searches UserInfo claims for admin role.
+// Returns true if role is present or false if claim was found but no role in there.
+// Error will be returned if the role claim is not delivered at all.
+func HasRoleAdmin(userinfo UserInfo) (bool, error) {
+	claim := userinfo.GetClaim(OidcRoleClaim)
+	return claimContainsProp(claim, OidcAdminRole)
+}
+
+func claimContainsProp(claim interface{}, property string) (bool, error) {
+	switch t := claim.(type) {
+	case nil:
+		return false, errors.New("oidc: claim not found")
+	case []interface{}:
+		for _, value := range t {
+			res, err := claimContainsProp(value, property)
+			if err != nil {
+				return false, err
+			}
+			if res {
+				return res, nil
+			}
+		}
+		return false, nil
+	case interface{}:
+		if value, ok := t.(string); ok {
+			return value == property, nil
+		} else {
+			return false, errors.New("oidc: unexpected type")
+		}
+	default:
+		return false, errors.New("oidc: unexpected type")
+	}
+}

internal/oidc/helper_test.go

@@ -0,0 +1,91 @@
+package oidc
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestUsernameFromUserInfo(t *testing.T) {
+	t.Run("PreferredUsername", func(t *testing.T) {
+		u := &userinfo{PreferredUsername: "testfest"}
+		assert.Equal(t, "testfest", UsernameFromUserInfo(u))
+	})
+	t.Run("PreferredUsername too short", func(t *testing.T) {
+		u := &userinfo{PreferredUsername: "tes"}
+		assert.Equal(t, "", UsernameFromUserInfo(u))
+	})
+	t.Run("EMail", func(t *testing.T) {
+		u := &userinfo{Nickname: "tes", Email: "hello@world.com"}
+		assert.Equal(t, "hello@world.com", UsernameFromUserInfo(u))
+	})
+	t.Run("Nickname", func(t *testing.T) {
+		u := &userinfo{Nickname: "testofesto", Email: "hel"}
+		assert.Equal(t, "testofesto", UsernameFromUserInfo(u))
+	})
+}
+
+func TestHasRoleAdmin(t *testing.T) {
+	t.Run("true case", func(t *testing.T) {
+		u := &userinfo{Claim: []interface{}{
+			"admin",
+			"photoprism_admin",
+			"photoprism",
+			"random",
+		}}
+		hasRoleAdmin, err := HasRoleAdmin(u)
+		assert.True(t, hasRoleAdmin)
+		assert.Nil(t, err)
+	})
+	t.Run("false case", func(t *testing.T) {
+		u := &userinfo{Claim: []interface{}{
+			"admin",
+			"photoprismo_admin",
+			"photoprism",
+			"random",
+		}}
+		hasRoleAdmin, err := HasRoleAdmin(u)
+		assert.False(t, hasRoleAdmin)
+		assert.Nil(t, err)
+	})
+	t.Run("false case 2", func(t *testing.T) {
+		u := &userinfo{Claim: []interface{}{}}
+		hasRoleAdmin, err := HasRoleAdmin(u)
+		assert.False(t, hasRoleAdmin)
+		assert.Nil(t, err)
+	})
+	t.Run("error case", func(t *testing.T) {
+		u := &userinfo{Claim: nil}
+		hasRoleAdmin, err := HasRoleAdmin(u)
+		assert.False(t, hasRoleAdmin)
+		assert.Error(t, err)
+	})
+}
+
+type userinfo struct {
+	PreferredUsername string
+	Nickname          string
+	Name              string
+	Email             string
+	Claim             interface{}
+}
+
+func (u *userinfo) GetPreferredUsername() string {
+	return u.PreferredUsername
+}
+
+func (u *userinfo) GetNickname() string {
+	return u.Nickname
+}
+
+func (u *userinfo) GetName() string {
+	return u.Name
+}
+
+func (u *userinfo) GetEmail() string {
+	return u.Email
+}
+
+func (u *userinfo) GetClaim(key string) interface{} {
+	return u.Claim
+}