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

Implemented route /v2/_catalog API functionality #19

Merged
merged 4 commits into from
Mar 4, 2024
Merged

Implemented route /v2/_catalog API functionality #19

merged 4 commits into from
Mar 4, 2024

Conversation

keyskull
Copy link
Contributor

@keyskull keyskull commented Mar 1, 2024

No description provided.

@gabivlj
Copy link
Collaborator

gabivlj commented Mar 1, 2024

Thank you for your contribution!

@keyskull
Copy link
Contributor Author

keyskull commented Mar 3, 2024

Anything else that I need to cover to merge this PR to the branch?

@skepticfx skepticfx self-requested a review March 3, 2024 08:42
@skepticfx
Copy link
Member

It looks there is no more user namespace or special authentication involved here. Shouldn't catalog be restricted to a privileged user?

@skepticfx skepticfx requested a review from gabivlj March 3, 2024 08:43
src/registry/r2.ts Show resolved Hide resolved
src/router.ts Show resolved Hide resolved
@gabivlj
Copy link
Collaborator

gabivlj commented Mar 3, 2024

Anything else that I need to cover to merge this PR to the branch?

Sorry forgot to submit my review :-). There are some comments I wanted to be addressed

@keyskull
Copy link
Contributor Author

keyskull commented Mar 3, 2024

About @skepticfx mentioned authentication feature, @gabivlj do you know we should have restrictions for each user or just general authentication?

src/registry/r2.ts Outdated Show resolved Hide resolved
@gabivlj
Copy link
Collaborator

gabivlj commented Mar 3, 2024

Thank you @keyskull, I had another nit but once that is addressed I think all is ok to merge from my side.

@gabivlj
Copy link
Collaborator

gabivlj commented Mar 3, 2024

About @skepticfx mentioned authentication feature, @gabivlj do you know we should have restrictions for each user or just general authentication?

We have some kind of read/write permissions. IMO this is already handled on the middleware side. What do you think @skepticfx?

@skepticfx
Copy link
Member

@gabivlj registries don't allow access to this by default AFAIK. But I believe, this specific authorization check is not really relevant to this PR. Believe a middleware which does the right RBAC should handle this correctly. None of our middleware examples allow this today, but something to be cautious about.

@gabivlj gabivlj merged commit 7e58b84 into cloudflare:main Mar 4, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants