fix: update admin IDP APIs to be more RESTful (#15896)

cmd/admin-handlers-idp-config.go

@@ -28,21 +28,16 @@ import (
 
 	"github.com/gorilla/mux"
 	"github.com/minio/madmin-go"
+	"github.com/minio/minio-go/v7/pkg/set"
 	"github.com/minio/minio/internal/config"
+	cfgldap "github.com/minio/minio/internal/config/identity/ldap"
 	"github.com/minio/minio/internal/config/identity/openid"
 	"github.com/minio/minio/internal/logger"
 	iampolicy "github.com/minio/pkg/iam/policy"
 	"github.com/minio/pkg/ldap"
 )
 
-// SetIdentityProviderCfg:
+func (a adminAPIHandlers) addOrUpdateIDPHandler(ctx context.Context, w http.ResponseWriter, r *http.Request, isUpdate bool) {
-//
-// PUT <admin-prefix>/id-cfg?type=openid&name=dex1
-func (a adminAPIHandlers) SetIdentityProviderCfg(w http.ResponseWriter, r *http.Request) {
-	ctx := newContext(r, w, "SetIdentityCfg")
-
-	defer logger.AuditLog(ctx, w, r, mustGetClaimsFromToken(r))
-
 	objectAPI, cred := validateAdminReq(ctx, w, r, iampolicy.ConfigUpdateAdminAction)
 	if objectAPI == nil {
 		return
@@ -54,6 +49,14 @@ func (a adminAPIHandlers) SetIdentityProviderCfg(w http.ResponseWriter, r *http.
 		return
 	}
 
+	// Ensure body content type is opaque to ensure that request body has not
+	// been interpreted as form data.
+	contentType := r.Header.Get("Content-Type")
+	if contentType != "application/octet-stream" {
+		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrBadRequest), r.URL)
+		return
+	}
+
 	password := cred.SecretKey
 	reqBytes, err := madmin.DecryptData(password, io.LimitReader(r.Body, r.ContentLength))
 	if err != nil {
@@ -68,43 +71,42 @@ func (a adminAPIHandlers) SetIdentityProviderCfg(w http.ResponseWriter, r *http.
 		return
 	}
 
-	var cfgDataBuilder strings.Builder
+	var subSys string
 	switch idpCfgType {
 	case madmin.OpenidIDPCfg:
-		fmt.Fprintf(&cfgDataBuilder, "identity_openid")
+		subSys = madmin.IdentityOpenIDSubSys
 	case madmin.LDAPIDPCfg:
-		fmt.Fprintf(&cfgDataBuilder, "identity_ldap")
+		subSys = madmin.IdentityLDAPSubSys
 	}
 
-	// Ensure body content type is opaque.
-	contentType := r.Header.Get("Content-Type")
-	if contentType != "application/octet-stream" {
-		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrBadRequest), r.URL)
-		return
-	}
-
-	// Subsystem configuration name could be empty.
 	cfgName := mux.Vars(r)["name"]
+	cfgTarget := madmin.Default
 	if cfgName != "" {
-		if idpCfgType == madmin.LDAPIDPCfg {
+		cfgTarget = cfgName
-			// LDAP does not support multiple configurations. So this must be
+		if idpCfgType == madmin.LDAPIDPCfg && cfgName != madmin.Default {
-			// empty.
+			// LDAP does not support multiple configurations. So cfgName must be
+			// empty or `madmin.Default`.
 			writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrBadRequest), r.URL)
 			return
 		}
-
-		fmt.Fprintf(&cfgDataBuilder, "%s%s", config.SubSystemSeparator, cfgName)
 	}
 
-	fmt.Fprintf(&cfgDataBuilder, "%s%s", config.KvSpaceSeparator, string(reqBytes))
+	// Check that this is a valid Create vs Update API call.
-
+	s := globalServerConfig.Clone()
-	cfgData := cfgDataBuilder.String()
+	if apiErrCode := handleCreateUpdateValidation(s, subSys, cfgTarget, isUpdate); apiErrCode != ErrNone {
-	subSys, _, _, err := config.GetSubSys(cfgData)
+		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(apiErrCode), r.URL)
-	if err != nil {
-		writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
 		return
 	}
 
