Merge pull request #8662 from stonezdj/email_sec2

Set default email to null if not provided

make/migrations/postgresql/0001_initial_schema.up.sql

@@ -56,9 +56,9 @@ $$;
 
 CREATE TRIGGER harbor_user_update_time_at_modtime BEFORE UPDATE ON harbor_user FOR EACH ROW EXECUTE PROCEDURE update_update_time_at_column();
 
-insert into harbor_user (username, email, password, realname, comment, deleted, sysadmin_flag, creation_time, update_time) values 
-('admin', 'admin@example.com', '', 'system admin', 'admin user',false, true, NOW(), NOW()),
-('anonymous', 'anonymous@example.com', '', 'anonymous user', 'anonymous user', true, false, NOW(), NOW());
+insert into harbor_user (username, password, realname, comment, deleted, sysadmin_flag, creation_time, update_time) values
+('admin', '', 'system admin', 'admin user',false, true, NOW(), NOW()),
+('anonymous', '', 'anonymous user', 'anonymous user', true, false, NOW(), NOW());
 
 create table project (
  project_id SERIAL PRIMARY KEY NOT NULL,

src/common/dao/user.go

@@ -234,6 +234,14 @@ func OnBoardUser(u *models.User) error {
 	}
 	if created {
 		u.UserID = int(id)
+		// current orm framework doesn't support to fetch a pointer or sql.NullString with QueryRow
+		// https://github.com/astaxie/beego/issues/3767
+		if len(u.Email) == 0 {
+			_, err = o.Raw("update harbor_user set email = null where user_id = ? ", id).Exec()
+			if err != nil {
+				return err
+			}
+		}
 	} else {
 		existing, err := GetUser(*u)
 		if err != nil {

src/common/dao/user_test.go

@@ -90,3 +90,23 @@ func TestOnBoardUser(t *testing.T) {
 	assert.True(u.UserID == id)
 	CleanUser(int64(id))
 }
+func TestOnBoardUser_EmptyEmail(t *testing.T) {
+	assert := assert.New(t)
+	u := &models.User{
+		Username: "empty_email",
+		Password: "password1",
+		Realname: "empty_email",
+	}
+	err := OnBoardUser(u)
+	assert.Nil(err)
+	id := u.UserID
+	assert.True(id > 0)
+	err = OnBoardUser(u)
+	assert.Nil(err)
+	assert.True(u.UserID == id)
+	assert.Equal("", u.Email)
+
+	user, err := GetUser(models.User{Username: "empty_email"})
+	assert.Equal("", user.Email)
+	CleanUser(int64(id))
+}

src/core/auth/authproxy/auth.go

@@ -211,8 +211,6 @@ func (a *Auth) fillInModel(u *models.User) error {
 	u.Comment = userEntryComment
 	if strings.Contains(u.Username, "@") {
 		u.Email = u.Username
-	} else {
-		u.Email = fmt.Sprintf("%s@placeholder.com", u.Username)
 	}
 	return nil
 }

src/core/auth/authproxy/auth_test.go

@@ -154,7 +154,7 @@ func TestAuth_PostAuthenticate(t *testing.T) {
 			},
 			expect: models.User{
 				Username: "jt",
-				Email:    "jt@placeholder.com",
+				Email:    "",
 				Realname: "jt",
 				Password: pwd,
 				Comment:  userEntryComment,

src/core/auth/ldap/ldap.go

@@ -124,8 +124,6 @@ func (l *Auth) OnBoardUser(u *models.User) error {
 	if u.Email == "" {
 		if strings.Contains(u.Username, "@") {
 			u.Email = u.Username
-		} else {
-			u.Email = u.Username + "@placeholder.com"
 		}
 	}
 	u.Password = "12345678AbC" // Password is not kept in local db

src/core/auth/ldap/ldap_test.go

@@ -224,7 +224,7 @@ func TestOnBoardUser_02(t *testing.T) {
 		t.Errorf("Failed to onboard user")
 	}
 
-	assert.Equal(t, "sample02@placeholder.com", user.Email)
+	assert.Equal(t, "", user.Email)
 	dao.CleanUser(int64(user.UserID))
 }
 

src/core/auth/uaa/uaa.go

@@ -77,9 +77,8 @@ func fillEmailRealName(user *models.User) {
 	if len(user.Realname) == 0 {
 		user.Realname = user.Username
 	}
-	if len(user.Email) == 0 {
-		// TODO: handle the case when user.Username itself is an email address.
-		user.Email = user.Username + "@uaa.placeholder"
+	if len(user.Email) == 0 && strings.Contains(user.Username, "@") {
+		user.Email = user.Username
 	}
 }
 

src/core/auth/uaa/uaa_test.go

@@ -110,7 +110,7 @@ func TestOnBoardUser(t *testing.T) {
 	user, _ := dao.GetUser(models.User{Username: "test"})
 	assert.Equal("test", user.Realname)
 	assert.Equal("test", user.Username)
-	assert.Equal("test@uaa.placeholder", user.Email)
+	assert.Equal("", user.Email)
 	err3 := dao.ClearTable(models.UserTable)
 	assert.Nil(err3)
 }
@@ -128,7 +128,7 @@ func TestPostAuthenticate(t *testing.T) {
 	}
 	assert.Nil(err)
 	user, _ := dao.GetUser(models.User{Username: "test"})
-	assert.Equal("test@uaa.placeholder", user.Email)
+	assert.Equal("", user.Email)
 	um2.Email = "newEmail@new.com"
 	um2.Realname = "newName"
 	err2 := auth.PostAuthenticate(um2)
@@ -145,7 +145,7 @@ func TestPostAuthenticate(t *testing.T) {
 	assert.Nil(err3)
 	user3, _ := dao.GetUser(models.User{Username: "test"})
 	assert.Equal(user3.UserID, um3.UserID)
-	assert.Equal("test@uaa.placeholder", user3.Email)
+	assert.Equal("", user3.Email)
 	assert.Equal("test", user3.Realname)
 	err4 := dao.ClearTable(models.UserTable)
 	assert.Nil(err4)

src/core/controllers/oidc.go

@@ -17,6 +17,9 @@ package controllers
 import (
 	"encoding/json"
 	"fmt"
+	"net/http"
+	"strings"
+
 	"github.com/goharbor/harbor/src/common"
 	"github.com/goharbor/harbor/src/common/dao"
 	"github.com/goharbor/harbor/src/common/models"
@@ -26,8 +29,6 @@ import (
 	"github.com/goharbor/harbor/src/core/api"
 	"github.com/goharbor/harbor/src/core/config"
 	"github.com/pkg/errors"
-	"net/http"
-	"strings"
 )
 
 const tokenKey = "oidc_token"
@@ -189,9 +190,6 @@ func (oc *OIDCController) Onboard() {
 	}
 
 	email := d.Email
-	if email == "" {
-		email = utils.GenerateRandomString() + "@placeholder.com"
-	}
 	user := models.User{
 		Username:     username,
 		Realname:     d.Username,