[GitHub] [servicecomb-service-center] aseTo2016 commented on a change in pull request #652: able to change password

[GitBox <gi...@apache.org>]

aseTo2016 commented on a change in pull request #652:
URL: https://github.com/apache/servicecomb-service-center/pull/652#discussion_r446638138



##########
File path: server/rest/controller/v4/auth_resource.go
##########
@@ -37,9 +37,55 @@ type AuthResource struct {
 func (r *AuthResource) URLPatterns() []rest.Route {
 	return []rest.Route{
 		{http.MethodPost, "/v4/token", r.Login},
+		{http.MethodPut, "/v4/account-password", r.ChangePassword},
 	}
 }
+func (r *AuthResource) ChangePassword(w http.ResponseWriter, req *http.Request) {
+	body, err := ioutil.ReadAll(req.Body)
+	if err != nil {
+		log.Error("read body err", err)
+		controller.WriteError(w, scerror.ErrInternal, err.Error())
+		return
+	}
+	a := &model.Account{}
+	if err = json.Unmarshal(body, a); err != nil {
+		log.Error("json err", err)
+		controller.WriteError(w, scerror.ErrInvalidParams, err.Error())
+		return
+	}
+	if a.Password == "" {
+		controller.WriteError(w, scerror.ErrInvalidParams, "new password is empty")
+		return
+	}
+	claims := req.Context().Value("accountInfo")
+	m, ok := claims.(map[string]interface{})
+	if !ok {
+		log.Errorf(err, "claims convert failed: %T", claims)
+		controller.WriteError(w, scerror.ErrInvalidParams, "wrong account info")
+		return
+	}
+	accountNameI := m[rbac.ClaimsUser]
+	changer, ok := accountNameI.(string)
+	if !ok {
+		log.Error("can not convert claim", nil)
+		controller.WriteError(w, scerror.ErrInvalidParams, err.Error())

Review comment:
       err为nil，打印会panic

##########
File path: server/handler/auth/auth.go
##########
@@ -63,15 +64,22 @@ func (h *Handler) Handle(i *chain.Invocation) {
 	}
 	to := s[1]
 	//TODO rbac
-	_, err := authr.Authenticate(i.Context(), to)
-	if err == nil {
-		log.Info("user access")
-		i.Next()
+	claims, err := authr.Authenticate(i.Context(), to)
+	if err != nil {
+		log.Errorf(err, "authenticate request failed, %s %s", req.Method, req.RequestURI)
+		controller.WriteError(w, scerr.ErrUnauthorized, err.Error())
+		i.Fail(nil)
 		return
 	}
-	log.Errorf(err, "authenticate request failed, %s %s", req.Method, req.RequestURI)
-	controller.WriteError(w, scerr.ErrUnauthorized, err.Error())
-	i.Fail(nil)
+	log.Info("user access")
+	req2 := req.WithContext(context.WithValue(req.Context(), "accountInfo", claims))
+
+	*req = *req2
+	log.Infof("%s", req.Context())

Review comment:
       context是否需要打印？是否有敏感信息

##########
File path: server/service/rbac/rbac.go
##########
@@ -49,43 +55,52 @@ func Init() {
 	if err != nil {
 		log.Fatal("can not enable auth module", err)
 	}
-	admin := archaius.GetString(InitRoot, "")
-	if admin == "" {
-		log.Fatal("can not enable rbac, root is empty", nil)
-		return
-	}
-	accountExist, err := dao.AccountExist(context.Background(), admin)
+	accountExist, err := dao.AccountExist(context.Background(), RootName)
 	if err != nil {
 		log.Fatal("can not enable auth module", err)
 	}
 	if !accountExist {
-		initFirstTime(admin)
+		initFirstTime(RootName)
 	}
-	overrideSecretKey()
+	readPrivateKey()
 	readPublicKey()
 	log.Info("rbac is enabled")
 }
 
+//readPublicKey read key to memory
+func readPrivateKey() {
+	pf := beego.AppConfig.String("rbac_rsa_private_key_file")
+	// 打开文件
+	data, err := ioutil.ReadFile(pf)
+	if err != nil {
+		log.Fatal("can not read private key", err)
+		return
+	}
+	archaius.Set("rbac_private_key", string(data))
+	log.Info("read private key success %s")

Review comment:
       %s后面没有值

##########
File path: server/service/rbac/rbac.go
##########
@@ -49,43 +55,52 @@ func Init() {
 	if err != nil {
 		log.Fatal("can not enable auth module", err)
 	}
-	admin := archaius.GetString(InitRoot, "")
-	if admin == "" {
-		log.Fatal("can not enable rbac, root is empty", nil)
-		return
-	}
-	accountExist, err := dao.AccountExist(context.Background(), admin)
+	accountExist, err := dao.AccountExist(context.Background(), RootName)
 	if err != nil {
 		log.Fatal("can not enable auth module", err)
 	}
 	if !accountExist {
-		initFirstTime(admin)
+		initFirstTime(RootName)
 	}
-	overrideSecretKey()
+	readPrivateKey()
 	readPublicKey()
 	log.Info("rbac is enabled")
 }
 
+//readPublicKey read key to memory
+func readPrivateKey() {
+	pf := beego.AppConfig.String("rbac_rsa_private_key_file")
+	// 打开文件
+	data, err := ioutil.ReadFile(pf)
+	if err != nil {
+		log.Fatal("can not read private key", err)
+		return
+	}
+	archaius.Set("rbac_private_key", string(data))
+	log.Info("read private key success %s")
+}
+
 //readPublicKey read key to memory
 func readPublicKey() {
-	pf := beego.AppConfig.String("rbac_rsa_pub_key_file")
+	pf := beego.AppConfig.String("rbac_rsa_public_key_file")
 	// 打开文件
-	fp, err := os.Open(pf)
+	f, err := os.Open(pf)
 	if err != nil {
 		log.Fatal("can not find public key", err)
 		return
 	}
-	defer fp.Close()
+	defer f.Close()
 	buf := make([]byte, 1024)
 	for {
 		// 循环读取文件
-		_, err := fp.Read(buf)
+		_, err := f.Read(buf)

Review comment:
       建议使用ioutil.ReadFile()

##########
File path: server/service/rbac/rbac.go
##########
@@ -49,43 +55,52 @@ func Init() {
 	if err != nil {
 		log.Fatal("can not enable auth module", err)
 	}
-	admin := archaius.GetString(InitRoot, "")
-	if admin == "" {
-		log.Fatal("can not enable rbac, root is empty", nil)
-		return
-	}
-	accountExist, err := dao.AccountExist(context.Background(), admin)
+	accountExist, err := dao.AccountExist(context.Background(), RootName)
 	if err != nil {
 		log.Fatal("can not enable auth module", err)
 	}
 	if !accountExist {
-		initFirstTime(admin)
+		initFirstTime(RootName)
 	}
-	overrideSecretKey()
+	readPrivateKey()
 	readPublicKey()
 	log.Info("rbac is enabled")
 }
 
+//readPublicKey read key to memory
+func readPrivateKey() {
+	pf := beego.AppConfig.String("rbac_rsa_private_key_file")
+	// 打开文件
+	data, err := ioutil.ReadFile(pf)
+	if err != nil {
+		log.Fatal("can not read private key", err)
+		return
+	}
+	archaius.Set("rbac_private_key", string(data))
+	log.Info("read private key success %s")
+}
+
 //readPublicKey read key to memory
 func readPublicKey() {
-	pf := beego.AppConfig.String("rbac_rsa_pub_key_file")
+	pf := beego.AppConfig.String("rbac_rsa_public_key_file")

Review comment:
       rbac_rsa_public_key_file有两个地方引用，建议写成const

##########
File path: server/service/rbac/rbac.go
##########
@@ -158,3 +158,52 @@ func GetPrivateKey(ctx context.Context) (*rsa.PrivateKey, error) {
 	}
 	return p, nil
 }
+
+func ChangePassword(ctx context.Context, changerRole, changerName string, a *model.Account) error {
+	if a.Name != "" {
+		if changerRole != model.RoleAdmin { //need to check password mismatch. but admin role can change any user password without supply current password
+			log.Error("can not change other account pwd", nil)
+			return ErrInputChangeAccount
+		} else {

Review comment:
       可以删除else，写成return changePasswordForcibly(ctx, a.Name, a.Password)

##########
File path: server/rest/controller/v4/auth_resource.go
##########
@@ -37,9 +37,55 @@ type AuthResource struct {
 func (r *AuthResource) URLPatterns() []rest.Route {
 	return []rest.Route{
 		{http.MethodPost, "/v4/token", r.Login},
+		{http.MethodPut, "/v4/account-password", r.ChangePassword},
 	}
 }
+func (r *AuthResource) ChangePassword(w http.ResponseWriter, req *http.Request) {
+	body, err := ioutil.ReadAll(req.Body)
+	if err != nil {
+		log.Error("read body err", err)
+		controller.WriteError(w, scerror.ErrInternal, err.Error())
+		return
+	}
+	a := &model.Account{}
+	if err = json.Unmarshal(body, a); err != nil {
+		log.Error("json err", err)
+		controller.WriteError(w, scerror.ErrInvalidParams, err.Error())
+		return
+	}
+	if a.Password == "" {
+		controller.WriteError(w, scerror.ErrInvalidParams, "new password is empty")
+		return
+	}
+	claims := req.Context().Value("accountInfo")
+	m, ok := claims.(map[string]interface{})
+	if !ok {
+		log.Errorf(err, "claims convert failed: %T", claims)
+		controller.WriteError(w, scerror.ErrInvalidParams, "wrong account info")
+		return
+	}
+	accountNameI := m[rbac.ClaimsUser]
+	changer, ok := accountNameI.(string)
+	if !ok {
+		log.Error("can not convert claim", nil)
+		controller.WriteError(w, scerror.ErrInvalidParams, err.Error())
+		return
+	}
+	roleI := m[rbac.ClaimsRole]
+	role, ok := roleI.(string)
+	if !ok {
+		log.Error("can not convert claim", nil)
+		controller.WriteError(w, scerror.ErrInvalidParams, err.Error())

Review comment:
       同上




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org