Implement API endpoint to link a package to a repo #23851
Conversation
silverwind
added
type/feature
GiteaBot
added
lgtm/need 2
pull-request-size
bot
added
the
size/L
|
@sclu1034 Cloud you update your branch and resolve merge conflicts? |
To satisfy the API's restrictions on write access when linking packages, the test needs to be run with a different user.
Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
sclu1034
force-pushed
the
feat/link-package-api
branch
from
8b3be5e
to
6e2661b
Compare
|
Rebased on current |
services/packages/packages.go
Outdated
| if !canWrite { | ||
| perms, err := access_model.GetUserRepoPermission(ctx, repo, doer) | ||
| if err != nil { | ||
| return fmt.Errorf("Error getting repository permissions for %d on %d: %w", doer.ID, repo.ID, err) | ||
| } | ||
|
|
||
| canWrite = perms.CanWrite(unit.TypePackages) | ||
| } | ||
|
|
||
| if !canWrite { | ||
| return fmt.Errorf("No permissions to link this package and repository") | ||
| } |
There was a problem hiding this comment.
| if !canWrite { | |
| perms, err := access_model.GetUserRepoPermission(ctx, repo, doer) | |
| if err != nil { | |
| return fmt.Errorf("Error getting repository permissions for %d on %d: %w", doer.ID, repo.ID, err) | |
| } | |
| canWrite = perms.CanWrite(unit.TypePackages) | |
| } | |
| if !canWrite { | |
| return fmt.Errorf("No permissions to link this package and repository") | |
| } | |
| perms, err := access_model.GetUserRepoPermission(ctx, repo, doer) | |
| if err != nil { | |
| return fmt.Errorf("Error getting repository permissions for %d on %d: %w", doer.ID, repo.ID, err) | |
| } | |
| if !perms.CanWrite(unit.TypePackages) { | |
| return fmt.Errorf("No permissions to link this package and repository") | |
| } |
It is also possible that packages are disabled for this repo, so I think we should check for all users, not just if user isn't the owner.
|
Goooooooooooooooooooooooooooooooooooooooooood !!! |
Co-authored-by: Denys Konovalov <kontakt@denyskon.de>
|
|
||
| func LinkPackageToRepository(ctx context.Context, doer *user_model.User, p *packages_model.Package, repoID int64) error { | ||
| if repoID != 0 { | ||
| repo, err := repo_model.GetRepositoryByID(ctx, repoID) |
There was a problem hiding this comment.
This MUST check if the repo belongs to the package owner. And there should be a test for linking a repo of another user.
There was a problem hiding this comment.
To avoid further back and forth on permissions, and me doing best-effort guesses, can you give me a detailed run down on who should be allowed to do what?
routers/api/v1/packages/package.go
Outdated
| // - name: repo | ||
| // in: query | ||
| // description: ID of the repository to link | ||
| // type: integer | ||
| // required: true |
There was a problem hiding this comment.
Would be nice if you can't just pass the repository id (which you don't have usually) but the repo name.
Could have two parameters repo_id=... and repo_name=.... The endpoint should choose the matching repo then. Could prefer the id over the name.
There was a problem hiding this comment.
Not particularly proud of how my code parsing those two parameters turned out, but I didn't find any useful utilities.
Any ideas to improve 221dcdc?
GiteaBot
added
lgtm/blocked
Co-authored-by: KN4CK3R <admin@oldschoolhack.me>
|
@KN4CK3R can you please have a look at the author's questions? |
|
|
||
| var repoID int64 | ||
|
|
||
| formRepoID := ctx.FormString("repo_id") |
There was a problem hiding this comment.
Do we really want to expose the repository's id? I don't think we should do that. Using OwnerName and RepoName should be better.
There was a problem hiding this comment.
I'm not sure what you mean by "exposing" in this context. The existing GET /repos/{owner}/{repo} endpoint makes the id value available in its response, so that information is already "exposed" to every consumer of the public API.
What's the harm in giving the user a choice which identifier they can run this new endpoint against?
There was a problem hiding this comment.
@sclu1034 I think last comments refer to the fact that most, if not all the API, refer to orgs and repos using their name, and not their id. At least I already played with the API and I don't recall I ever sent requests using the repo ids. It just feels more natural to use the repo name instead.
Moreover, the web frontend only allow linking to a repository of the same owner. If that is by design, you would have to drop the owner part in the repo name.
EDIT: from @KN4CK3R's previous comment, it is indeed by design.
Just my last two cents on the endpoint: there could only be zero or one link to a repository, so I'd rather use PUT and DELETE.
There was a problem hiding this comment.
Moreover, the web frontend only allow linking to a repository of the same owner. If that is by design, you would have to drop the owner part in the repo name.
I think this is by design. It's weird to link a package to another owner's repository.
There was a problem hiding this comment.
@lunny well I'm still wondering. I develop some Unity (game engine) packages. Distribution relies on NPM protocol, although packages are obviously not meant for Nodejs (it's C#). And I also develop some Nodejs packages. But Gitea only provides a single NPM registry per owner. So it seems to me the only way to cleanly separate both usages is to make a dedicated "owner", so to have two registries. But this is only for distribution. Development still occurs in the same org.
Closes #21062.