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 private API endpoints for admin tasks #3119

Open
9 tasks
Turbo87 opened this issue Dec 25, 2020 · 9 comments
Open
9 tasks

Add private API endpoints for admin tasks #3119

Turbo87 opened this issue Dec 25, 2020 · 9 comments
Labels
A-backend ⚙️ C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear

Comments

@Turbo87
Copy link
Member

Turbo87 commented Dec 25, 2020

Heroku allows us to perform one-off tasks directly on the server, and we have the crates-admin binary in this project which allows this. This tool locks us into platforms that support this kind of direct server access though, which would make it harder for us to potentially switch hosting providers in the future.

The way to resolve this problem is to convert all of the existing tools in crates-admin into private API endpoints. These endpoints should be protected by checking if the current user is part of a specific GitHub team.

These are the tools that will need to be converted:

render_readmes appears to be somewhat special here because it is a potentially long-running operation. Converting this might involve creating a new swirl background task that performs the actual operation.

@Turbo87 Turbo87 added the C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear label Dec 25, 2020
@Turbo87 Turbo87 linked a pull request Jan 7, 2021 that will close this issue
@hi-rustin hi-rustin removed their assignment Jul 27, 2021
@hi-rustin
Copy link
Member

@rustbot claim

I am working on this.

@hi-rustin hi-rustin removed their assignment Oct 26, 2021
@mario-areias
Copy link

Hi @Turbo87 I was thinking in working on this issue. Is this one still relevant?

I was going over the requirements, my understanding is that the relationship between users and teams are not mapped in the database. To do the authorisation for this ticket we would need a method similar to team_with_gh_id_contains_user. Is that a correct assumption?

One last question: Would you prefer have one PR for each endpoint or a single PR with all endpoints implemented?

Thanks

@Turbo87
Copy link
Member Author

Turbo87 commented Oct 23, 2022

Is this one still relevant?

yeah, I guess it is.

I was going over the requirements, my understanding is that the relationship between users and teams are not mapped in the database. To do the authorisation for this ticket we would need a method similar to team_with_gh_id_contains_user. Is that a correct assumption?

for a first implementation it would probably be sufficient to just hardcode the list of admin user accounts. we can still enhance it later if necessary :)

Would you prefer have one PR for each endpoint or a single PR with all endpoints implemented?

definitely one PR per endpoint. the smaller the PRs are the more likely it will be to get quick reviews 😉

in general I recommend have a look at and roughly following the suggestions in https://mtlynch.io/code-review-love/ :)

@carols10cents
Copy link
Member

I've actually gotten started on this but I'd love help; I'm really slow. Here's my branch if you want to work off of it: #5376

@mario-areias
Copy link

Oh I didn't know you were working on it @carols10cents, my apologies. Thanks for sending that PR it is good for me to learn from your approach in solving this problem 🙏

Appreciate you sharing your branch, but I am looking for a ticket that I can implement in Rust only (which I thought this ticket was). I am avoiding JavaScript for now because I want to avoid learning two code bases at the same time and want to focus on Rust first. So it may be better you finish this ticket since it has some JavaScript involved. I will try to find another ticket else I can focus on. Hope that's okay 😄

@carols10cents
Copy link
Member

Don't be sorry, there's no way you could have known :)

It's up to you, but like I said, I'm slow, so I'd love help even if it's just with the backend. In fact, I'd prefer if each one of these backend endpoints was its own PR!

@mdtro
Copy link
Contributor

mdtro commented Mar 13, 2023

Since this admin panel will provide sensitive functionality, what are your thoughts on using a different cookie to allow access to it? The current cargo_session cookie has a pretty lengthy lifespan.

If we used a separate cookie, for example cargo_admin, with a short lifespan of an hour or less this would help mitigate session hijacking threats.

@carols10cents
Copy link
Member

Sure, sounds great!

@Turbo87
Copy link
Member Author

Turbo87 commented Mar 13, 2023

If we used a separate cookie, for example cargo_admin, with a short lifespan of an hour or less this would help mitigate session hijacking threats.

that would introduce a third authentication option though, which I assume would make our auth code even more complicated. but I'm happy to be wrong about this if you can come up with a simple solution :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear
Projects
None yet
Development

No branches or pull requests

5 participants