[DDO-3890] Add models.User.DeactivatedAt (#683)

sherlock/db/migrations/000100_deactivated_users.down.sql

@@ -0,0 +1,2 @@
+alter table users
+    drop column if exists deactivated_at;

sherlock/db/migrations/000100_deactivated_users.up.sql

@@ -0,0 +1,2 @@
+alter table users
+    add column if not exists deactivated_at timestamp with time zone;

sherlock/internal/api/login/login_test.go

@@ -1,12 +1,14 @@
 package login
 
 import (
+	"github.com/broadinstitute/sherlock/go-shared/pkg/utils"
 	"github.com/broadinstitute/sherlock/sherlock/internal/oidc_models"
 	"github.com/google/uuid"
 	"github.com/zitadel/oidc/v3/pkg/oidc"
 	"gorm.io/gorm/clause"
 	"net/http"
 	"net/http/httptest"
+	"time"
 )
 
 func (s *handlerSuite) TestLoginGet_noAuthRequestID() {
@@ -56,3 +58,37 @@ func (s *handlerSuite) TestLoginGet() {
 	s.True(reloadedAuthRequest.DoneAt.Valid)
 	s.Equal(s.TestData.User_Suitable().ID, *reloadedAuthRequest.UserID)
 }
+
+func (s *handlerSuite) TestLoginGet_DeactivatedUser() {
+	clientID, _, err := s.GenerateClient(s.DB)
+	s.NoError(err)
+
+	authRequest := oidc_models.AuthRequest{
+		ID:          uuid.New(),
+		ClientID:    clientID,
+		Nonce:       "some-nonce",
+		RedirectURI: s.GeneratedClientRedirectURI(),
+		Scopes:      []string{oidc.ScopeOpenID, oidc.ScopeProfile, oidc.ScopeEmail, "groups"},
+		State:       "some-state",
+	}
+
+	s.NoError(s.DB.Omit(clause.Associations).Create(&authRequest).Error)
+
+	request, err := http.NewRequest("GET", "/login?id="+authRequest.GetID(), nil)
+	s.NoError(err)
+	s.UseSuitableUserFor(request)
+
+	// Deactivate the user right before making the request
+	s.SetUserForDB(utils.PointerTo(s.TestData.User_SuperAdmin()))
+	s.NoError(s.DB.Model(utils.PointerTo(s.TestData.User_Suitable())).Omit(clause.Associations).Update("deactivated_at", utils.PointerTo(time.Now())).Error)
+
+	recorder := httptest.NewRecorder()
+	s.internalRouter.ServeHTTP(recorder, request)
+
+	s.Equal(http.StatusForbidden, recorder.Code)
+
+	// Check that the auth request was not marked as done
+	var reloadedAuthRequest oidc_models.AuthRequest
+	s.NoError(s.DB.Where("id = ?", authRequest.ID.String()).First(&reloadedAuthRequest).Error)
+	s.False(reloadedAuthRequest.DoneAt.Valid)
+}

sherlock/internal/api/sherlock/role_assignments_v3_create_test.go

@@ -268,3 +268,16 @@ func (s *handlerSuite) TestRoleAssignmentsV3Create_defaultFalseNoSuitability() {
 	s.Equal(http.StatusCreated, code)
 	s.False(*got.Suspended)
 }
+
+func (s *handlerSuite) TestRoleAssignmentsV3Create_deactivatedUser() {
+	s.SetSelfSuperAdminForDB()
+	user := s.TestData.User_Deactivated()
+	role := models.Role{RoleFields: models.RoleFields{Name: utils.PointerTo("test-role"), SuspendNonSuitableUsers: utils.PointerTo(false)}}
+	s.NoError(s.DB.Create(&role).Error)
+	var got errors.ErrorResponse
+	code := s.HandleRequest(
+		s.NewSuperAdminRequest("POST", "/api/role-assignments/v3/"+utils.UintToString(role.ID)+"/"+utils.UintToString(user.ID), nil),
+		&got)
+	s.Equal(http.StatusForbidden, code)
+	s.Equal(errors.Forbidden, got.Type)
+}

sherlock/internal/api/sherlock/users_v3.go

@@ -3,6 +3,7 @@ package sherlock
 import (
 	"github.com/broadinstitute/sherlock/go-shared/pkg/utils"
 	"github.com/broadinstitute/sherlock/sherlock/internal/models"
+	"time"
 )
 
 type UserV3 struct {
@@ -15,6 +16,7 @@ type UserV3 struct {
 	SlackID                *string             `json:"slackID,omitempty" form:"slackID"`
 	Suitable               *bool               `json:"suitable,omitempty" form:"suitable"`                             // Available only in responses; indicates whether the user is production-suitable
 	SuitabilityDescription *string             `json:"suitabilityDescription,omitempty" form:"suitabilityDescription"` // Available only in responses; describes the user's production-suitability
+	DeactivatedAt          *time.Time          `json:"deactivatedAt,omitempty" form:"deactivatedAt"`                   // If set, indicates that the user is currently deactivated
 	Assignments            []*RoleAssignmentV3 `json:"assignments,omitempty" form:"-"`
 	userDirectlyEditableFields
 }
@@ -35,6 +37,7 @@ func (u UserV3) toModel() models.User {
 		SlackID:        u.SlackID,
 		Name:           u.Name,
 		NameFrom:       u.NameFrom,
+		DeactivatedAt:  u.DeactivatedAt,
 	}
 	return ret
 }
@@ -54,6 +57,7 @@ func userFromModel(model models.User) UserV3 {
 		GithubID:               model.GithubID,
 		SlackUsername:          model.SlackUsername,
 		SlackID:                model.SlackID,
+		DeactivatedAt:          model.DeactivatedAt,
 		Suitable:               &suitable,
 		SuitabilityDescription: &suitabilityDescription,
 		userDirectlyEditableFields: userDirectlyEditableFields{

sherlock/internal/api/sherlock/users_v3_get_test.go

@@ -8,8 +8,10 @@ import (
 	"github.com/broadinstitute/sherlock/sherlock/internal/models"
 	"github.com/stretchr/testify/assert"
 	"gorm.io/gorm"
+	"gorm.io/gorm/clause"
 	"net/http"
 	"testing"
+	"time"
 )
 
 func (s *handlerSuite) TestUserV3Get_badSelector() {
@@ -30,6 +32,23 @@ func (s *handlerSuite) TestUserV3Get_notFound() {
 	s.Equal(errors.NotFound, got.Type)
 }
 
+// TestUserV3Get_deactivated is more of a smoke test -- we actually test the code that powers this
+// where it's defined in the middleware package, and it technically applies across every single API
+// route in this package, but it's unnecessary to exhaustively test that. We just check here that
+// it's wired up correctly.
+func (s *handlerSuite) TestUserV3Get_deactivated() {
+	// Have to switch to super admin to make this user deactivated
+	s.SetUserForDB(utils.PointerTo(s.TestData.User_SuperAdmin()))
+	s.NoError(s.DB.Model(utils.PointerTo(s.TestData.User_NonSuitable())).Omit(clause.Associations).Update("deactivated_at", utils.PointerTo(time.Now())).Error)
+
+	var got errors.ErrorResponse
+	code := s.HandleRequest(
+		s.NewNonSuitableRequest("GET", "/api/users/v3/[email protected]", nil),
+		&got)
+	s.Equal(http.StatusForbidden, code)
+	s.Equal(errors.Forbidden, got.Type)
+}
+
 func (s *handlerSuite) TestUsersV3Get_self() {
 	for _, selector := range []string{"self", "me", s.TestData.User_Suitable().Email, fmt.Sprintf("google-id/%s", s.TestData.User_Suitable().GoogleID)} {
 		s.Run(fmt.Sprintf("get own user via '%s'", selector), func() {

sherlock/internal/api/sherlock/users_v3_list.go

@@ -20,6 +20,7 @@ import (
 //	@param			filter					query		UserV3	false	"Filter the returned Users"
 //	@param			limit					query		int		false	"Control how many Users are returned (default 0, no limit)"
 //	@param			offset					query		int		false	"Control the offset for the returned Users (default 0)"
+//	@param			include-deactivated		query		bool	false	"Include deactivated users in the results (default false)"
 //	@success		200						{array}		UserV3
 //	@failure		400,403,404,407,409,500	{object}	errors.ErrorResponse
 //	@router			/api/users/v3 [get]
@@ -44,11 +45,24 @@ func usersV3List(ctx *gin.Context) {
 		errors.AbortRequest(ctx, fmt.Errorf("(%s) %v", errors.BadRequest, err))
 		return
 	}
+	var includeDeactivated bool
+	if includeDeactivatedString := ctx.DefaultQuery("include-deactivated", "false"); includeDeactivatedString == "true" {
+		includeDeactivated = true
+	} else if includeDeactivatedString == "false" {
+		includeDeactivated = false
+	} else {
+		errors.AbortRequest(ctx, fmt.Errorf("(%s) couldn't parse 'include-deactivated' to a boolean", errors.BadRequest))
+		return
+	}
+
 	var results []models.User
 	chain := db.Where(&modelFilter)
 	if limit > 0 {
 		chain = chain.Limit(limit)
 	}
+	if !includeDeactivated {
+		chain = chain.Where("deactivated_at IS NULL")
+	}
 	if err = chain.
 		Offset(offset).
 		Order("email asc").

sherlock/internal/api/sherlock/users_v3_list_test.go

@@ -8,6 +8,38 @@ import (
 	"net/http"
 )
 
+func (s *handlerSuite) TestUsersV3List_includeDeactivated() {
+	var got []UserV3
+	code := s.HandleRequest(
+		s.NewRequest("GET", "/api/users/v3", nil),
+		&got)
+	s.Equal(http.StatusOK, code)
+	baseline := len(got)
+
+	s.TestData.User_Deactivated()
+	s.Run("default false", func() {
+		code = s.HandleRequest(
+			s.NewRequest("GET", "/api/users/v3", nil),
+			&got)
+		s.Equal(http.StatusOK, code)
+		s.Len(got, baseline) // no new users
+	})
+	s.Run("explicit false", func() {
+		code = s.HandleRequest(
+			s.NewRequest("GET", "/api/users/v3?include-deactivated=false", nil),
+			&got)
+		s.Equal(http.StatusOK, code)
+		s.Len(got, baseline) // no new users
+	})
+	s.Run("explicit true", func() {
+		code = s.HandleRequest(
+			s.NewRequest("GET", "/api/users/v3?include-deactivated=true", nil),
+			&got)
+		s.Equal(http.StatusOK, code)
+		s.Len(got, baseline+1) // one new user
+	})
+}
+
 func (s *handlerSuite) TestUsersV3List_minimal() {
 	var got []UserV3
 	code := s.HandleRequest(

sherlock/internal/boot/application.go

@@ -153,7 +153,7 @@ func (a *Application) Start() {
 	go firecloud_account_manager.RunManagersHourly(ctx)
 
 	go models.KeepAutoAssigningRoles(ctx, a.gormDB)
-	go models.KeepAutoExpiringRoleAssignments(ctx, a.gormDB, role_propagation.DoOnDemandPropagation)
+	go models.KeepAutoDeletingRoleAssignments(ctx, a.gormDB, role_propagation.DoOnDemandPropagation)
 
 	log.Info().Msgf("BOOT | building Gin router...")
 	gin.SetMode(gin.ReleaseMode) // gin.DebugMode can help resolve routing issues

sherlock/internal/middleware/authentication/middleware.go

@@ -25,6 +25,7 @@ func Middleware(db *gorm.DB) gin.HandlersChain {
 	return gin.HandlersChain{
 		userWhoConnectedMiddleware,
 		setGithubClaimsAndEscalateUser(db),
+		forbidDeactivatedUsers(),
 		setDatabaseWithUser(db),
 	}
 }
@@ -35,6 +36,7 @@ func TestMiddleware(db *gorm.DB, td models.TestData) gin.HandlersChain {
 	return gin.HandlersChain{
 		setUserWhoConnected(db, test_users.MakeHeaderParser(td.User_SuperAdmin(), td.User_Suitable(), td.User_NonSuitable()), authentication_method.TEST),
 		setGithubClaimsAndEscalateUser(db),
+		forbidDeactivatedUsers(),
 		setDatabaseWithUser(db),
 	}
 }

sherlock/internal/middleware/authentication/user_provider.go

@@ -90,6 +90,25 @@ func setGithubClaimsAndEscalateUser(db *gorm.DB) gin.HandlerFunc {
 	}
 }
 
+func forbidDeactivatedUsers() gin.HandlerFunc {
+	return func(ctx *gin.Context) {
+		if user, err := ShouldUseUser(ctx); err != nil {
+			errors.AbortRequest(ctx, fmt.Errorf("failed to read user to check if deactivated: %w", err))
+			return
+		} else {
+			for user != nil {
+				// We technically check DeletedAt here too for completeness but kind of soft-deletion isn't
+				// implemented at the time of writing
+				if user.DeactivatedAt != nil || user.DeletedAt.Valid {
+					errors.AbortRequest(ctx, fmt.Errorf("(%s) user %s is deactivated within Sherlock", errors.Forbidden, user.Email))
+					return
+				}
+				user = user.Via
+			}
+		}
+	}
+}
+
 // ShouldUseUser returns a non-nil *models.User who made the request, or an error if that isn't possible.
 func ShouldUseUser(ctx *gin.Context) (*models.User, error) {
 	userValue, exists := ctx.Get(ctxUserFieldName)

sherlock/internal/middleware/authentication/user_provider_test.go

@@ -0,0 +1,83 @@
+package authentication
+
+import (
+	"github.com/broadinstitute/sherlock/go-shared/pkg/utils"
+	"github.com/broadinstitute/sherlock/sherlock/internal/config"
+	"github.com/broadinstitute/sherlock/sherlock/internal/models"
+	"github.com/gin-gonic/gin"
+	"github.com/stretchr/testify/assert"
+	"gorm.io/gorm"
+	"net/http/httptest"
+	"testing"
+	"time"
+)
+
+func Test_forbidDeactivatedUsers(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	config.LoadTestConfig()
+	tests := []struct {
+		name        string
+		user        *models.User
+		expectError bool
+	}{
+		{
+			name:        "nil",
+			user:        nil,
+			expectError: true,
+		},
+		{
+			name:        "active",
+			user:        &models.User{},
+			expectError: false,
+		},
+		{
+			name: "deactivated",
+			user: &models.User{
+				DeactivatedAt: utils.PointerTo(time.Now()),
+			},
+			expectError: true,
+		},
+		{
+			name: "deleted",
+			user: &models.User{
+				Model: gorm.Model{DeletedAt: gorm.DeletedAt{Time: time.Now(), Valid: true}},
+			},
+			expectError: true,
+		},
+		{
+			name: "via active",
+			user: &models.User{
+				Via: &models.User{},
+			},
+		},
+		{
+			name: "via deactivated",
+			user: &models.User{
+				Via: &models.User{
+					DeactivatedAt: utils.PointerTo(time.Now()),
+					Via:           &models.User{},
+				},
+			},
+			expectError: true,
+		},
+		{
+			name: "via deleted",
+			user: &models.User{
+				Via: &models.User{
+					Model: gorm.Model{DeletedAt: gorm.DeletedAt{Time: time.Now(), Valid: true}},
+					Via:   &models.User{},
+				},
+			},
+			expectError: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			recorder := httptest.NewRecorder()
+			ctx, _ := gin.CreateTestContext(recorder)
+			ctx.Set(ctxUserFieldName, tt.user)
+			forbidDeactivatedUsers()(ctx)
+			assert.Equal(t, tt.expectError, ctx.IsAborted())
+		})
+	}
+}

sherlock/internal/models/role_assignment.go

@@ -178,11 +178,26 @@ func (ra *RoleAssignment) errorIfForbidden(tx *gorm.DB) error {
 	}
 }
 
+// errorIfUserDeactivated does what it says on the tin, but we don't want to call it often. We
+// only want to forbid creation of role assignments with deactivated users, because we want to
+// rely on
+func (ra *RoleAssignment) errorIfUserDeactivated(tx *gorm.DB) error {
+	var user User
+	if err := tx.First(&user, ra.UserID).Error; err != nil {
+		return fmt.Errorf("failed to find user to determine if it is deactivated: %w", err)
+	} else if user.DeactivatedAt != nil {
+		return fmt.Errorf("(%s) cannot create role assignment targeting deactivated user", errors.Forbidden)
+	}
+	return nil
+}
+
 func (ra *RoleAssignment) AfterCreate(tx *gorm.DB) error {
 	if user, err := GetCurrentUserForDB(tx); err != nil {
 		return err
 	} else if err = ra.errorIfForbidden(tx); err != nil {
 		return err
+	} else if err = ra.errorIfUserDeactivated(tx); err != nil {
+		return err
 	} else if err = tx.Omit(clause.Associations).Create(&RoleAssignmentOperation{
 		RoleID:    ra.RoleID,
 		UserID:    ra.UserID,