Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add team option to grant rights for all organization repositories #6791

Closed
wants to merge 13 commits into from

Conversation

ngourdon
Copy link
Contributor

  • add new option to team
  • use the new option for owner team to unify the processing
  • grant access to members
  • update team API

Fix #6409 and #2654

related PR go-gitea/go-sdk#166

models/migrations/v85.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 28, 2019
models/migrations/v85.go Outdated Show resolved Hide resolved
models/org_team.go Outdated Show resolved Hide resolved
@lafriks lafriks added the type/enhancement An improvement of existing functionality label Apr 28, 2019
@lafriks lafriks added this to the 1.9.0 milestone Apr 28, 2019
@lafriks lafriks added the type/changelog Adds the changelog for a new Gitea version label Apr 28, 2019
models/org_team.go Show resolved Hide resolved
@ngourdon ngourdon changed the title Add team option to grant rights for all organization repositories [WIP] Add team option to grant rights for all organization repositories May 7, 2019
@ngourdon
Copy link
Contributor Author

ngourdon commented May 9, 2019

Behaviour is the same for Owners teams and teams which includes all repositories.
I added a handling in TransferOwnership which was missed.
All repositories are still added to teams which includes all repositories because it is how it is for Owners teams and some functions won't work without it (like access.go:recalculateTeamAccesses or org_team.go:AddTeamMember.
Ready for review.

@ngourdon ngourdon changed the title [WIP] Add team option to grant rights for all organization repositories Add team option to grant rights for all organization repositories May 9, 2019
@lafriks
Copy link
Member

lafriks commented May 9, 2019

Drone fails

@ngourdon
Copy link
Contributor Author

ngourdon commented May 9, 2019

There's still a change in the gitea sdk to add the new field IncludesAllRepositories in the API.
This fails the task test-vendor in the CI build.

There is a PR go-gitea/go-sdk#166 opened for this change.

@codecov-io
Copy link

codecov-io commented May 12, 2019

Codecov Report

Merging #6791 into master will increase coverage by 0.01%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #6791      +/-   ##
=========================================
+ Coverage   41.19%   41.2%   +0.01%     
=========================================
  Files         469     470       +1     
  Lines       63536   63591      +55     
=========================================
+ Hits        26171   26205      +34     
- Misses      33943   33960      +17     
- Partials     3422    3426       +4
Impacted Files Coverage Δ
models/migrations/migrations.go 1.32% <ø> (ø) ⬆️
modules/auth/org.go 0% <ø> (ø) ⬆️
models/migrations/v90.go 0% <0%> (ø)
routers/org/teams.go 0% <0%> (ø) ⬆️
models/org.go 69.54% <100%> (+0.42%) ⬆️
routers/api/v1/convert/convert.go 84.03% <100%> (+0.07%) ⬆️
routers/api/v1/org/team.go 25.93% <100%> (+0.34%) ⬆️
models/repo.go 49.23% <17.64%> (+0.63%) ⬆️
models/org_team.go 50.88% <35.71%> (-1.17%) ⬇️
models/repo_watch.go 65.11% <0%> (-3.49%) ⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0820e0...8f297ca. Read the comment docs.

@ngourdon
Copy link
Contributor Author

Merge to master.
The PR go-gitea/go-sdk#166 is no longer required thanks to #6905
Build is passing

@techknowlogick techknowlogick modified the milestones: 1.9.0, 1.10.0 Jun 25, 2019
@lunny
Copy link
Member

lunny commented Jul 8, 2019

Please resolve the conflicts

@ngourdon
Copy link
Contributor Author

@lunny conflicts resolved

// addAllRepositories adds all repositories to the team.
// If the team already has some repositories they will be left unchanged.
func (t *Team) addAllRepositories(e Engine) error {
err := e.Iterate(&Repository{OwnerID: t.OrgID}, func(i int, bean interface{}) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just change includesAllRepository to true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All repositories need to be added to the team because access rights management won't work without them.
It is the same behavior as for owners teams.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. And I think we should use a better method to batch insert a slice of team_repo.

Copy link
Contributor Author

@ngourdon ngourdon Jul 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure we can use a better method to batch insert a slice of team_repo but the method addRepository do a lot more than that.
imho I don't think the batch insert will be worth it compared to the whole processing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assume there are 1000 repositories on this organization. And the code here is not work with sqlite database.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still return 500 on my local test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't reproduce the issue. I must do it wrong.
I build with TAGS="bindata sqlite sqlite_unlock_notify" make clean generate build
I tested on Linux and Windows.
Do the UT pass on your local machine?
Can you please give me the steps which lead to the error 500?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I compiled locally. I'm on macOS. Just grant team to all organization repositories.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UT must fail, don't they?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problematic in pgSQL as well (500). After some debugging I found hasTeamRepo get this error: lib/pq#635

@davidsvantesson
Copy link
Contributor

I would like to have a warning when pressing 'Update Setting' if includes_all_repositories was changed from false to true. Something like "This will add access for the team access to all 1118 repositories in the organization" Ok/Cancel.
If you have really many repositories and someone do this change by mistake, it will take time to restore.

@lafriks
Copy link
Member

lafriks commented Sep 30, 2019

Please resolve conflicts

@lunny lunny modified the milestones: 1.10.0, 1.11.0 Oct 4, 2019
@davidsvantesson
Copy link
Contributor

@ngourdon doesn't seem to be active right now. Can I take over this (create a new PR)?

@lunny
Copy link
Member

lunny commented Nov 6, 2019

replaced by #8688

@lunny lunny closed this Nov 6, 2019
@lunny lunny removed this from the 1.11.0 milestone Nov 6, 2019
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
@delvh delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

I want a team in organization can has all repository,not have to add it one by one
8 participants