Skip to content

Commit

Permalink
Restricted users (go-gitea#4334): initial implementation
Browse files Browse the repository at this point in the history
* Add User.IsRestricted & UI to edit it

* Pass user object instead of user id to places where IsRestricted flag matters

* Restricted users: maintain access rows for all referenced repos (incl public)

* Take logged in user & IsRestricted flag into account in org/repo listings, searches and accesses

* Add basic repo access tests for restricted users

Signed-off-by: Manush Dodunekov <manush@stendahls.se>
  • Loading branch information
mnsh committed Sep 2, 2019
1 parent 5fc93b4 commit 9335044
Show file tree
Hide file tree
Showing 27 changed files with 278 additions and 87 deletions.
40 changes: 27 additions & 13 deletions models/access.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,17 @@ type Access struct {
Mode AccessMode
}

func accessLevel(e Engine, userID int64, repo *Repository) (AccessMode, error) {
func accessLevel(e Engine, user *User, repo *Repository) (AccessMode, error) {
mode := AccessModeNone
if !repo.IsPrivate {
var userID int64
restricted := false

if user != nil {
userID = user.ID
restricted = user.IsRestricted
}

if !restricted && !repo.IsPrivate {
mode = AccessModeRead
}

Expand Down Expand Up @@ -163,17 +171,23 @@ func maxAccessMode(modes ...AccessMode) AccessMode {
}

// FIXME: do cross-comparison so reduce deletions and additions to the minimum?
func (repo *Repository) refreshAccesses(e Engine, accessMap map[int64]AccessMode) (err error) {
func (repo *Repository) refreshAccesses(e Engine, accessMap map[*User]AccessMode) (err error) {
minMode := AccessModeRead
if !repo.IsPrivate {
minMode = AccessModeWrite
}

newAccesses := make([]Access, 0, len(accessMap))
for userID, mode := range accessMap {
if mode < minMode {
// merge accessMap contents into a single ID-keyed map to eliminate duplicates
idMap := make(map[int64]AccessMode, len(accessMap))
for user, mode := range accessMap {
if mode < minMode && !user.IsRestricted {
continue
}
idMap[user.ID] = maxAccessMode(idMap[user.ID], mode)
}

newAccesses := make([]Access, 0, len(idMap))
for userID, mode := range idMap {
newAccesses = append(newAccesses, Access{
UserID: userID,
RepoID: repo.ID,
Expand All @@ -191,13 +205,13 @@ func (repo *Repository) refreshAccesses(e Engine, accessMap map[int64]AccessMode
}

// refreshCollaboratorAccesses retrieves repository collaborations with their access modes.
func (repo *Repository) refreshCollaboratorAccesses(e Engine, accessMap map[int64]AccessMode) error {
collaborations, err := repo.getCollaborations(e)
func (repo *Repository) refreshCollaboratorAccesses(e Engine, accessMap map[*User]AccessMode) error {
collaborators, err := repo.getCollaborators(e)
if err != nil {
return fmt.Errorf("getCollaborations: %v", err)
}
for _, c := range collaborations {
accessMap[c.UserID] = c.Mode
for _, c := range collaborators {
accessMap[c.User] = maxAccessMode(accessMap[c.User], c.Collaboration.Mode)
}
return nil
}
Expand All @@ -206,7 +220,7 @@ func (repo *Repository) refreshCollaboratorAccesses(e Engine, accessMap map[int6
// except the team whose ID is given. It is used to assign a team ID when
// remove repository from that team.
func (repo *Repository) recalculateTeamAccesses(e Engine, ignTeamID int64) (err error) {
accessMap := make(map[int64]AccessMode, 20)
accessMap := make(map[*User]AccessMode, 20)

if err = repo.getOwner(e); err != nil {
return err
Expand Down Expand Up @@ -239,7 +253,7 @@ func (repo *Repository) recalculateTeamAccesses(e Engine, ignTeamID int64) (err
return fmt.Errorf("getMembers '%d': %v", t.ID, err)
}
for _, m := range t.Members {
accessMap[m.ID] = maxAccessMode(accessMap[m.ID], t.Authorize)
accessMap[m] = maxAccessMode(accessMap[m], t.Authorize)
}
}

Expand All @@ -251,7 +265,7 @@ func (repo *Repository) recalculateAccesses(e Engine) error {
return repo.recalculateTeamAccesses(e, 0)
}

accessMap := make(map[int64]AccessMode, 20)
accessMap := make(map[*User]AccessMode, 20)
if err := repo.refreshCollaboratorAccesses(e, accessMap); err != nil {
return fmt.Errorf("refreshCollaboratorAccesses: %v", err)
}
Expand Down
50 changes: 50 additions & 0 deletions models/access_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,20 @@ func TestAccessLevel(t *testing.T) {

user2 := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
user5 := AssertExistsAndLoadBean(t, &User{ID: 5}).(*User)
user24 := AssertExistsAndLoadBean(t, &User{ID: 24}).(*User)
// A public repository owned by User 2
repo1 := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
assert.False(t, repo1.IsPrivate)
// A private repository owned by Org 3
repo3 := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository)
assert.True(t, repo3.IsPrivate)

// Another public repository
repo4 := AssertExistsAndLoadBean(t, &Repository{ID: 4}).(*Repository)
assert.False(t, repo4.IsPrivate)
// org. owned private repo
repo24 := AssertExistsAndLoadBean(t, &Repository{ID: 24}).(*Repository)

level, err := AccessLevel(user2, repo1)
assert.NoError(t, err)
assert.Equal(t, AccessModeOwner, level)
Expand All @@ -37,6 +44,21 @@ func TestAccessLevel(t *testing.T) {
level, err = AccessLevel(user5, repo3)
assert.NoError(t, err)
assert.Equal(t, AccessModeNone, level)

// restricted user has no access to a public repo
level, err = AccessLevel(user24, repo1)
assert.NoError(t, err)
assert.Equal(t, AccessModeNone, level)

// ... unless he's a collaborator
level, err = AccessLevel(user24, repo4)
assert.NoError(t, err)
assert.Equal(t, AccessModeWrite, level)

// ... or a team member
level, err = AccessLevel(user24, repo24)
assert.NoError(t, err)
assert.Equal(t, AccessModeRead, level)
}

func TestHasAccess(t *testing.T) {
Expand Down Expand Up @@ -72,6 +94,11 @@ func TestUser_GetRepositoryAccesses(t *testing.T) {
accesses, err := user1.GetRepositoryAccesses()
assert.NoError(t, err)
assert.Len(t, accesses, 0)

user24 := AssertExistsAndLoadBean(t, &User{ID: 24}).(*User)
accesses, err = user24.GetRepositoryAccesses()
assert.NoError(t, err)
assert.Len(t, accesses, 2)
}

func TestUser_GetAccessibleRepositories(t *testing.T) {
Expand All @@ -86,6 +113,11 @@ func TestUser_GetAccessibleRepositories(t *testing.T) {
repos, err = user2.GetAccessibleRepositories(0)
assert.NoError(t, err)
assert.Len(t, repos, 1)

user24 := AssertExistsAndLoadBean(t, &User{ID: 24}).(*User)
repos, err = user24.GetAccessibleRepositories(0)
assert.NoError(t, err)
assert.Len(t, repos, 2)
}

func TestRepository_RecalculateAccesses(t *testing.T) {
Expand Down Expand Up @@ -119,3 +151,21 @@ func TestRepository_RecalculateAccesses2(t *testing.T) {
assert.NoError(t, err)
assert.False(t, has)
}

func TestRepository_RecalculateAccesses3(t *testing.T) {
assert.NoError(t, PrepareTestDatabase())
team5 := AssertExistsAndLoadBean(t, &Team{ID: 5}).(*Team)
user24 := AssertExistsAndLoadBean(t, &User{ID: 24}).(*User)

has, err := x.Get(&Access{UserID: 24, RepoID: 23})
assert.NoError(t, err)
assert.False(t, has)

// adding user24 to team5 should add an explicit access row for repo 23
// even though repo 23 is public
assert.NoError(t, AddTeamMember(team5, user24.ID))

has, err = x.Get(&Access{UserID: 24, RepoID: 23})
assert.NoError(t, err)
assert.True(t, has)
}
20 changes: 14 additions & 6 deletions models/action.go
Original file line number Diff line number Diff line change
Expand Up @@ -788,20 +788,26 @@ func MirrorSyncDeleteAction(repo *Repository, refName string) error {

// GetFeedsOptions options for retrieving feeds
type GetFeedsOptions struct {
RequestedUser *User
RequestingUserID int64
IncludePrivate bool // include private actions
OnlyPerformedBy bool // only actions performed by requested user
IncludeDeleted bool // include deleted actions
RequestedUser *User // the user we want activity for
Actor *User // the user viewing the activity
IncludePrivate bool // include private actions
OnlyPerformedBy bool // only actions performed by requested user
IncludeDeleted bool // include deleted actions
}

// GetFeeds returns actions according to the provided options
func GetFeeds(opts GetFeedsOptions) ([]*Action, error) {
cond := builder.NewCond()

var repoIDs []int64
var actorID int64

if opts.Actor != nil {
actorID = opts.Actor.ID
}

if opts.RequestedUser.IsOrganization() {
env, err := opts.RequestedUser.AccessibleReposEnv(opts.RequestingUserID)
env, err := opts.RequestedUser.AccessibleReposEnv(actorID)
if err != nil {
return nil, fmt.Errorf("AccessibleReposEnv: %v", err)
}
Expand All @@ -810,6 +816,8 @@ func GetFeeds(opts GetFeedsOptions) ([]*Action, error) {
}

cond = cond.And(builder.In("repo_id", repoIDs))
} else if opts.Actor != nil {
cond = cond.And(builder.In("repo_id", opts.Actor.AccessibleRepoIDsQuery()))
}

cond = cond.And(builder.Eq{"user_id": opts.RequestedUser.ID})
Expand Down
40 changes: 20 additions & 20 deletions models/action_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -489,11 +489,11 @@ func TestGetFeeds(t *testing.T) {
user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)

actions, err := GetFeeds(GetFeedsOptions{
RequestedUser: user,
RequestingUserID: user.ID,
IncludePrivate: true,
OnlyPerformedBy: false,
IncludeDeleted: true,
RequestedUser: user,
Actor: user,
IncludePrivate: true,
OnlyPerformedBy: false,
IncludeDeleted: true,
})
assert.NoError(t, err)
if assert.Len(t, actions, 1) {
Expand All @@ -502,10 +502,10 @@ func TestGetFeeds(t *testing.T) {
}

actions, err = GetFeeds(GetFeedsOptions{
RequestedUser: user,
RequestingUserID: user.ID,
IncludePrivate: false,
OnlyPerformedBy: false,
RequestedUser: user,
Actor: user,
IncludePrivate: false,
OnlyPerformedBy: false,
})
assert.NoError(t, err)
assert.Len(t, actions, 0)
Expand All @@ -515,14 +515,14 @@ func TestGetFeeds2(t *testing.T) {
// test with an organization user
assert.NoError(t, PrepareTestDatabase())
org := AssertExistsAndLoadBean(t, &User{ID: 3}).(*User)
const userID = 2 // user2 is an owner of the organization
user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)

actions, err := GetFeeds(GetFeedsOptions{
RequestedUser: org,
RequestingUserID: userID,
IncludePrivate: true,
OnlyPerformedBy: false,
IncludeDeleted: true,
RequestedUser: org,
Actor: user,
IncludePrivate: true,
OnlyPerformedBy: false,
IncludeDeleted: true,
})
assert.NoError(t, err)
assert.Len(t, actions, 1)
Expand All @@ -532,11 +532,11 @@ func TestGetFeeds2(t *testing.T) {
}

actions, err = GetFeeds(GetFeedsOptions{
RequestedUser: org,
RequestingUserID: userID,
IncludePrivate: false,
OnlyPerformedBy: false,
IncludeDeleted: true,
RequestedUser: org,
Actor: user,
IncludePrivate: false,
OnlyPerformedBy: false,
IncludeDeleted: true,
})
assert.NoError(t, err)
assert.Len(t, actions, 0)
Expand Down
14 changes: 13 additions & 1 deletion models/fixtures/access.yml
Original file line number Diff line number Diff line change
Expand Up @@ -74,4 +74,16 @@
id: 13
user_id: 20
repo_id: 28
mode: 4 # owner
mode: 4 # owner

-
id: 14
user_id: 22
repo_id: 4
mode: 2 # write (collaborator)

-
id: 15
user_id: 22
repo_id: 24
mode: 1 # read
8 changes: 7 additions & 1 deletion models/fixtures/collaboration.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,10 @@
id: 3
repo_id: 40
user_id: 4
mode: 2 # write
mode: 2 # write

-
id: 4
repo_id: 4
user_id: 26
mode: 2 # write
6 changes: 6 additions & 0 deletions models/fixtures/org_user.yml
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,9 @@
uid: 24
org_id: 25
is_public: true

-
id: 9
uid: 26
org_id: 17
is_public: true
2 changes: 1 addition & 1 deletion models/fixtures/team.yml
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@
name: review_team
authorize: 1 # read
num_repos: 1
num_members: 1
num_members: 2

-
id: 10
Expand Down
6 changes: 6 additions & 0 deletions models/fixtures/team_user.yml
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,9 @@
org_id: 25
team_id: 10
uid: 24

-
id: 13
org_id: 17
team_id: 9
uid: 26
18 changes: 17 additions & 1 deletion models/fixtures/user.yml
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@
avatar_email: user17@example.com
num_repos: 2
is_active: true
num_members: 2
num_members: 3
num_teams: 3

-
Expand Down Expand Up @@ -410,3 +410,19 @@
num_repos: 0
num_members: 1
num_teams: 1

-
id: 26
lower_name: user26
name: user26
full_name: User 26
email: user26@example.com
passwd: 7d93daa0d1e6f2305cc8fa496847d61dc7320bb16262f9c55dd753480207234cdd96a93194e408341971742f4701772a025a # password
type: 0 # individual
salt: ZogKvWdyEx
is_admin: false
is_restricted: true
avatar: avatar26
avatar_email: user26@example.com
num_repos: 0
is_active: true
2 changes: 2 additions & 0 deletions models/migrations/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,8 @@ var migrations = []Migration{
NewMigration("remove orphaned repository index statuses", removeLingeringIndexStatus),
// v93 -> v94
NewMigration("add email notification enabled preference to user", addEmailNotificationEnabledToUser),
// v94 -> v95
NewMigration("add is_restricted column for users table", addIsRestricted),
}

// Migrate database to current version
Expand Down
Loading

0 comments on commit 9335044

Please sign in to comment.