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

Check user instead of organization when creating a repo from a template via API #16346

Merged
merged 4 commits into from
Jul 15, 2021
Merged

Check user instead of organization when creating a repo from a template via API #16346

merged 4 commits into from
Jul 15, 2021

Conversation

ijaureguialzo
Copy link
Contributor

API documentation for repos/generate action implemented on #15958 says for the owner parameter: The organization or person who will own the new repository.

Current code only checks if destination owner is an organization and error is given if trying to copy a template to another user.

Checking for user instead of organization allows for both targets.

@codecov-commenter
Copy link

codecov-commenter commented Jul 5, 2021

Codecov Report

Merging #16346 (6b507cb) into main (ff69dff) will decrease coverage by 0.01%.
The diff coverage is 14.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16346      +/-   ##
==========================================
- Coverage   45.43%   45.42%   -0.02%     
==========================================
  Files         717      717              
  Lines       84177    84180       +3     
==========================================
- Hits        38250    38239      -11     
- Misses      39780    39793      +13     
- Partials     6147     6148       +1     
Impacted Files Coverage Δ
routers/api/v1/repo/repo.go 54.54% <14.28%> (-0.35%) ⬇️
services/pull/temp_repo.go 25.77% <0.00%> (-4.13%) ⬇️
modules/charset/charset.go 71.71% <0.00%> (-4.05%) ⬇️
services/pull/check.go 25.51% <0.00%> (-2.76%) ⬇️
services/pull/patch.go 54.23% <0.00%> (-1.70%) ⬇️
models/repo_list.go 77.04% <0.00%> (-0.78%) ⬇️
modules/queue/workerpool.go 54.96% <0.00%> (+1.14%) ⬆️
modules/util/timer.go 85.71% <0.00%> (+42.85%) ⬆️

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 ff69dff...6b507cb. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 5, 2021
@6543 6543 added this to the 1.15.0 milestone Jul 6, 2021
@6543 6543 added type/enhancement An improvement of existing functionality skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Jul 6, 2021
@6543 6543 requested a review from a1012112796 July 6, 2021 01:12
@a1012112796
Copy link
Member

Not sure in curent design, whether user can create new repo for another user, and if yes, which permission should be request ...

/cc @lunny

@ijaureguialzo
Copy link
Contributor Author

At least, admins should be able to do the copy to another user's account.

I'm using this feature in a Learning Management System and before this feature was available, I had to use repos/migrate to create copies of the repos for the students from base repos owned by root user.

This is much more convenient and efficient.

@6543
Copy link
Member

6543 commented Jul 7, 2021

Well for admins we have the sudo header

@zeripath
Copy link
Contributor

At present just use Sudo.

For other non-admin users we would need to think a bit more about authorization.

@zeripath zeripath removed this from the 1.15.0 milestone Jul 13, 2021
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

Use SUDO instead.

@6543 6543 closed this Jul 13, 2021
@ijaureguialzo
Copy link
Contributor Author

At present just use Sudo.

For other non-admin users we would need to think a bit more about authorization.

It's not a problem of permissions, it's that this endpoint, using an API call token from a root account, doesn't work.

Documentation says that the repo can be copied to an org or user, and it doesn't, it only copies if destination is an organization, that's what I was trying to solve.

@6543
Copy link
Member

6543 commented Jul 13, 2021

@zeripath should we realy enforce sudo header?
-> from permissions it wont make a difference

@ijaureguialzo you can solve your problem with a sudo header

@ijaureguialzo
Copy link
Contributor Author

Sorry, @6543, could you explain me how? I don't understand how sudo can solve my problem.

