acl: sso auth methods RPC/API/CLI should return created or updated ob…

…jects (#15410) Currently CRUD code that operates on SSO auth methods does not return created or updated object upon creation/update. This is bad UX and inconsistent behavior compared to other ACL objects like roles, policies or tokens. This PR fixes it. Relates to #13120
hashicorp · Nov 29, 2022 · fe1ff60 · fe1ff60
1 parent fa0afdb
commit fe1ff60
Show file tree

Hide file tree

Showing 9 changed files with 54 additions and 28 deletions.
diff --git a/api/acl.go b/api/acl.go
@@ -319,27 +319,29 @@ func (a *ACLAuthMethods) List(q *QueryOptions) ([]*ACLAuthMethodListStub, *Query
 }
 
 // Create is used to create an ACL auth-method.
-func (a *ACLAuthMethods) Create(authMethod *ACLAuthMethod, w *WriteOptions) (*WriteMeta, error) {
+func (a *ACLAuthMethods) Create(authMethod *ACLAuthMethod, w *WriteOptions) (*ACLAuthMethod, *WriteMeta, error) {
 	if authMethod.Name == "" {
-		return nil, errMissingACLAuthMethodName
+		return nil, nil, errMissingACLAuthMethodName
 	}
-	wm, err := a.client.write("/v1/acl/auth-method", authMethod, nil, w)
+	var resp ACLAuthMethod
+	wm, err := a.client.write("/v1/acl/auth-method", authMethod, &resp, w)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
-	return wm, nil
+	return &resp, wm, nil
 }
 
 // Update is used to update an existing ACL auth-method.
-func (a *ACLAuthMethods) Update(authMethod *ACLAuthMethod, w *WriteOptions) (*WriteMeta, error) {
+func (a *ACLAuthMethods) Update(authMethod *ACLAuthMethod, w *WriteOptions) (*ACLAuthMethod, *WriteMeta, error) {
 	if authMethod.Name == "" {
-		return nil, errMissingACLAuthMethodName
+		return nil, nil, errMissingACLAuthMethodName
 	}
-	wm, err := a.client.write("/v1/acl/auth-method/"+authMethod.Name, authMethod, nil, w)
+	var resp ACLAuthMethod
+	wm, err := a.client.write("/v1/acl/auth-method/"+authMethod.Name, authMethod, &resp, w)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
-	return wm, nil
+	return &resp, wm, nil
 }
 
 // Delete is used to delete an ACL auth-method.

diff --git a/api/acl_test.go b/api/acl_test.go
@@ -209,7 +209,7 @@ func TestACLTokens_CreateUpdate(t *testing.T) {
 	out2.Roles = []*ACLTokenRoleLink{{Name: aclRoleCreateResp.Name}}
 	out2.ExpirationTTL = 0
 
-	out3, writeMeta, err = at.Update(out2, nil)
+	out3, _, err = at.Update(out2, nil)
 	require.NoError(t, err)
 	require.NotNil(t, out3)
 	require.Len(t, out3.Policies, 1)
@@ -608,7 +608,7 @@ func TestACLAuthMethods(t *testing.T) {
 		MaxTokenTTL:   15 * time.Minute,
 		Default:       true,
 	}
-	writeMeta, err := testClient.ACLAuthMethods().Create(&authMethod, nil)
+	_, writeMeta, err := testClient.ACLAuthMethods().Create(&authMethod, nil)
 	must.NoError(t, err)
 	assertWriteMeta(t, writeMeta)
 
@@ -632,7 +632,7 @@ func TestACLAuthMethods(t *testing.T) {
 
 	// Update the auth-method token locality.
 	authMethod.TokenLocality = ACLAuthMethodTokenLocalityGlobal
-	writeMeta, err = testClient.ACLAuthMethods().Update(&authMethod, nil)
+	_, writeMeta, err = testClient.ACLAuthMethods().Update(&authMethod, nil)
 	must.NoError(t, err)
 	assertWriteMeta(t, writeMeta)
 

diff --git a/command/acl_auth_method_create.go b/command/acl_auth_method_create.go
@@ -168,12 +168,12 @@ func (a *ACLAuthMethodCreateCommand) Run(args []string) int {
 	}
 
 	// Create the auth method via the API.
-	_, err = client.ACLAuthMethods().Create(&authMethod, nil)
+	method, _, err := client.ACLAuthMethods().Create(&authMethod, nil)
 	if err != nil {
 		a.Ui.Error(fmt.Sprintf("Error creating ACL auth method: %v", err))
 		return 1
 	}
 
-	a.Ui.Output(fmt.Sprintf("Created ACL auth method %s", a.name))
+	a.Ui.Output(fmt.Sprintf("Created ACL auth method:\n%s", formatAuthMethod(method)))
 	return 0
 }
diff --git a/command/acl_auth_method_update.go b/command/acl_auth_method_update.go
@@ -185,13 +185,13 @@ func (a *ACLAuthMethodUpdateCommand) Run(args []string) int {
 	}
 
 	// Update the auth method via the API.
-	_, err = client.ACLAuthMethods().Update(&updatedMethod, nil)
+	method, _, err := client.ACLAuthMethods().Update(&updatedMethod, nil)
 	if err != nil {
 		a.Ui.Error(fmt.Sprintf("Error updating ACL auth method: %v", err))
 		return 1
 	}
 
-	a.Ui.Output(fmt.Sprintf("Updated ACL auth method %s", originalMethodName))
+	a.Ui.Output(fmt.Sprintf("Updated ACL auth method:\n%s", formatAuthMethod(method)))
 	return 0
 }
 

diff --git a/command/agent/acl_endpoint.go b/command/agent/acl_endpoint.go
@@ -670,5 +670,9 @@ func (s *HTTPServer) aclAuthMethodUpsertRequest(
 		return nil, err
 	}
 	setIndex(resp, out.Index)
+
+	if len(out.AuthMethods) > 0 {
+		return out.AuthMethods[0], nil
+	}
 	return nil, nil
 }
diff --git a/command/agent/acl_endpoint_test.go b/command/agent/acl_endpoint_test.go
@@ -1104,14 +1104,14 @@ func TestHTTPServer_ACLAuthMethodRequest(t *testing.T) {
 
 				// Build the HTTP request.
 				req, err := http.NewRequest(http.MethodPut, "/v1/acl/auth-method", encodeReq(mockACLRole))
-				require.NoError(t, err)
+				must.NoError(t, err)
 				respW := httptest.NewRecorder()
 
 				// Send the HTTP request.
 				obj, err := srv.Server.ACLAuthMethodRequest(respW, req)
-				require.Error(t, err)
-				require.ErrorContains(t, err, "Permission denied")
-				require.Nil(t, obj)
+				must.Error(t, err)
+				must.StrContains(t, err.Error(), "Permission denied")
+				must.Nil(t, obj)
 			},
 		},
 		{
@@ -1120,17 +1120,17 @@ func TestHTTPServer_ACLAuthMethodRequest(t *testing.T) {
 
 				// Build the HTTP request.
 				req, err := http.NewRequest(http.MethodConnect, "/v1/acl/auth-method", nil)
-				require.NoError(t, err)
+				must.NoError(t, err)
 				respW := httptest.NewRecorder()
 
 				// Ensure we have a token set.
 				setToken(req, srv.RootToken)
 
 				// Send the HTTP request.
 				obj, err := srv.Server.ACLAuthMethodRequest(respW, req)
-				require.Error(t, err)
-				require.ErrorContains(t, err, "Invalid method")
-				require.Nil(t, obj)
+				must.Error(t, err)
+				must.StrContains(t, err.Error(), "Invalid method")
+				must.Nil(t, obj)
 			},
 		},
 		{
@@ -1142,16 +1142,18 @@ func TestHTTPServer_ACLAuthMethodRequest(t *testing.T) {
 
 				// Build the HTTP request.
 				req, err := http.NewRequest(http.MethodPut, "/v1/acl/auth-method", encodeReq(mockACLAuthMethod))
-				require.NoError(t, err)
+				must.NoError(t, err)
 				respW := httptest.NewRecorder()
 
 				// Ensure we have a token set.
 				setToken(req, srv.RootToken)
 
 				// Send the HTTP request.
 				obj, err := srv.Server.ACLAuthMethodRequest(respW, req)
-				require.NoError(t, err)
-				require.Nil(t, obj)
+				must.NoError(t, err)
+
+				result := obj.(*structs.ACLAuthMethod)
+				must.Eq(t, result, mockACLAuthMethod)
 			},
 		},
 	}

