Merge pull request #82 from ConductorOne/bugfix/add_grant_already_exists

agustin-conductor · web-flow · commit 41f68f97f18b · 2025-06-05T14:36:42.000-03:00
[BB-801] add grant already exists for groups
diff --git a/pkg/client/client.go b/pkg/client/client.go
@@ -92,7 +92,7 @@ const (
 	allGroupsV2           = "rest/api/2/groups/picker"
 	allPermissionSchemeV2 = "rest/api/2/permissionscheme"
 	addUserToGroup        = "rest/api/2/group/user"
-	createUserPath        = "rest/api/2/user"
+	baseUserPath          = "rest/api/2/user"
 	NF                    = -1
 )
 
@@ -331,6 +331,26 @@ func (client *Client) GetUser(ctx context.Context, userName string) (jira.User,
 	}, err
 }
 
+func (client *Client) GetUserByKey(ctx context.Context, userKey string) (*UsersAPIData, error) {
+	var userAPIData UsersAPIData
+	req, err := getRequest(ctx, client, baseUserPath, Query{
+		"key":    userKey,
+		"expand": "groups",
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	resp, err := client.httpClient.Do(req, uhttp.WithJSONResponse(&userAPIData))
+	if err != nil {
+		return nil, err
+	}
+
+	defer resp.Body.Close()
+
+	return &userAPIData, err
+}
+
 // GetProjectRoles
 // Returns all roles that are present in specific project.
 func (client *Client) GetProjectRoles(ctx context.Context, projectId string) (map[string]string, error) {
@@ -553,12 +573,12 @@ func (client *Client) RemoveActorsFromProjectRole(ctx context.Context, projectId
 
 // AddUserToGroup
 // Adds given user to a group in the Jira DC.
-// Returns the current state of the group.
+// Returns the true is the user was added successfully (status code = 201).
 // https://docs.atlassian.com/software/jira/docs/api/REST/9.14.0/#api/2/group-addUserToGroup
-func (client *Client) AddUserToGroup(ctx context.Context, groupName, userName string) (int, error) {
+func (client *Client) AddUserToGroup(ctx context.Context, groupName, userName string) (bool, error) {
 	endpointUrl, err := url.JoinPath(addUserToGroup)
 	if err != nil {
-		return NF, err
+		return false, err
 	}
 
 	req, err := getXRequest(ctx, client, http.MethodPost, endpointUrl, Query{
@@ -567,16 +587,16 @@ func (client *Client) AddUserToGroup(ctx context.Context, groupName, userName st
 		Name: userName,
 	})
 	if err != nil {
-		return NF, err
+		return false, err
 	}
 
 	resp, err := client.httpClient.Do(req)
 	if err != nil {
-		return NF, err
+		return false, err
 	}
 
 	defer resp.Body.Close()
-	return resp.StatusCode, err
+	return resp.StatusCode == http.StatusCreated, err
 }
 
 // GetUserName
@@ -600,28 +620,29 @@ func (client *Client) GetUserName(ctx context.Context, userId string) (string, e
 
 // RemoveUserFromGroup
 // Removes given user from a group in the Jira DC.
+// Returns true if the user was successfully removed.
 // https://docs.atlassian.com/software/jira/docs/api/REST/9.14.0/#api/2/group-removeUserFromGroup
-func (client *Client) RemoveUserFromGroup(ctx context.Context, groupName, userName string) (int, error) {
+func (client *Client) RemoveUserFromGroup(ctx context.Context, groupName, userName string) (bool, error) {
 	endpointUrl, err := url.JoinPath(addUserToGroup)
 	if err != nil {
-		return NF, err
+		return false, err
 	}
 
 	req, err := getXRequest(ctx, client, http.MethodDelete, endpointUrl, Query{
 		"groupname": groupName,
 		"username":  userName,
 	}, BodyActors{})
 	if err != nil {
-		return NF, err
+		return false, err
 	}
 
 	resp, err := client.httpClient.Do(req)
 	if err != nil {
-		return NF, err
+		return false, err
 	}
 
 	defer resp.Body.Close()
-	return resp.StatusCode, err
+	return resp.StatusCode == http.StatusOK, err
 }
 
 // AddProjectRoleActorsToRole
@@ -693,7 +714,7 @@ func (client *Client) CreateUser(ctx context.Context, userRequest *CreateUserReq
 	var userData UsersAPIData
 
 	// Create the URL
-	endpointUrl, err := url.JoinPath(client.BaseURL, createUserPath)
+	endpointUrl, err := url.JoinPath(client.BaseURL, baseUserPath)
 	if err != nil {
 		return nil, err
 	}
diff --git a/pkg/client/internal_test.go b/pkg/client/internal_test.go
@@ -258,9 +258,9 @@ func TestClientAddUserToGroup(t *testing.T) {
 	userName, err := client.GetUserName(ctx, "JIRAUSER10103")
 	assert.Nil(t, err)
 
-	statusCode, err := client.AddUserToGroup(ctx, "jira-administrators", userName)
+	success, err := client.AddUserToGroup(ctx, "jira-administrators", userName)
 	assert.Nil(t, err)
-	assert.Equal(t, http.StatusCreated, statusCode)
+	assert.True(t, success)
 }
 
 func TestClientRemoveUserFromGroup(t *testing.T) {
@@ -272,9 +272,9 @@ func TestClientRemoveUserFromGroup(t *testing.T) {
 	userName, err := client.GetUserName(ctx, "JIRAUSER10103")
 	assert.Nil(t, err)
 
-	statusCode, err := client.RemoveUserFromGroup(ctx, "jira-administrators", userName)
+	success, err := client.RemoveUserFromGroup(ctx, "jira-administrators", userName)
 	assert.Nil(t, err)
-	assert.Equal(t, http.StatusOK, statusCode)
+	assert.True(t, success)
 }
 
 func TestClientAddProjectRoleActorsToRoleWithGroup(t *testing.T) {
diff --git a/pkg/client/model.go b/pkg/client/model.go
@@ -73,16 +73,27 @@ type Actors struct {
 }
 
 type UsersAPIData struct {
-	Self         string     `json:"self,omitempty"`
-	Key          string     `json:"key,omitempty"`
-	Name         string     `json:"name,omitempty"`
-	EmailAddress string     `json:"emailAddress,omitempty"`
-	AvatarUrls   AvatarUrls `json:"avatarUrls,omitempty"`
-	DisplayName  string     `json:"displayName,omitempty"`
-	Active       bool       `json:"active,omitempty"`
-	Deleted      bool       `json:"deleted,omitempty"`
-	TimeZone     string     `json:"timeZone,omitempty"`
-	Locale       string     `json:"locale,omitempty"`
+	Self         string          `json:"self,omitempty"`
+	Key          string          `json:"key,omitempty"`
+	Name         string          `json:"name,omitempty"`
+	EmailAddress string          `json:"emailAddress,omitempty"`
+	AvatarUrls   AvatarUrls      `json:"avatarUrls,omitempty"`
+	DisplayName  string          `json:"displayName,omitempty"`
+	Active       bool            `json:"active,omitempty"`
+	Deleted      bool            `json:"deleted,omitempty"`
+	TimeZone     string          `json:"timeZone,omitempty"`
+	Locale       string          `json:"locale,omitempty"`
+	Groups       *UserGroupsList `json:"groups,omitempty"`
+}
+
+type UserGroupsList struct {
+	Size  int         `json:"size,omitempty"`
+	Items []UserGroup `json:"items,omitempty"`
+}
+
+type UserGroup struct {
+	Name string `json:"name,omitempty"`
+	Self string `json:"self,omitempty"`
 }
 
 type GroupRolesAPIData struct {
diff --git a/pkg/connector/group.go b/pkg/connector/group.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"net/http"
 
 	v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2"
 	"github.com/conductorone/baton-sdk/pkg/annotations"
@@ -163,25 +162,40 @@ func (g *groupBuilder) Grant(ctx context.Context, principal *v2.Resource, entitl
 
 	groupId := entitlement.Resource.Id.Resource
 	userId := principal.Id.Resource
-	userName, err := g.client.GetUserName(ctx, userId)
+	userInfo, err := g.client.GetUserByKey(ctx, userId)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("jira(dc)-connector: error retrieving user %s: %w", userId, err)
+	}
+
+	// Check user's group membership
+	if userInfo.Groups != nil && userInfo.Groups.Size > 0 {
+		for _, group := range userInfo.Groups.Items {
+			if group.Name == groupId {
+				l.Debug("Group Membership already exists.",
+					zap.String("userId", userId),
+					zap.String("userName", userInfo.Name),
+					zap.String("group", groupId),
+				)
+				return annotations.New(&v2.GrantAlreadyExists{}), nil
+			}
+		}
 	}
 
-	statusCode, err := g.client.AddUserToGroup(ctx, groupId, userName)
+	succeded, err := g.client.AddUserToGroup(ctx, groupId, userInfo.Name)
 	err = getError(err)
 	if err != nil {
 		return nil, err
 	}
 
-	if statusCode == http.StatusCreated {
-		l.Warn("Group Membership has been created.",
-			zap.String("userId", userId),
-			zap.String("userName", userName),
-			zap.String("group", groupId),
-		)
+	if !succeded {
+		return nil, fmt.Errorf("jira(dc)-connector: failed when granting user to group %s", groupId)
 	}
 
+	l.Warn("Group Membership has been created.",
+		zap.String("userId", userId),
+		zap.String("userName", userInfo.Name),
+		zap.String("group", groupId),
+	)
 	return nil, nil
 }
 
@@ -207,25 +221,50 @@ func (g *groupBuilder) Revoke(ctx context.Context, grant *v2.Grant) (annotations
 
 	groupId := projectResourceId.Resource
 	userId := principal.Id.Resource
-	userName, err := g.client.GetUserName(ctx, userId)
+	userInfo, err := g.client.GetUserByKey(ctx, userId)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("jira(dc)-connector: error retrieving user %s: %w", userId, err)
 	}
 
-	statusCode, err := g.client.RemoveUserFromGroup(ctx, groupId, userName)
+	// Check user's group membership
+	membershipExists := false
+	if userInfo.Groups != nil && userInfo.Groups.Size > 0 {
+		for _, group := range userInfo.Groups.Items {
+			if group.Name == groupId {
+				membershipExists = true
+				break
+			}
+		}
+	}
+	if !membershipExists {
+		l.Debug("Group Membership already revoked.",
+			zap.String("userId", userId),
+			zap.String("userName", userInfo.Name),
+			zap.String("group", groupId),
+		)
+		return annotations.New(&v2.GrantAlreadyRevoked{}), nil
+	}
+
+	success, err := g.client.RemoveUserFromGroup(ctx, groupId, userInfo.Name)
 	err = getError(err)
 	if err != nil {
 		return nil, err
 	}
 
-	if statusCode == http.StatusOK {
-		l.Warn("Group Membership has been revoked.",
+	if !success {
+		l.Error("Failed to revoke Group Membership.",
 			zap.String("groupId", groupId),
 			zap.String("userId", userId),
-			zap.String("userName", userName),
+			zap.String("userName", userInfo.Name),
 		)
+		return nil, fmt.Errorf("jira(dc)-connector: failed when revoking user from group %s", groupId)
 	}
 
+	l.Warn("Group Membership has been revoked.",
+		zap.String("groupId", groupId),
+		zap.String("userId", userId),
+		zap.String("userName", userInfo.Name),
+	)
 	return nil, nil
 }
 func newGroupBuilder(client *client.Client) *groupBuilder {