My goal is to be able to copy an admin-owned private repo to another random user on the system (I'm creating tasks for my students).

I'm using this PHP code to query the Gitea API:

$request = self::$client->post("repos/$repositorio/generate", [
    'headers' => [
        'Authorization' => 'token ' . config('gitea.token'),
        'Accept' => 'application/json',
    ],
    'json' => [
        "owner" => $username,
        "name" => $destination,
        "private" => true,
        "git_content" => true,
        "description" => $description,
    ]
]);

The gitea.token is a token created from an admin account (root) and I can make other operations (repository deletions for example).

If I try to run that code and copy a repo named task-repo owned by root to student, a normal user on the system, I get this log message: {"error":"request owner 'task-repo' is not exist"}

The log message itself is wrong, but looking at the source I realized that ever if I have full permissions, if I'm not the owner of the repository and the destination is not and org, the copy is not allowed; and documentation says it should be.

With the patch I submitted, the copy is made correctly and fails if the destination user or organization doesn't exist.

Thanks in advance for your time.

@6543 6543 reopened this Jul 13, 2021
@6543
Copy link
Member

6543 commented Jul 13, 2021

I have to agree if the repo is private sudo wont work

@zeripath
Copy link
Contributor

You're creating repos for other users. We don't have that functionality yet and haven't properly considered the consequences of doing so.

We'd really need to completely double check that this actually does the permissions checks properly and really does handle all of the counters properly.

@6543 6543 removed the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Jul 14, 2021
@6543 6543 added this to the 1.16.0 milestone Jul 14, 2021
@6543 6543 self-assigned this Jul 14, 2021
@a1012112796
Copy link
Member

Hmm, In my view. I'd like add a new api for admin to generate repo for other user. for example:
/admin/users/{username}/repos/generate

like:
https://try.gitea.io/api/swagger#/admin/adminCreateRepo

@ijaureguialzo
Copy link
Contributor Author

Hmm, In my view. I'd like add a new api for admin to generate repo for other user. for example:
/admin/users/{username}/repos/generate

like:
https://try.gitea.io/api/swagger#/admin/adminCreateRepo

That would be great but I don't have enough profiency with Gitea's code, so, can't help much with that.

@zeripath
Copy link
Contributor

OK on deeper looking I think this would work

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 14, 2021
Copy link
Member

@a1012112796 a1012112796 left a comment

Choose a reason for hiding this comment

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

index 9c534a194..5e0228fdb 100644
--- a/routers/api/v1/repo/repo.go
+++ b/routers/api/v1/repo/repo.go
@@ -387,6 +387,11 @@ func Generate(ctx *context.APIContext) {
                        return
                }
 
+               if !ctx.User.IsAdmin && !ctxUser.IsOrganization() {
+                       ctx.Error(http.StatusForbidden, "", "Only admin can generate repository for other user.")
+                       return
+               }
+
                if !ctx.User.IsAdmin {
                        canCreate, err := ctxUser.CanCreateOrgRepo(ctx.User.ID)
                        if err != nil {

My suggestion about permission check.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jul 15, 2021
@ijaureguialzo
Copy link
Contributor Author

Thank you for the approval @a1012112796. Will this changes make it to 1.15? I see it's marked for 1.16 milestone.

@zeripath
Copy link
Contributor

I'm afraid it won't make 1.15.

@6543 6543 merged commit 251d7f5 into go-gitea:main Jul 15, 2021
@ijaureguialzo
Copy link
Contributor Author

Great, thank you all for your help.

AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 10, 2021
…te via API (go-gitea#16346)

* Check user instead of organization

* Enforce that only admins can copy a repo to another user
6543 pushed a commit to 6543-forks/gitea that referenced this pull request Sep 30, 2021
…te via API (go-gitea#16346)

* Check user instead of organization

* Enforce that only admins can copy a repo to another user
@6543 6543 added backport/v1.15 backport/done All backports for this PR have been created labels Sep 30, 2021
6543 added a commit that referenced this pull request Oct 1, 2021
…te via API (#16346) (#17195)

* Check user instead of organization

* Enforce that only admins can copy a repo to another user

Co-authored-by: Ion Jaureguialzo Sarasola <ion@jaureguialzo.com>
@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants