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

Restricted users #6274

Merged
merged 23 commits into from
Jan 13, 2020
Merged

Restricted users #6274

merged 23 commits into from
Jan 13, 2020

Conversation

mnsh
Copy link
Contributor

@mnsh mnsh commented Mar 8, 2019

First part of an implementation of restricted users (#4334), as described in this comment.

Implemented so far:

  • Access checking when actaully accessing an org, repo or repo sub-unit
  • Filtering of results when browsing an org
  • Filtering of results when exploring repos and orgs

Remaining bits:

  • Filter results when exploring users
  • Filter /org/orgname/dashboard content
  • Ability for external users to create organizations and repositories (separate option for count limit)

@lunny lunny added type/feature Completely new functionality. Can only be merged if feature freeze is not active. pr/wip This PR is not ready for review labels Mar 8, 2019
@lafriks
Copy link
Member

lafriks commented Mar 8, 2019

Would be nice to add this option also to auth source so that all users from this auth source would get restricted option set automatically

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 8, 2019
@codecov-io
Copy link

codecov-io commented Mar 10, 2019

Codecov Report

Merging #6274 into master will increase coverage by 0.01%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6274      +/-   ##
==========================================
+ Coverage   42.29%   42.31%   +0.01%     
==========================================
  Files         598      598              
  Lines       78299    78318      +19     
==========================================
+ Hits        33118    33138      +20     
- Misses      41125    41126       +1     
+ Partials     4056     4054       -2
Impacted Files Coverage Δ
models/issue_comment.go 46.4% <100%> (+0.79%) ⬆️
routers/api/v1/repo/issue_comment.go 56.33% <75%> (+1.17%) ⬆️
services/pull/check.go 53.37% <0%> (-2.03%) ⬇️
models/gpg_key.go 55.59% <0%> (+0.55%) ⬆️

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 eaed6b3...a54d9d6. Read the comment docs.

@mnsh mnsh force-pushed the feature/restricted-users branch 2 times, most recently from 8e7f398 to 67fa39f Compare April 7, 2019 22:47
@mnsh
Copy link
Contributor Author

mnsh commented Apr 7, 2019

All parts should now behave as described, with one exception: Explore / Users lists all registered users. Since user-owned public repos are properly "handled" (access & visibility), that doesn't feel like a problem.

There are several places where the code picks between using models.GetSomethings(lots of args) and SearchSomethings(opts) based on whether a search string is provided (eg. if len(keyword) == 0). Instead of adding yet another argument to a like 7 argument func, I chose to always use the SearchSomething(opts) method (where the opts struct now contains Actor). Does this make sense?

Any feedback is appreciated.

routers/user/profile.go Outdated Show resolved Hide resolved
routers/user/profile.go Outdated Show resolved Hide resolved
@mnsh
Copy link
Contributor Author

mnsh commented Apr 15, 2019

@lafriks allowing auth sources to set or force an is_restricted value is indeed a good idea deserving it's own PR; this one feels big enough as it is.

Is this starting to look somewhat mergeable? Any feedback is appreciated.

@stale
Copy link

stale bot commented Jun 14, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Jun 14, 2019
@mnsh mnsh force-pushed the feature/restricted-users branch from 5185edd to f69772c Compare June 14, 2019 20:43
@stale stale bot removed the issue/stale label Jun 14, 2019
@kokarn
Copy link

kokarn commented Jun 15, 2019

Any progress on this? @lafriks ?

@lunny lunny added this to the 1.10.0 milestone Jun 16, 2019
@mnsh mnsh force-pushed the feature/restricted-users branch 4 times, most recently from 22ae258 to 6d15aa1 Compare June 23, 2019 12:53
@mnsh mnsh force-pushed the feature/restricted-users branch from 6d15aa1 to 5c78817 Compare July 10, 2019 22:02
@mnsh mnsh force-pushed the feature/restricted-users branch 2 times, most recently from e6419c2 to 8bb15b2 Compare September 2, 2019 19:15
@mnsh
Copy link
Contributor Author

mnsh commented Sep 2, 2019

Any chance of merging this soon @lafriks @lunny ?

@techknowlogick techknowlogick modified the milestones: 1.10.0, 1.11.0 Sep 3, 2019
Copy link
Member

@lafriks lafriks left a comment

Choose a reason for hiding this comment

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

Did not still want to approve :)

@lafriks lafriks added the type/changelog Adds the changelog for a new Gitea version label Jan 11, 2020
@lafriks
Copy link
Member

lafriks commented Jan 11, 2020

Sorry for so many iterations in reviews, good work ;)

@lunny
Copy link
Member

lunny commented Jan 12, 2020

For a restricted user, all repositories are private repositories. So we could only change function getUserRepoPermission on models/repo_permission.go and also SearchRepository to make repo as private if user is a restricted one.

models/org.go Show resolved Hide resolved
models/org.go Show resolved Hide resolved
@mnsh
Copy link
Contributor Author

mnsh commented Jan 12, 2020

So, what now? =)

@kokarn
Copy link

kokarn commented Jan 13, 2020

So if this is approved by 2 people, when can it be merged? Seems like there will be a lot of rebasing otherwise

@6543
Copy link
Member

6543 commented Jan 13, 2020

@mnsh can you "Update Branch" - CI should work now

@sapk
Copy link
Member

sapk commented Jan 13, 2020

@mnsh @6543 I have done it for you.

@mnsh
Copy link
Contributor Author

mnsh commented Jan 13, 2020

thankst @sapk

@sapk sapk merged commit 1751d5f into go-gitea:master Jan 13, 2020
@sapk
Copy link
Member

sapk commented Jan 13, 2020

Thanks @mnsh for the works.

@mnsh
Copy link
Contributor Author

mnsh commented Jan 13, 2020

Yess!

Thanks to everyone involved.

@mnsh mnsh mentioned this pull request Mar 13, 2020
4 tasks
@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
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.