Fix issue with ServiceNow incidents not including link to access request (#33565)

* Fix issue with ServiceNow incidents not including link to access request

* Add cluster to incident desciption and include user as caller

* Add status sink to servicenow client

* Fix formatting

* Undefer status updating in servicenow plugin

* Add log of plugin status

* Update integrations/access/servicenow/client.go

Co-authored-by: Marco André Dinis <marco.dinis@goteleport.com>

* Fix bug caused by caller_id field being a different type on response

* Change format for description for resource requests

* Fix mock servicenow to use seperate incident response type

* Update integrations/access/servicenow/client_test.go

Co-authored-by: Roman Tkachenko <roman@goteleport.com>

* Fix formating

* Update tests

---------

Co-authored-by: Roman Tkachenko <roman@goteleport.com>
Co-authored-by: Marco André Dinis <marco.dinis@goteleport.com>

integrations/access/servicenow/bot.go

@@ -50,6 +50,7 @@ func (b *Bot) Broadcast(ctx context.Context, recipients []common.Recipient, reqI
 		Created:       time.Now().UTC(),
 		RequestReason: reqData.RequestReason,
 		ReviewsCount:  reqData.ReviewsCount,
+		Resources:     reqData.Resources,
 	}
 	serviceNowData, err := b.client.CreateIncident(ctx, reqID, serviceNowReqData)
 	if err != nil {

integrations/access/servicenow/client.go

@@ -30,7 +30,9 @@ import (
 	"github.com/gravitational/trace"
 
 	"github.com/gravitational/teleport/api/types"
+	"github.com/gravitational/teleport/integrations/access/common"
 	"github.com/gravitational/teleport/integrations/lib"
+	"github.com/gravitational/teleport/integrations/lib/logger"
 )
 
 const (
@@ -38,24 +40,6 @@ const (
 	DateTimeFormat = "2006-01-02 15:04:05"
 )
 
-var (
-	incidentBodyTemplate = template.Must(template.New("incident body").Parse(
-		`{{.User}} requested permissions for roles {{range $index, $element := .Roles}}{{if $index}}, {{end}}{{ . }}{{end}} on Teleport at {{.Created.Format .TimeFormat}}.
-{{if .RequestReason}}Reason: {{.RequestReason}}{{end}}
-{{if .RequestLink}}To approve or deny the request, proceed to {{.RequestLink}}{{end}}
-`,
-	))
-	reviewNoteTemplate = template.Must(template.New("review note").Parse(
-		`{{.Author}} reviewed the request at {{.Created.Format .TimeFormat}}.
-Resolution: {{.ProposedState}}.
-{{if .Reason}}Reason: {{.Reason}}.{{end}}`,
-	))
-	resolutionNoteTemplate = template.Must(template.New("resolution note").Parse(
-		`Access request has been {{.Resolution}}
-{{if .ResolveReason}}Reason: {{.ResolveReason}}{{end}}`,
-	))
-)
-
 // Client is a wrapper around resty.Client.
 type Client struct {
 	ClientConfig
@@ -73,12 +57,19 @@ type ClientConfig struct {
 	// allowing links to the access requests to be built
 	WebProxyURL *url.URL
 
+	// ClusterName is the name of the Teleport cluster.
+	ClusterName string
+
 	// Username is the username used by the client for basic auth.
 	Username string
 	// APIToken is the token used for basic auth.
 	APIToken string
 	// CloseCode is the ServiceNow close code that incidents will be closed with.
 	CloseCode string
+
+	// StatusSink receives any status updates from the plugin for
+	// further processing. Status updates will be ignored if not set.
+	StatusSink common.StatusSink
 }
 
 // NewClient creates a new Servicenow client for managing incidents.
@@ -134,8 +125,9 @@ func (snc *Client) CreateIncident(ctx context.Context, reqID string, reqData Req
 	}
 
 	body := Incident{
-		ShortDescription: fmt.Sprintf("Access request from %s", reqData.User),
+		ShortDescription: fmt.Sprintf("Teleport access request from user %s", reqData.User),
 		Description:      bodyDetails,
+		Caller:           reqData.User,
 	}
 
 	var result incidentResult
@@ -152,7 +144,7 @@ func (snc *Client) CreateIncident(ctx context.Context, reqID string, reqData Req
 		return Incident{}, errWrapper(resp.StatusCode(), string(resp.Body()))
 	}
 
-	return result.Result, nil
+	return Incident{IncidentID: result.Result.IncidentID}, nil
 }
 
 // PostReviewNote posts a note once a new request review appears.
@@ -250,6 +242,23 @@ func (snc *Client) CheckHealth(ctx context.Context) error {
 		return trace.Wrap(err)
 	}
 	defer resp.RawResponse.Body.Close()
+
+	if snc.StatusSink != nil {
+		var code types.PluginStatusCode
+		switch {
+		case resp.StatusCode() == http.StatusUnauthorized:
+			code = types.PluginStatusCode_UNAUTHORIZED
+		case resp.StatusCode() >= 200 && resp.StatusCode() < 400:
+			code = types.PluginStatusCode_RUNNING
+		default:
+			code = types.PluginStatusCode_OTHER_ERROR
+		}
+		if err := snc.StatusSink.Emit(ctx, &types.PluginStatusV1{Code: code}); err != nil {
+			log := logger.Get(resp.Request.Context())
+			log.WithError(err).WithField("code", resp.StatusCode()).Errorf("Error while emitting servicenow plugin status: %v", err)
+		}
+	}
+
 	if resp.IsError() {
 		return errWrapper(resp.StatusCode(), string(resp.Body()))
 	}
@@ -283,6 +292,30 @@ func (snc *Client) GetUserEmail(ctx context.Context, userID string) (string, err
 	return result.Result[0].Email, nil
 }
 
+var (
+	incidentWithRolesBodyTemplate = template.Must(template.New("incident body").Parse(
+		`Teleport user {{.User}} submitted access request for roles {{range $index, $element := .Roles}}{{if $index}}, {{end}}{{ . }}{{end}} on Teleport cluster {{.ClusterName}}.
+{{if .RequestReason}}Reason: {{.RequestReason}}{{end}}
+{{if .RequestLink}}Click this link to review the request in Teleport: {{.RequestLink}}{{end}}
+`,
+	))
+	incidentBodyTemplate = template.Must(template.New("incident body").Parse(
+		`Teleport user {{.User}} submitted access request on Teleport cluster {{.ClusterName}}.
+{{if .RequestReason}}Reason: {{.RequestReason}}{{end}}
+{{if .RequestLink}}Click this link to review the request in Teleport: {{.RequestLink}}{{end}}
+`,
+	))
+	reviewNoteTemplate = template.Must(template.New("review note").Parse(
+		`{{.Author}} reviewed the request at {{.Created.Format .TimeFormat}}.
+Resolution: {{.ProposedState}}.
+{{if .Reason}}Reason: {{.Reason}}.{{end}}`,
+	))
+	resolutionNoteTemplate = template.Must(template.New("resolution note").Parse(
+		`Access request has been {{.Resolution}}
+{{if .ResolveReason}}Reason: {{.ResolveReason}}{{end}}`,
+	))
+)
+
 func (snc *Client) buildIncidentBody(webProxyURL *url.URL, reqID string, reqData RequestData) (string, error) {
 	var requestLink string
 	if webProxyURL != nil {
@@ -292,15 +325,21 @@ func (snc *Client) buildIncidentBody(webProxyURL *url.URL, reqID string, reqData
 	}
 
 	var builder strings.Builder
-	err := incidentBodyTemplate.Execute(&builder, struct {
+	template := incidentBodyTemplate
+	if reqData.Resources == nil {
+		template = incidentWithRolesBodyTemplate
+	}
+	err := template.Execute(&builder, struct {
 		ID          string
 		TimeFormat  string
 		RequestLink string
+		ClusterName string
 		RequestData
 	}{
 		ID:          reqID,
 		TimeFormat:  time.RFC822,
 		RequestLink: requestLink,
+		ClusterName: snc.ClusterName,
 		RequestData: reqData,
 	})
 	if err != nil {

integrations/access/servicenow/client_test.go

@@ -57,8 +57,9 @@ func TestCreateIncident(t *testing.T) {
 	assert.NoError(t, err)
 
 	expected := Incident{
-		ShortDescription: "Access request from someUser",
-		Description:      "someUser requested permissions for roles role1, role2 on Teleport at 01 Jan 01 00:00 UTC.\nReason: someReason\n\n",
+		ShortDescription: "Teleport access request from user someUser",
+		Description:      "Teleport user someUser submitted access request for roles role1, role2 on Teleport cluster .\nReason: someReason\n\n",
+		Caller:           "someUser",
 	}
 	var got Incident
 	err = json.Unmarshal([]byte(recievedReq), &got)

integrations/access/servicenow/config.go

@@ -65,11 +65,15 @@ func (c *Config) NewBot(clusterName, webProxyAddr string) (common.MessagingBot,
 		err         error
 	)
 	if webProxyAddr != "" {
-		if webProxyURL, err = lib.AddrToURL(webProxyAddr); err != nil {
+		if c.WebProxyURL, err = lib.AddrToURL(webProxyAddr); err != nil {
 			return nil, trace.Wrap(err)
 		}
 	}
 
+	if clusterName != "" {
+		c.ClusterName = clusterName
+	}
+
 	client, err := NewClient(c.ClientConfig)
 	if err != nil {
 		return nil, trace.Wrap(err)

integrations/access/servicenow/config_test.go

@@ -0,0 +1,37 @@
+/*
+Copyright 2015-2023 Gravitational, Inc.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package servicenow
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestNewBot(t *testing.T) {
+	conf := Config{
+		ClientConfig: ClientConfig{
+			APIEndpoint: "serviceNowSpec.ApiEndpoint",
+			Username:    "username",
+			APIToken:    "password",
+			CloseCode:   "serviceNowSpec.CloseCode",
+		},
+	}
+	_, err := conf.NewBot("someClusterName", "someWebProxyAddr")
+	require.NoError(t, err)
+	require.Equal(t, conf.WebProxyURL.Host, "someWebProxyAddr")
+}

integrations/access/servicenow/fake_servicenow_test.go

@@ -90,7 +90,23 @@ func NewFakeServiceNow(concurrency int) *FakeServiceNow {
 		serviceNow.StoreIncident(incident)
 		serviceNow.newIncidents <- incident
 
-		err = json.NewEncoder(rw).Encode(incidentResult{Result: incident})
+		err = json.NewEncoder(rw).Encode(incidentResult{Result: struct {
+			IncidentID       string `json:"sys_id,omitempty"`
+			ShortDescription string `json:"short_description,omitempty"`
+			Description      string `json:"description,omitempty"`
+			CloseCode        string `json:"close_code,omitempty"`
+			CloseNotes       string `json:"close_notes,omitempty"`
+			IncidentState    string `json:"incident_state,omitempty"`
+			WorkNotes        string `json:"work_notes,omitempty"`
+		}{
+			IncidentID:       incident.IncidentID,
+			ShortDescription: incident.ShortDescription,
+			Description:      incident.Description,
+			CloseCode:        incident.CloseCode,
+			CloseNotes:       incident.CloseNotes,
+			IncidentState:    incident.IncidentState,
+			WorkNotes:        incident.WorkNotes,
+		}})
 		panicIf(err)
 	})
 	router.PATCH("/api/now/v1/table/incident/:incidentID/", func(rw http.ResponseWriter, r *http.Request, ps httprouter.Params) {

integrations/access/servicenow/servicenow_test.go

@@ -372,7 +372,7 @@ func (s *ServiceNowSuite) TestApproval() {
 
 	incident, err := s.fakeServiceNow.CheckIncidentUpdate(s.Context())
 	require.NoError(t, err)
-	require.Contains(t, incident.Description, "requested permissions")
+	require.Contains(t, incident.Description, "submitted access request")
 	assert.Contains(t, incident.CloseNotes, "Access request has been resolved")
 	assert.Contains(t, incident.CloseNotes, "Reason: okay")
 	assert.Equal(t, incident.CloseCode, "resolved")
@@ -387,7 +387,7 @@ func (s *ServiceNowSuite) TestDenial() {
 
 	incident, err := s.fakeServiceNow.CheckNewIncident(s.Context())
 	require.NoError(t, err, "no new incidents stored")
-	require.Contains(t, incident.Description, "requested permissions")
+	require.Contains(t, incident.Description, "submitted access request")
 
 	err = s.ruler().DenyAccessRequest(s.Context(), req.GetName(), "not okay")
 	require.NoError(t, err)
@@ -457,7 +457,7 @@ func (s *ServiceNowSuite) TestApprovalByReview() {
 
 	incident, err := s.fakeServiceNow.CheckNewIncident(s.Context())
 	require.NoError(t, err, "no new incidents stored")
-	require.Contains(t, incident.Description, "requested permissions")
+	require.Contains(t, incident.Description, "submitted access request")
 
 	err = s.reviewer1().SubmitAccessRequestReview(s.Context(), req.GetName(), types.AccessReview{
 		Author:        s.userNames.reviewer1,
@@ -508,7 +508,7 @@ func (s *ServiceNowSuite) TestDenialByReview() {
 
 	incident, err := s.fakeServiceNow.CheckNewIncident(s.Context())
 	require.NoError(t, err, "no new incidents stored")
-	require.Contains(t, incident.Description, "requested permissions")
+	require.Contains(t, incident.Description, "submitted access request")
 
 	err = s.reviewer1().SubmitAccessRequestReview(s.Context(), req.GetName(), types.AccessReview{
 		Author:        s.userNames.reviewer1,

integrations/access/servicenow/types.go

@@ -50,6 +50,8 @@ type Incident struct {
 	IncidentState string `json:"incident_state,omitempty"`
 	// WorkNotes contains comments on the progress of the incident.
 	WorkNotes string `json:"work_notes,omitempty"`
+	// Caller is the user on whose behalf the incident is being created. (Must be an existing servicenow user)
+	Caller string `json:"caller_id,omitempty"`
 }
 
 const (
@@ -86,6 +88,8 @@ type RequestData struct {
 	Resolution Resolution
 	// SystemAnnotations contains key value annotations for the request.
 	SystemAnnotations types.Labels
+	// Resources are the resources being requested.
+	Resources []string
 }
 
 type onCallResult struct {
@@ -103,5 +107,20 @@ type userResult struct {
 }
 
 type incidentResult struct {
-	Result Incident `json:"result"`
+	Result struct {
+		// IncidentID is the sys_id of the incident
+		IncidentID string `json:"sys_id,omitempty"`
+		// ShortDescription contains a brief summary of the incident.
+		ShortDescription string `json:"short_description,omitempty"`
+		// Description contains the description of the incident.
+		Description string `json:"description,omitempty"`
+		// CloseCode contains the close code of the incident once it is resolved.
+		CloseCode string `json:"close_code,omitempty"`
+		// CloseNotes contains the closing comments on the incident once it is resolved.
+		CloseNotes string `json:"close_notes,omitempty"`
+		// IncidentState contains the current state the incident is in.
+		IncidentState string `json:"incident_state,omitempty"`
+		// WorkNotes contains comments on the progress of the incident.
+		WorkNotes string `json:"work_notes,omitempty"`
+	} `json:"result"`
 }