+	cfgData := ""
+	{
+		tgtSuffix := ""
+		if cfgTarget != madmin.Default {
+			tgtSuffix = config.SubSystemSeparator + cfgTarget
+		}
+		cfgData = subSys + tgtSuffix + config.KvSpaceSeparator + string(reqBytes)
+	}
+
 	cfg, err := readServerConfig(ctx, objectAPI)
 	if err != nil {
 		writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
@@ -153,79 +155,86 @@ func (a adminAPIHandlers) SetIdentityProviderCfg(w http.ResponseWriter, r *http.
 	writeSuccessResponseHeadersOnly(w)
 }
 
-// GetIdentityProviderCfg:
+func handleCreateUpdateValidation(s config.Config, subSys, cfgTarget string, isUpdate bool) APIErrorCode {
-//
+	if cfgTarget != madmin.Default {
-// GET <admin-prefix>/id-cfg?type=openid&name=dex_test
+		// This cannot give an error at this point.
-//
+		subSysTargets, _ := s.GetAvailableTargets(subSys)
-// GetIdentityProviderCfg returns a list of configured IDPs on the server if
+		subSysTargetsSet := set.CreateStringSet(subSysTargets...)
-// name is empty. If name is non-empty, returns the configuration details for
+		if isUpdate && !subSysTargetsSet.Contains(cfgTarget) {
-// the IDP of the given type and configuration name. The configuration name for
+			return ErrAdminConfigIDPCfgNameDoesNotExist
-// the default ("un-named") configuration target is `_`.
+		}
-func (a adminAPIHandlers) GetIdentityProviderCfg(w http.ResponseWriter, r *http.Request) {
+		if !isUpdate && subSysTargetsSet.Contains(cfgTarget) {
-	ctx := newContext(r, w, "GetIdentityProviderCfg")
+			return ErrAdminConfigIDPCfgNameAlreadyExists
+		}
 
+		return ErrNone
+	}
+
+	// For the default configuration name, since it will always be an available
+	// target, we need to check if a configuration value has been set previously
+	// to figure out if this is a valid create or update API call.
+
+	// This cannot really error (FIXME: improve the type for GetConfigInfo)
+	var cfgInfos []madmin.IDPCfgInfo
+	switch subSys {
+	case madmin.IdentityOpenIDSubSys:
+		cfgInfos, _ = globalOpenIDConfig.GetConfigInfo(s, cfgTarget)
+	case madmin.IdentityLDAPSubSys:
+		cfgInfos, _ = globalLDAPConfig.GetConfigInfo(s, cfgTarget)
+	}
+
+	if len(cfgInfos) > 0 && !isUpdate {
+		return ErrAdminConfigIDPCfgNameAlreadyExists
+	}
+	if len(cfgInfos) == 0 && isUpdate {
+		return ErrAdminConfigIDPCfgNameDoesNotExist
+	}
+	return ErrNone
+}
+
+// AddIdentityProviderCfg: adds a new IDP config for openid/ldap.
+//
+// PUT <admin-prefix>/idp-cfg/openid/dex1 -> create named config `dex1`
+//
+// PUT <admin-prefix>/idp-cfg/openid/_ -> create (default) named config `_`
+func (a adminAPIHandlers) AddIdentityProviderCfg(w http.ResponseWriter, r *http.Request) {
+	ctx := newContext(r, w, "AddIdentityProviderCfg")
+	defer logger.AuditLog(ctx, w, r, mustGetClaimsFromToken(r))
+
+	a.addOrUpdateIDPHandler(ctx, w, r, false)
+}
+
+// UpdateIdentityProviderCfg: updates an existing IDP config for openid/ldap.
+//
+// PATCH <admin-prefix>/idp-cfg/openid/dex1 -> update named config `dex1`
+//
+// PATCH <admin-prefix>/idp-cfg/openid/_ -> update (default) named config `_`
+func (a adminAPIHandlers) UpdateIdentityProviderCfg(w http.ResponseWriter, r *http.Request) {
+	ctx := newContext(r, w, "UpdateIdentityProviderCfg")
+	defer logger.AuditLog(ctx, w, r, mustGetClaimsFromToken(r))
+
+	a.addOrUpdateIDPHandler(ctx, w, r, true)
+}
+
+// ListIdentityProviderCfg:
+//
+// GET <admin-prefix>/idp-cfg/openid -> lists openid provider configs.
+func (a adminAPIHandlers) ListIdentityProviderCfg(w http.ResponseWriter, r *http.Request) {
+	ctx := newContext(r, w, "ListIdentityProviderCfg")
 	defer logger.AuditLog(ctx, w, r, mustGetClaimsFromToken(r))
 
 	objectAPI, cred := validateAdminReq(ctx, w, r, iampolicy.ConfigUpdateAdminAction)
 	if objectAPI == nil {
 		return
 	}
-
-	idpCfgType := mux.Vars(r)["type"]
-	cfgName := r.Form.Get("name")
 	password := cred.SecretKey
 
+	idpCfgType := mux.Vars(r)["type"]
 	if !madmin.ValidIDPConfigTypes.Contains(idpCfgType) {
 		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAdminConfigInvalidIDPType), r.URL)
 		return
 	}
 
-	// If no cfgName is provided, we list.
-	if cfgName == "" {
-		a.listIdentityProviders(ctx, w, r, idpCfgType, password)
-		return
-	}
-
-	cfg := globalServerConfig.Clone()
-	var cfgInfos []madmin.IDPCfgInfo
-	var err error
-	switch idpCfgType {
-	case madmin.OpenidIDPCfg:
-		cfgInfos, err = globalOpenIDConfig.GetConfigInfo(cfg, cfgName)
-	case madmin.LDAPIDPCfg:
-		cfgInfos, err = globalLDAPConfig.GetConfigInfo(cfg, cfgName)
-	}
-	if err != nil {
-		if errors.Is(err, openid.ErrProviderConfigNotFound) {
-			writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAdminNoSuchConfigTarget), r.URL)
-			return
-		}
-
-		writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
-		return
-	}
-
-	res := madmin.IDPConfig{
-		Type: idpCfgType,
-		Name: cfgName,
-		Info: cfgInfos,
-	}
-	data, err := json.Marshal(res)
-	if err != nil {
-		writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
-		return
-	}
-
-	econfigData, err := madmin.EncryptData(password, data)
-	if err != nil {
-		writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
-		return
-	}
-
-	writeSuccessResponseJSON(w, econfigData)
-}
-
-func (a adminAPIHandlers) listIdentityProviders(ctx context.Context, w http.ResponseWriter, r *http.Request, idpCfgType, password string) {
 	var cfgList []madmin.IDPListItem
 	var err error
 	switch idpCfgType {
@@ -261,9 +270,69 @@ func (a adminAPIHandlers) listIdentityProviders(ctx context.Context, w http.Resp
 	writeSuccessResponseJSON(w, econfigData)
 }
 
+// GetIdentityProviderCfg:
+//
+// GET <admin-prefix>/idp-cfg/openid/dex_test
+func (a adminAPIHandlers) GetIdentityProviderCfg(w http.ResponseWriter, r *http.Request) {
+	ctx := newContext(r, w, "GetIdentityProviderCfg")
+	defer logger.AuditLog(ctx, w, r, mustGetClaimsFromToken(r))
+
+	objectAPI, cred := validateAdminReq(ctx, w, r, iampolicy.ConfigUpdateAdminAction)
+	if objectAPI == nil {
+		return
+	}
+
+	idpCfgType := mux.Vars(r)["type"]
+	cfgName := mux.Vars(r)["name"]
+	password := cred.SecretKey
+
+	if !madmin.ValidIDPConfigTypes.Contains(idpCfgType) {
+		writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAdminConfigInvalidIDPType), r.URL)
+		return
+	}
+
+	cfg := globalServerConfig.Clone()
+	var cfgInfos []madmin.IDPCfgInfo
+	var err error
+	switch idpCfgType {
+	case madmin.OpenidIDPCfg:
+		cfgInfos, err = globalOpenIDConfig.GetConfigInfo(cfg, cfgName)
+	case madmin.LDAPIDPCfg:
+		cfgInfos, err = globalLDAPConfig.GetConfigInfo(cfg, cfgName)
+	}
+	if err != nil {
+		if errors.Is(err, openid.ErrProviderConfigNotFound) || errors.Is(err, cfgldap.ErrProviderConfigNotFound) {
+			writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAdminNoSuchConfigTarget), r.URL)
+			return
+		}
+
+		writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
+		return
+	}
+
+	res := madmin.IDPConfig{
+		Type: idpCfgType,
+		Name: cfgName,
+		Info: cfgInfos,
+	}
+	data, err := json.Marshal(res)
+	if err != nil {
+		writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
+		return
+	}
+
+	econfigData, err := madmin.EncryptData(password, data)
+	if err != nil {
+		writeErrorResponseJSON(ctx, w, toAdminAPIErr(ctx, err), r.URL)
+		return
+	}
+
+	writeSuccessResponseJSON(w, econfigData)
+}
+
 // DeleteIdentityProviderCfg:
 //
