Skip to content
This repository has been archived by the owner on Mar 11, 2021. It is now read-only.

Commit

Permalink
Add Delete user API by username to be called by the auth service (#2386)
Browse files Browse the repository at this point in the history 
* Add Delete user API by username to be called by admin-console service account

* delete assocaited space owned by an identities being deleted

* Add Delete user API by username to be called by admin-console service account

* delete assocaited space owned by an identities being deleted

* Delete user ans associated resources should be called from auth service

* make codacy happy with coding styling

* Update account/user.go

Co-Authored-By: corinnekrych <[email protected]>

* Update account/user.go

Co-Authored-By: corinnekrych <[email protected]>

* clean code with review's comments

* Update account/user.go

Co-Authored-By: corinnekrych <[email protected]>

* refactor delete user test in sub-test

* refactor delete user remove unnecessary tf.Users(1)

* refactor delete user remove to assert wuth CheckExists

* Update design/account.go

Co-Authored-By: corinnekrych <[email protected]>

* Update space/space.go

Co-Authored-By: corinnekrych <[email protected]>

* refactor space test to use nest sub-test

* refactor migration tests to use gorm.Dialect

* add test for deleting users with mutiple identities

* refactor account/user_blackbox_test.go to fail the sub-test instead of entire test

* refactor controller/users.go to group if statement

* refactor controller/users.go tests into subtest

* remove user should keep all data committed on spaces not owned by this user, whereas data in owned spaces are all deleted

* fix unit test for create wit revision, unknown modifier is allowed

* refactor

* refactor

* refactor

* PermanetDelete of a space to check for spaceId nil

* make codacy happy

* migration test to check modifier_id colunm is not dropped
  • Loading branch information
@corinnekrych
corinnekrych authored Jan 21, 2019
1 parent 8c71948 commit a9703fb
Show file tree
Hide file tree
Showing 12 changed files with 648 additions and 285 deletions.
63 changes: 61 additions & 2 deletions account/user.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@ import (
"github.com/goadesign/goa"
"github.com/jinzhu/gorm"
errs "github.com/pkg/errors"
uuid "github.com/satori/go.uuid"
"github.com/satori/go.uuid"
)

// In future, we could add support for FieldDefinitions the way we have for workitems.
// Hence. keeping the map as a string->interface and not string->string.
// At the moment, FieldDefinitions could be an overkill, so keeping it out.

// User describes a User account. A few identities can be assosiated with one user account
// User describes a User account. A few identities can be associated with one user account
type User struct {
gormsupport.Lifecycle
ID uuid.UUID `sql:"type:uuid default uuid_generate_v4()" gorm:"primary_key"` // This is the ID PK field
Expand Down Expand Up @@ -65,10 +65,12 @@ func NewUserRepository(db *gorm.DB) UserRepository {
type UserRepository interface {
repository.Exister
Load(ctx context.Context, ID uuid.UUID) (*User, error)
LoadByUsername(ctx context.Context, username string) ([]User, error)
Create(ctx context.Context, u *User) error
Save(ctx context.Context, u *User) error
List(ctx context.Context) ([]User, error)
Delete(ctx context.Context, ID uuid.UUID) error
PermanentDelete(ctx context.Context, ID uuid.UUID) error
Query(funcs ...func(*gorm.DB) *gorm.DB) ([]User, error)
}

Expand All @@ -92,6 +94,38 @@ func (m *GormUserRepository) Load(ctx context.Context, id uuid.UUID) (*User, err
return &native, errs.WithStack(err)
}

func (m *GormUserRepository) LoadByUsername(ctx context.Context, username string) ([]User, error) {
defer goa.MeasureSince([]string{"goa", "db", "user", "load_by_username"}, time.Now())
users := []User{}
rows, err := m.db.Raw("SELECT DISTINCT u.id, u.email FROM users u, identities i WHERE u.id = i.user_id AND i.username = ?", username).Rows()
defer rows.Close()
// Given there is no unicity key, we have several identities associated to different users.
// One user can have multiple identities. Only one of them is the “main” OSIO identity.
// But user can also have multiple remote identities like GitHub or Jira identities associated with the user.
// Remote identity usernames are not unique.
for rows.Next() {
var userId, email string
if err := rows.Scan(&userId, &email); err != nil {
log.Error(ctx, map[string]interface{}{
"username": username,
"err": err,
}, "invalid uuid for the user")
return nil, errs.WithStack(err)
}
userIdAsUid, err := uuid.FromString(userId)
if err != nil {
log.Error(ctx, map[string]interface{}{
"username": username,
"err": err,
}, "invalid uuid for the user")
return nil, errs.WithStack(err)
}
user := User{ID: userIdAsUid, Email: email}
users = append(users, user)
}
return users, errs.WithStack(err)
}

// CheckExists returns nil if the given ID exists otherwise returns an error
func (m *GormUserRepository) CheckExists(ctx context.Context, id uuid.UUID) error {
defer goa.MeasureSince([]string{"goa", "db", "user", "exists"}, time.Now())
Expand Down Expand Up @@ -156,6 +190,31 @@ func (m *GormUserRepository) Delete(ctx context.Context, id uuid.UUID) error {
return nil
}

// Delete removes a single record. This method use custom SQL to allow soft delete with GORM
// to coexist with permanent delete.
func (m *GormUserRepository) PermanentDelete(ctx context.Context, ID uuid.UUID) error {
defer goa.MeasureSince([]string{"goa", "db", "user", "permanent_delete"}, time.Now())
tx := m.db.Unscoped().Delete(&User{ID: ID})
if err := tx.Error; err != nil {
log.Error(ctx, map[string]interface{}{
"user_id": ID.String(),
"err": err,
}, "unable to permanently delete the user")
return errors.NewInternalError(ctx, err)
}
if tx.RowsAffected == 0 {
log.Error(ctx, map[string]interface{}{
"user_id": ID.String(),
}, "none row was affected by the permanently deletion operation")
return errors.NewNotFoundError("user", ID.String())
}
log.Debug(ctx, map[string]interface{}{
"user_id": ID,
}, "User permanently deleted!")

return nil
}

// List return all users
func (m *GormUserRepository) List(ctx context.Context) ([]User, error) {
defer goa.MeasureSince([]string{"goa", "db", "user", "list"}, time.Now())
Expand Down
130 changes: 84 additions & 46 deletions account/user_blackbox_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@ import (
"github.com/fabric8-services/fabric8-wit/errors"
"github.com/fabric8-services/fabric8-wit/gormtestsupport"
"github.com/fabric8-services/fabric8-wit/resource"
tf "github.com/fabric8-services/fabric8-wit/test/testfixture"

uuid "github.com/satori/go.uuid"
"github.com/satori/go.uuid"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
Expand All @@ -33,29 +34,72 @@ func (s *userBlackBoxTest) TestOKToDelete() {
resource.Require(t, resource.Database)

// create 2 users, where the first one would be deleted.
user := createAndLoadUser(s)
createAndLoadUser(s)
user := createAndLoadUser(s, t)
createAndLoadUser(s, t)

err := s.repo.Delete(s.Ctx, user.ID)
require.NoError(s.T(), err)
require.NoError(t, err)

// lets see how many are present.
users, err := s.repo.List(s.Ctx)
require.NoError(s.T(), err, "Could not list users")
require.True(s.T(), len(users) > 0)
require.NoError(t, err, "Could not list users")
require.True(t, len(users) > 0)

for _, data := range users {
// The user 'user' was deleted and rest were not deleted, hence we check
// that none of the user objects returned include the one deleted.
require.NotEqual(s.T(), user.ID.String(), data.ID.String())
require.NotEqual(t, user.ID.String(), data.ID.String())
}
}

func (s *userBlackBoxTest) TestOKToLoad() {
t := s.T()
resource.Require(t, resource.Database)

createAndLoadUser(s) // this function does the needful already
createAndLoadUser(s, t) // this function does the needful already
}

func (s *userBlackBoxTest) TestLoadByUsername() {
t := s.T()
resource.Require(t, resource.Database)
t.Run("load ok", func(t *testing.T) {
fxt := tf.NewTestFixture(t, s.DB, tf.Identities(1, func(fixture *tf.TestFixture, idx int) error {
fixture.Identities[idx].Username = "myusername"
return nil
}), tf.Users(1))
loadedUser, err := s.repo.LoadByUsername(s.Ctx, "myusername")
require.NoError(t, err, "Could not load user")
require.Equal(t, loadedUser[0].Email, fxt.Users[0].Email)
require.Equal(t, loadedUser[0].ID, fxt.Users[0].ID)
})
t.Run("load one user with a list of identities associated to a this user", func(t *testing.T) {
random := uuid.NewV4()
myusername := "myusername" + random.String()
fxt := tf.NewTestFixture(t, s.DB, tf.Users(1), tf.Identities(5, func(fixture *tf.TestFixture, idx int) error {
fixture.Identities[idx].Username = myusername
return nil
}))
loadedUser, err := s.repo.LoadByUsername(s.Ctx, myusername)
require.NoError(t, err, "Could not load user")
require.Equal(t, len(loadedUser), 1)
require.Equal(t, loadedUser[0].ID, fxt.Users[0].ID)
})
t.Run("load users with a list of identities associated to a those users", func(t *testing.T) {
random := uuid.NewV4()
myusername := "myusername" + random.String()
numUsers := 3
tf.NewTestFixture(t, s.DB,
tf.Users(numUsers),
tf.Identities(5, func(fixture *tf.TestFixture, idx int) error {
fixture.Identities[idx].Username = myusername
fixture.Identities[idx].User = *fixture.Users[idx%numUsers]
return nil
}),
)
loadedUser, err := s.repo.LoadByUsername(s.Ctx, myusername)
require.NoError(t, err, "Could not load user")
require.Len(t, loadedUser, numUsers)
})
}

func (s *userBlackBoxTest) TestExistsUser() {
Expand All @@ -64,7 +108,7 @@ func (s *userBlackBoxTest) TestExistsUser() {

t.Run("user exists", func(t *testing.T) {
//t.Parallel()
user := createAndLoadUser(s)
user := createAndLoadUser(s, t)
// when
err := s.repo.CheckExists(s.Ctx, user.ID)
// then
Expand All @@ -77,45 +121,39 @@ func (s *userBlackBoxTest) TestExistsUser() {
err := s.repo.CheckExists(s.Ctx, uuid.NewV4())
// then
//
require.IsType(s.T(), errors.NotFoundError{}, err)
require.IsType(t, errors.NotFoundError{}, err)
})
}

func (s *userBlackBoxTest) TestOKToSave() {
func (s *userBlackBoxTest) TestSave() {
t := s.T()
resource.Require(t, resource.Database)

user := createAndLoadUser(s)

user.FullName = "newusernameTestUser"
err := s.repo.Save(s.Ctx, user)
require.NoError(s.T(), err, "Could not update user")

updatedUser, err := s.repo.Load(s.Ctx, user.ID)
require.NoError(s.T(), err, "Could not load user")
assert.Equal(s.T(), user.FullName, updatedUser.FullName)
fields := user.ContextInformation
assert.Equal(s.T(), fields["last_visited"], "http://www.google.com")
assert.Equal(s.T(), fields["myid"], "71f343e3-2bfa-4ec6-86d4-79b91476acfc")

}

func (s *userBlackBoxTest) TestUpdateToEmptyString() {
t := s.T()
resource.Require(t, resource.Database)
user := createAndLoadUser(s)

err := s.repo.Save(s.Ctx, user)
require.NoError(t, err)
user.Bio = ""
err = s.repo.Save(s.Ctx, user)
require.NoError(t, err)
u, err := s.repo.Load(s.Ctx, user.ID)
require.NoError(t, err)
require.Empty(t, u.Bio)
user := createAndLoadUser(s, t)
t.Run("save is ok", func(t *testing.T) {
user.FullName = "newusernameTestUser"
err := s.repo.Save(s.Ctx, user)
require.NoError(t, err, "Could not update user")

updatedUser, err := s.repo.Load(s.Ctx, user.ID)
require.NoError(t, err, "Could not load user")
assert.Equal(t, user.FullName, updatedUser.FullName)
fields := user.ContextInformation
assert.Equal(t, fields["last_visited"], "http://www.google.com")
assert.Equal(t, fields["myid"], "71f343e3-2bfa-4ec6-86d4-79b91476acfc")
})
t.Run("update empty string", func(t *testing.T) {
err := s.repo.Save(s.Ctx, user)
require.NoError(t, err)
user.Bio = ""
err = s.repo.Save(s.Ctx, user)
require.NoError(t, err)
u, err := s.repo.Load(s.Ctx, user.ID)
require.NoError(t, err)
require.Empty(t, u.Bio)
})
}

func createAndLoadUser(s *userBlackBoxTest) *account.User {
func createAndLoadUser(s *userBlackBoxTest, t *testing.T) *account.User {
user := &account.User{
ID: uuid.NewV4(),
Email: "someuser@TestUser" + uuid.NewV4().String(),
Expand All @@ -131,13 +169,13 @@ func createAndLoadUser(s *userBlackBoxTest) *account.User {
}

err := s.repo.Create(s.Ctx, user)
require.NoError(s.T(), err, "Could not create user")
require.NoError(t, err, "Could not create user")

createdUser, err := s.repo.Load(s.Ctx, user.ID)
require.NoError(s.T(), err, "Could not load user")
require.Equal(s.T(), user.Email, createdUser.Email)
require.Equal(s.T(), user.ID, createdUser.ID)
require.Equal(s.T(), user.ContextInformation["last_visited"], createdUser.ContextInformation["last_visited"])
require.NoError(t, err, "Could not load user")
require.Equal(t, user.Email, createdUser.Email)
require.Equal(t, user.ID, createdUser.ID)
require.Equal(t, user.ContextInformation["last_visited"], createdUser.ContextInformation["last_visited"])

return createdUser
}
87 changes: 87 additions & 0 deletions controller/users.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,93 @@ func (c *UsersController) Obfuscate(ctx *app.ObfuscateUsersContext) error {
return ctx.OK([]byte{})
}

// Delete runs the delete action to prune all data associated with a username.
func (c *UsersController) Delete(ctx *app.DeleteUsersContext) error {
isSvcAuth, err := isServiceAccount(ctx, serviceNameAuth)
if err != nil {
log.Error(ctx, map[string]interface{}{
"err": err,
}, "failed to determine if account is a service account")
return jsonapi.JSONErrorResponse(ctx, err)

}
if !isSvcAuth {
log.Error(ctx, map[string]interface{}{
"username": ctx.Username,
}, "account used to call delete API is not a admin-console service account")
return jsonapi.JSONErrorResponse(ctx, errors.NewUnauthorizedError("account used to call delete API is not a admin-console service account"))
}
username := ctx.Username
if username == "" {
log.Error(ctx, map[string]interface{}{}, "failed to retrieve user by username")
return jsonapi.JSONErrorResponse(ctx, errors.NewBadParameterError("invalid username", ctx.Username).Expected("user name should not be empty"))
}
err = application.Transactional(c.db, func(appl application.Application) error {
// Collect all identities for a given username. Used to delete associated spaces, work_items, comments.
identities, err := appl.Identities().Query(account.IdentityFilterByUsername(username))
if err != nil || len(identities) == 0 {
log.Error(ctx, map[string]interface{}{
"username": username,
"err": err,
}, "unable to retrieve the identity associated to this user id")
return errors.NewNotFoundError("user", username)
}
// Collect all users for a given name to delete all users and identities (with on-cascade FK)
users, err := appl.Users().LoadByUsername(ctx, username)
if err != nil || len(users) == 0 {
log.Error(ctx, map[string]interface{}{
"username": username,
"err": err,
}, "unable to load the user")
return errors.NewNotFoundError("user", username)
}
// Delete all users, identities.
for _, user := range users {
if err := appl.Users().PermanentDelete(ctx, user.ID); err != nil {
log.Error(ctx, map[string]interface{}{
"user_id": user.ID,
"err": err,
}, "unable to delete the user")
return err
}
log.Debug(ctx, map[string]interface{}{
"username": username,
"user_id": user.ID,
}, "User permanently deleted!")
}
// Delete all spaces owned by this username. With on-cascade foreign keys, it will delete all wit,
// comments associated.
for _, identity := range identities {
spaces, _, err := appl.Spaces().LoadByOwner(ctx, &identity.ID, nil, nil)
if err != nil {
log.Error(ctx, map[string]interface{}{
"identity_id": identity.ID,
"err": err,
}, "unable to find spaces with this owner")
return err
}
for _, space := range spaces {
if err := appl.Spaces().PermanentDelete(ctx, space.ID); err != nil {
log.Error(ctx, map[string]interface{}{
"space_id": space.ID,
"err": err,
}, "unable to permanently delete space")
return err
}
}
log.Debug(ctx, map[string]interface{}{
"identity_id": identity.ID,
}, "Space deleted!")

}
return nil
})
if err != nil {
return jsonapi.JSONErrorResponse(ctx, err)
}
return ctx.OK([]byte{})
}

// Show runs the show action.
func (c *UsersController) Show(ctx *app.ShowUsersContext) error {
return proxy.RouteHTTP(ctx, c.config.GetAuthShortServiceHostName())
Expand Down
Loading

0 comments on commit a9703fb

Please sign in to comment.