diff --git a/nomad/acl_endpoint.go b/nomad/acl_endpoint.go
@@ -1724,6 +1724,22 @@ func (a *ACL) UpsertAuthMethods(
 		return err
 	}
 
+	// Populate the response. We do a lookup against the state to pick up the
+	// proper create / modify times.
+	stateSnapshot, err := a.srv.State().Snapshot()
+	if err != nil {
+		return err
+	}
+	for _, method := range args.AuthMethods {
+		lookupAuthMethod, err := stateSnapshot.GetACLAuthMethodByName(nil, method.Name)
+		if err != nil {
+			return structs.NewErrRPCCodedf(400, "ACL auth method lookup failed: %v", err)
+		}
+		if lookupAuthMethod != nil {
+			reply.AuthMethods = append(reply.AuthMethods, lookupAuthMethod)
+		}
+	}
+
 	// Update the index
 	reply.Index = index
 	return nil

diff --git a/nomad/acl_endpoint_test.go b/nomad/acl_endpoint_test.go
@@ -3041,4 +3041,5 @@ func TestACLEndpoint_UpsertACLAuthMethods(t *testing.T) {
 	out, err := s1.fsm.State().GetACLAuthMethodByName(nil, am1.Name)
 	must.Nil(t, err)
 	must.NotNil(t, out)
+	must.True(t, am1.Equal(resp.AuthMethods[0]))
 }
diff --git a/nomad/structs/acl.go b/nomad/structs/acl.go
@@ -825,6 +825,7 @@ type ACLAuthMethodUpsertRequest struct {
 // ACLAuthMethodUpsertResponse is a response of the upsert ACL auth methods
 // operation
 type ACLAuthMethodUpsertResponse struct {
+	AuthMethods []*ACLAuthMethod
 	WriteMeta
 }