-// DELETE <admin-prefix>/id-cfg?type=openid&name=dex_test
+// DELETE <admin-prefix>/idp-cfg/openid/dex_test
 func (a adminAPIHandlers) DeleteIdentityProviderCfg(w http.ResponseWriter, r *http.Request) {
 	ctx := newContext(r, w, "DeleteIdentityProviderCfg")
 

cmd/admin-router.go

@@ -184,9 +184,11 @@ func registerAdminRouter(router *mux.Router, enableConfigOps bool) {
 		adminRouter.Methods(http.MethodPut).Path(adminVersion + "/import-iam").HandlerFunc(httpTraceHdrs(adminAPI.ImportIAM))
 
 		// IDentity Provider configuration APIs
-		adminRouter.Methods(http.MethodPut).Path(adminVersion+"/idp-config").HandlerFunc(gz(httpTraceHdrs(adminAPI.SetIdentityProviderCfg))).Queries("type", "{type:.*}").Queries("name", "{name:.*}")
+		adminRouter.Methods(http.MethodPut).Path(adminVersion + "/idp-config/{type}/{name}").HandlerFunc(gz(httpTraceHdrs(adminAPI.AddIdentityProviderCfg)))
-		adminRouter.Methods(http.MethodGet).Path(adminVersion+"/idp-config").HandlerFunc(gz(httpTraceHdrs(adminAPI.GetIdentityProviderCfg))).Queries("type", "{type:.*}")
+		adminRouter.Methods(http.MethodPost).Path(adminVersion + "/idp-config/{type}/{name}").HandlerFunc(gz(httpTraceHdrs(adminAPI.UpdateIdentityProviderCfg)))
-		adminRouter.Methods(http.MethodDelete).Path(adminVersion+"/idp-config").HandlerFunc(gz(httpTraceHdrs(adminAPI.DeleteIdentityProviderCfg))).Queries("type", "{type:.*}").Queries("name", "{name:.*}")
+		adminRouter.Methods(http.MethodGet).Path(adminVersion + "/idp-config/{type}").HandlerFunc(gz(httpTraceHdrs(adminAPI.ListIdentityProviderCfg)))
+		adminRouter.Methods(http.MethodGet).Path(adminVersion + "/idp-config/{type}/{name}").HandlerFunc(gz(httpTraceHdrs(adminAPI.GetIdentityProviderCfg)))
+		adminRouter.Methods(http.MethodDelete).Path(adminVersion + "/idp-config/{type}/{name}").HandlerFunc(gz(httpTraceHdrs(adminAPI.DeleteIdentityProviderCfg)))
 
 		// -- END IAM APIs --
 

cmd/api-errors.go

@@ -277,6 +277,8 @@ const (
 	ErrAdminConfigDuplicateKeys
 	ErrAdminConfigInvalidIDPType
 	ErrAdminConfigLDAPValidation
+	ErrAdminConfigIDPCfgNameAlreadyExists
+	ErrAdminConfigIDPCfgNameDoesNotExist
 	ErrAdminCredentialsMismatch
 	ErrInsecureClientRequest
 	ErrObjectTampered
@@ -1305,6 +1307,16 @@ var errorCodes = errorCodeMap{
 		Description:    "LDAP Configuration validation failed",
 		HTTPStatusCode: http.StatusBadRequest,
 	},
+	ErrAdminConfigIDPCfgNameAlreadyExists: {
+		Code:           "XMinioAdminConfigIDPCfgNameAlreadyExists",
+		Description:    "An IDP configuration with the given name aleady exists",
+		HTTPStatusCode: http.StatusBadRequest,
+	},
+	ErrAdminConfigIDPCfgNameDoesNotExist: {
+		Code:           "XMinioAdminConfigIDPCfgNameDoesNotExist",
+		Description:    "No such IDP configuration exists",
+		HTTPStatusCode: http.StatusBadRequest,
+	},
 	ErrAdminConfigNotificationTargetsFailed: {
 		Code:           "XMinioAdminNotificationTargetsTestFailed",
 		Description:    "Configuration update failed due an unsuccessful attempt to connect to one or more notification servers",