From 69288ae83f11d5f2c5db20ee8779aa1e3bde920e Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Fri, 16 Aug 2024 00:55:30 +0000 Subject: [PATCH 1/2] Enable admins to forcefully unlock deployments --- deploy/lock.go | 2 +- slack.go | 26 ++++++++----------- slack_test.go | 42 +++++++++++++++++++++++++----- user.go | 46 ++++++++++++++++++++++++++++----- user_test.go | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 158 insertions(+), 28 deletions(-) create mode 100644 user_test.go diff --git a/deploy/lock.go b/deploy/lock.go index ba6c494..f20e154 100644 --- a/deploy/lock.go +++ b/deploy/lock.go @@ -190,7 +190,7 @@ type NotAllowedTounlockError struct { } func (e NotAllowedTounlockError) Error() string { - return fmt.Sprintf("user %s is not allowed to unlock", e.User) + return fmt.Sprintf("user %s is not allowed to unlock this project", e.User) } func newNotAllowedToUnlockError(user string) NotAllowedTounlockError { diff --git a/slack.go b/slack.go index 55eae26..b383d50 100644 --- a/slack.go +++ b/slack.go @@ -4,7 +4,6 @@ import ( "bytes" "context" "encoding/json" - "errors" "fmt" "log" "net/http" @@ -260,11 +259,13 @@ func createDeployButtonSection(pj DeployProject, phaseName string) *slack.Sectio func (s *SlackListener) runCommand(cmd slackcmd.Command, triggeredBy string, replyIn string) error { var msgOpt slack.MsgOption + user := s.userList.FindBySlackUserID(triggeredBy) + switch cmd := cmd.(type) { case *slackcmd.Lock: - msgOpt = s.lock(cmd, triggeredBy, replyIn) + msgOpt = s.lock(cmd, user, replyIn) case *slackcmd.Unlock: - msgOpt = s.unlock(cmd, triggeredBy, replyIn, false) + msgOpt = s.unlock(cmd, user, replyIn, false) default: panic("unreachable") } @@ -277,12 +278,12 @@ func (s *SlackListener) runCommand(cmd slackcmd.Command, triggeredBy string, rep } // lock locks the given project and environment, and replies to the given channel. -func (s *SlackListener) lock(cmd *slackcmd.Lock, triggeredBy string, replyIn string) slack.MsgOption { +func (s *SlackListener) lock(cmd *slackcmd.Lock, triggeredBy User, replyIn string) slack.MsgOption { if err := s.validateProjectEnvUser(cmd.Project, cmd.Env, triggeredBy, replyIn); err != nil { return s.errorMessage(err.Error()) } - if err := s.getOrCreateCoordinator().Lock(context.Background(), cmd.Project, cmd.Env, triggeredBy, cmd.Reason); err != nil { + if err := s.getOrCreateCoordinator().Lock(context.Background(), cmd.Project, cmd.Env, triggeredBy.SlackUserID, cmd.Reason); err != nil { return s.errorMessage(err.Error()) } @@ -290,19 +291,19 @@ func (s *SlackListener) lock(cmd *slackcmd.Lock, triggeredBy string, replyIn str } // unlock unlocks the given project and environment, and replies to the given channel. -func (s *SlackListener) unlock(cmd *slackcmd.Unlock, triggeredBy string, replyIn string, force bool) slack.MsgOption { +func (s *SlackListener) unlock(cmd *slackcmd.Unlock, triggeredBy User, replyIn string, force bool) slack.MsgOption { if err := s.validateProjectEnvUser(cmd.Project, cmd.Env, triggeredBy, replyIn); err != nil { return s.errorMessage(err.Error()) } - if err := s.getOrCreateCoordinator().Unlock(context.Background(), cmd.Project, cmd.Env, triggeredBy, force); err != nil { + if err := s.getOrCreateCoordinator().Unlock(context.Background(), cmd.Project, cmd.Env, triggeredBy.SlackUserID, triggeredBy.IsAdmin()); err != nil { return s.errorMessage(err.Error()) } return s.infoMessage(fmt.Sprintf("Unlocked %s %s", cmd.Project, cmd.Env)) } -func (s *SlackListener) validateProjectEnvUser(projectID, env, userID, replyIn string) error { +func (s *SlackListener) validateProjectEnvUser(projectID, env string, user User, replyIn string) error { pj, err := s.projectList.FindByAlias(projectID) if err != nil { log.Println("[ERROR] ", err) @@ -321,13 +322,8 @@ func (s *SlackListener) validateProjectEnvUser(projectID, env, userID, replyIn s return fmt.Errorf("find phase %q: %w", env, err) } - if user := s.userList.FindBySlackUserID(userID); !user.IsDeveloper() { - err = errors.New("you are not allowed to lock this project") - log.Println("[ERROR] ", err) - if _, _, err := s.client.PostMessage(replyIn, s.errorMessage(err.Error())); err != nil { - log.Println("[ERROR] ", err) - } - return fmt.Errorf("find by slack user id %q: %w", userID, err) + if !user.IsDeveloper() { + return fmt.Errorf("you are not allowed to lock/unlock projects: slack user id %q", user.SlackUserID) } return nil diff --git a/slack_test.go b/slack_test.go index 5fce76c..e9855ef 100644 --- a/slack_test.go +++ b/slack_test.go @@ -63,8 +63,10 @@ var rolebindingConfigMap = corev1.ConfigMap{ }, Data: map[string]string{ "Developer": fmt.Sprintf(`%s -user3 +user2 +user4 `, user1SlackDisplayName), + "Admin": "user4", }, } @@ -102,7 +104,7 @@ func TestSlackLockUnlock(t *testing.T) { }) // List users c.Handle("/users.list", func(w http.ResponseWriter, r *http.Request) { - if _, err := w.Write([]byte(`{"ok": true, "members": [{"id": "U1234", "name": "User 1", "profile": {"display_name": "user1"}}]}`)); err != nil { + if _, err := w.Write([]byte(`{"ok": true, "members": [{"id": "U1234", "name": "User 1", "profile": {"display_name": "user1"}}, {"id": "U1235", "name": "User 2", "profile": {"display_name": "user2"}}, {"id": "U1237", "name": "User 4", "profile": {"display_name": "user4"}}]}`)); err != nil { t.Logf("failed to write response: %v", err) } }) @@ -219,7 +221,7 @@ func TestSlackLockUnlock(t *testing.T) { Channel: "C1234", Text: "lock myproject1 production for deployment of revision a", })) - require.Equal(t, "deployment is locked", nextMessage().Text()) + require.Equal(t, "deployment is already locked", nextMessage().Text()) require.NoError(t, l.handleMessageEvent(&slackevents.AppMentionEvent{ User: "U1234", @@ -235,19 +237,47 @@ func TestSlackLockUnlock(t *testing.T) { })) require.Equal(t, "deployment is already unlocked", nextMessage().Text()) + // + // We assume that any users who aren't visible from the Slack API are not allowed to lock/unlock projects. + // + require.NoError(t, l.handleMessageEvent(&slackevents.AppMentionEvent{ - User: "U1235", + User: "U1236", Channel: "C1234", Text: "lock myproject1 production for deployment of revision a", })) - require.Equal(t, "you are not allowed to lock this project", nextMessage().Text()) + require.Equal(t, "you are not allowed to lock/unlock projects: slack user id \"\"", nextMessage().Text()) + + require.NoError(t, l.handleMessageEvent(&slackevents.AppMentionEvent{ + User: "U1236", + Channel: "C1234", + Text: "unlock myproject1 production", + })) + require.Equal(t, "you are not allowed to lock/unlock projects: slack user id \"\"", nextMessage().Text()) + // User 2 is a developer so can lock the project require.NoError(t, l.handleMessageEvent(&slackevents.AppMentionEvent{ User: "U1235", Channel: "C1234", + Text: "lock myproject1 production for deployment of revision a", + })) + require.Equal(t, "Locked myproject1 production", nextMessage().Text()) + + // User 1 is a developer so cannot unlock the project forcefully + require.NoError(t, l.handleMessageEvent(&slackevents.AppMentionEvent{ + User: "U1234", + Channel: "C1234", + Text: "unlock myproject1 production", + })) + require.Equal(t, "user U1234 is not allowed to unlock this project", nextMessage().Text()) + + // User 4 is an admin and can unlock the project forcefully + require.NoError(t, l.handleMessageEvent(&slackevents.AppMentionEvent{ + User: "U1237", + Channel: "C1234", Text: "unlock myproject1 production", })) - require.Equal(t, "find by slack user id \"U1235\": you are not allowed to lock this project", nextMessage().Text()) + require.Equal(t, "Unlocked myproject1 production", nextMessage().Text()) } // Message is a message posted to the fake Slack API's chat.postMessage endpoint diff --git a/user.go b/user.go index b0f23e3..469d642 100644 --- a/user.go +++ b/user.go @@ -5,6 +5,20 @@ import ( "strings" "github.com/slack-go/slack" + v1 "k8s.io/api/core/v1" +) + +// Role is a type to represent a role of a user. +// +// A user can have multiple roles. +// But we currently assume any Admin users are also Developer users. +// So, a consumer of this type should be able to check if a user has an equal or higher role than +// Developer by checking if the user has the Developer role. +type Role string + +const ( + RoleDeveloper Role = "Developer" + RoleAdmin Role = "Admin" ) type User struct { @@ -13,12 +27,20 @@ type User struct { GitHubUserName string GitHubNodeID string isDeveloper bool + isAdmin bool } func (u User) IsDeveloper() bool { return u.isDeveloper } +// IsAdmin returns a flag to indicate whether the user is an admin or not. +// An admin can force-unlock a project/phase using `@bot unlock` command, +// even if the project/phase is locked by other users. +func (u User) IsAdmin() bool { + return u.isAdmin +} + type UserList struct { Items []User github GitHub @@ -42,6 +64,7 @@ func (ul *UserList) Reload() { cml := getConfigMapList("githubuser-mapping") rolebindings := getConfigMapList("rolebinding") + userNamesInGroups := ul.createUserNamesInGroups(rolebindings) for _, slackUser := range slackUsers { if slackUser.IsBot || slackUser.Deleted { @@ -55,18 +78,29 @@ func (ul *UserList) Reload() { } } user.GitHubNodeID = githubUsers[user.GitHubUserName] - for _, rolebinding := range rolebindings.Items { - raw := rolebinding.Data["Developer"] + _, user.isDeveloper = userNamesInGroups[RoleDeveloper][user.SlackDisplayName] + _, user.isAdmin = userNamesInGroups[RoleAdmin][user.SlackDisplayName] + ul.Items = append(ul.Items, user) + } +} + +func (ul UserList) createUserNamesInGroups(rolebindings *v1.ConfigMapList) map[Role]map[string]struct{} { + userNamesInGroups := map[Role]map[string]struct{}{ + RoleDeveloper: {}, + RoleAdmin: {}, + } + for _, rolebinding := range rolebindings.Items { + for group, users := range userNamesInGroups { + raw := rolebinding.Data[string(group)] userNames := strings.Split(raw, "\n") for _, userName := range userNames { - if user.SlackDisplayName == userName { - user.isDeveloper = true - break + if userName != "" { + users[userName] = struct{}{} } } } - ul.Items = append(ul.Items, user) } + return userNamesInGroups } func (ul UserList) FindBySlackUserID(slackUserID string) User { diff --git a/user_test.go b/user_test.go new file mode 100644 index 0000000..655f6ba --- /dev/null +++ b/user_test.go @@ -0,0 +1,70 @@ +package main + +import ( + "testing" + + "github.com/stretchr/testify/require" + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestCreateUserNamesInGroups(t *testing.T) { + var ( + ul UserList + + rolebindings = &v1.ConfigMapList{ + Items: []v1.ConfigMap{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "rolebinding1", + Labels: map[string]string{ + "gocat.zaim.net/configmap-type": "rolebinding", + }, + }, + Data: map[string]string{ + "Developer": "user1\nuser2\n", + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "rolebinding2", + Labels: map[string]string{ + "gocat.zaim.net/configmap-type": "rolebinding", + }, + }, + Data: map[string]string{ + "Admin": "user3\nuser4", + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "rolebinding3", + Labels: map[string]string{ + "gocat.zaim.net/configmap-type": "rolebinding", + }, + }, + Data: map[string]string{ + "Developer": "user5", + "Admin": "user6", + }, + }, + }, + } + ) + + userNamesInGroups := ul.createUserNamesInGroups(rolebindings) + + if len(userNamesInGroups) != 2 { + t.Fatalf("userNamesInGroups length is not 2") + } + + require.Equal(t, + map[string]struct{}{"user1": {}, "user2": {}, "user5": {}}, + userNamesInGroups[RoleDeveloper], + ) + + require.Equal(t, + map[string]struct{}{"user3": {}, "user4": {}, "user6": {}}, + userNamesInGroups[RoleAdmin], + ) +} From 52cf9623fbdb580d93c6a7c8a856009e98a3286d Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Mon, 12 Aug 2024 05:45:30 +0000 Subject: [PATCH 2/2] Rename ErrLocked to ErrAlreadyUnlocked --- deploy/lock.go | 4 ++-- deploy/lock_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/deploy/lock.go b/deploy/lock.go index f20e154..1178ff2 100644 --- a/deploy/lock.go +++ b/deploy/lock.go @@ -38,7 +38,7 @@ func NewCoordinator(ns, configMap string) *Coordinator { } } -var ErrLocked = fmt.Errorf("deployment is locked") +var ErrAlreadyLocked = fmt.Errorf("deployment is already locked") var ErrAlreadyUnlocked = fmt.Errorf("deployment is already unlocked") const ( @@ -82,7 +82,7 @@ func (c *Coordinator) lock(ctx context.Context, project, environment, user, reas } if value.Locked { - return ErrLocked + return ErrAlreadyLocked } if n := len(value.History); n >= MaxHistoryItems { diff --git a/deploy/lock_test.go b/deploy/lock_test.go index 9b0c12f..162b7c9 100644 --- a/deploy/lock_test.go +++ b/deploy/lock_test.go @@ -34,8 +34,8 @@ func TestLockUnlock(t *testing.T) { ctx := context.Background() require.NoError(t, c.Lock(ctx, "myproject1", "prod", "user1", "for deployment of revision a")) - require.ErrorIs(t, c.Lock(ctx, "myproject1", "prod", "user1", "for deployment of revision b"), ErrLocked) - require.ErrorIs(t, c.Lock(ctx, "myproject1", "prod", "user2", "for deployment of revision b"), ErrLocked) + require.ErrorIs(t, c.Lock(ctx, "myproject1", "prod", "user1", "for deployment of revision b"), ErrAlreadyLocked) + require.ErrorIs(t, c.Lock(ctx, "myproject1", "prod", "user2", "for deployment of revision b"), ErrAlreadyLocked) require.ErrorIs(t, c.Unlock(ctx, "myproject1", "prod", "user2", false), newNotAllowedToUnlockError("user2")) require.NoError(t, c.Unlock(ctx, "myproject1", "prod", "user2", true)) require.ErrorIs(t, c.Unlock(ctx, "myproject1", "prod", "user1", false), ErrAlreadyUnlocked)