search

squidfunk / mkdocs-material

Documentation that simply works
https://squidfunk.github.io/mkdocs-material/
MIT License
20.91k stars 3.54k forks source link

Improve Insiders workflow #4104

Closed rfay closed 2 years ago

rfay commented 2 years ago

Contribution guidelines

I've found a bug and checked that ...

Description

Although we love the new features in mkdocs-material-insiders, and are perfectly happy to subscribe, there are massive problems with the approach used for sharing the code.

  1. Using mkdocs-material-insiders requires a token with repo scope, which means a hugely powerful token, powerful enough to write every repo the user has access to, including private repos. It (and I) can even write new branches into https://github.com/squidfunk/mkdocs-material-insiders, which seems totally inappropriate. Bottom line: A powerful token like this, if compromised, is a disaster for everything the user owning it has ever touched.
  2. Using mkdocs-material-insiders (with required auth) means that the open-source workflow is completely broken. With my project ddev for example, where forked PRs are the norm (and the way I work as well), it means that forked PR docs can't be properly be built, because of the requirement for authenticated access to mkdocs-material-insiders. Of course on GitHub actions (for good reason) there's no way for a fork to use a secret like the all-powerful token needed here.

Sadly, I'm going to have to back out the insiders work (https://github.com/drud/ddev/issues/3967). We'll continue the sponsorship and appreciate the work, but...

Please find a better way to work with this in an open-source environment. Thanks!

Expected behaviour

Actual behaviour

Described above

Steps to reproduce

Above

Package versions

described above

Configuration

described above

System information

described above

squidfunk commented 2 years ago

Thanks for reporting. Harsh words. In general, I'm happy to improve the workflow wherever possible, but it's not trivial to share a code base and not share it at the same time. If you have concrete ideas, I'm very willing to discuss them.

rfay commented 2 years ago

It is hard stuff, but at least figure out how to do it with a read-token instead of full repo, thanks!

squidfunk commented 2 years ago

Using mkdocs-material-insiders requires a token with repo scope, which means a hugely powerful token, powerful enough to write every repo the user has access to, including private repos. It (and I) can even write new branches into https://github.com/squidfunk/mkdocs-material-insiders, which seems totally inappropriate. Bottom line: A powerful token like this, if compromised, is a disaster for everything the user owning it has ever touched.

Okay, so first of all, note that what we describe in our documentation is one way of doing things, i.e., a recommendation which has some upsides and downsides. The biggest upside is that it is easy, and as long as you don't share your personal access token (PAT), you're safe. You could (and should) create a separate access token for this, so you can easily revoke it when there's a breach. The downside is that you'll grant write access to all repositories, because GitHub's access control doesn't allow for more granular settings. The repo scope is the smallest possible scope to grant access to the code:

Bildschirmfoto 2022-07-10 um 09 38 26

We always include the source as part of the repository, so you can pip install Material for MkDocs (and Insiders) directly from git. Although this increases the size of the repository, it has been shown to be very practical in terms of quickly testing bugfixes being able to install the latest master. For this reason, we're recommending the same logic for installing Insiders, as the built artifacts are checked into the repository. This, however, needs the repo scoped PAT.

In the past, we also offered a Docker image which we hosted as part of our private registry on GitHub, allowing users to install the Docker-hosted build of Insiders. This would only need the read:packages scope, but was discontinued for several reasons explained in https://github.com/squidfunk/mkdocs-material/issues/2442#issue-831208170:

  1. As you can see from the download numbers, most users use pip to install Material for MkDocs and Insiders, mainly because the Docker image would need to be extended in order to install additional third-party plugins or extensions.

  2. I have no cost control over the private registry, since there's no way to attribute downloads to users or even limiting the number of downloads they're allowed to do for specific tiers. It's not possible to offer this at a flat fee of $10 a month.

So, what other options would we have? In theory, we could spin up a private pip registry, because GitHub doesn't allow for hosting Python packages. There's an open issue tracking progress, but no ETA whatsoever. This would at least solve problem 1.), since we're in Python land. I have only shallowly investigated this since it wasn't necessary until now, but it might also solve problem 2.), if cost and downloads could be attributed to specific users. We would then limit downloads per month per tier to keep cost in control. I'm not sure whether we can use GitHub OAuth scopes for authenticating, so a separate registration to use our self-hosted Python registry might be necessary. I may investigate this in the future, given that the number of sponsors is increasing and I want to offer the most comfortable way of installing Insiders, but this is definitely a long shot given the additional setup and maintenance effort.

Until then, the recommendation is to create a bot account you solely use for sponsoring and access control. If the PAT of the bot is compromised, none of your other repositories is exposed when the token is compromised.

Using mkdocs-material-insiders (with required auth) means that the open-source workflow is completely broken. With my project ddev for example, where forked PRs are the norm (and the way I work as well), it means that forked PR docs can't be properly be built, because of the requirement for authenticated access to mkdocs-material-insiders. Of course on GitHub actions (for good reason) there's no way for a fork to use a secret like the all-powerful token needed here.

This is where things start to get interesting. Obviously, you shouldn't share your code with outside collaborators, because that would counteract the whole sponsorware model. I could just fork your repository, then craft a specific action to extract the Insiders source code. Easy. The fact that Insiders access is tied to a PAT which has write access is actually a strong incentive for you not to share your access. I would go as far as to say that Insiders might have never been successful if people wouldn't be incentivized to keep the token secret at all cost, but that is speculation. For this reason, it is only logical that forking the repository will remove Insiders access. It's just how things have to be for this model to work. However, Insiders is in large parts a drop-in replacement for the community edition. Yes, there are some parts that may not render as beautifully (like for example tooltips and card grids), but most of the other improvements are either new plugins that progressively enhance the community edition, or can be switched off with a separate plugin configuration. If you feel that some things do not work without Insiders at all, please elaborate, and I'll investigate whether we can improve interop.

All this allows outside collaborators to fork the repository, make changes and preview them with the community edition, then PR those changes and inside the PR (which you manually approve to run) see the changes as part of a deployment preview built with Insiders.

Alternatively, what you could do is create an organization, add all trusted outside collaborators for your project and then fork the Insiders repository into your organization. This is exactly what we recommend organizations doing, because then access can be shared among all members.

I hope I could shed some light on some of the decisions that lead to the current technical limitations, inducing the potential threats and problems you mentioned in your post. If you have concrete ideas how we can improve this workflow, I'm happy to discuss as I already noted in my previous comment, but from my investigations, there are no low-hanging fruits.

If you feel otherwise, please reopen.

squidfunk commented 2 years ago

Update: one improvement on the current process that I'm going to ship in the coming weeks is moving the Insiders repository into a dedicated organization, so we can grant read-only access to collaborators and have finer control in general. I'm still checking how this would impact current collaborators, but it looks that the process should be quite seemless.

rfay commented 2 years ago

Using an org will be a major step forward for the mkdocs-material-insiders repo, because at least you can just give collaborators read privileges and you won't be giving them write privileges. GitHub does a great job of forwarding when you move repos, even when you do a 2-step move, which I recently did. Of course you'll have to set the pip repo to point to the new one because I doubt it's as good at that.

Thanks for thinking about this carefully, and taking it seriously. I'm sorry to be so harsh, but for somebody who lives and dies as an open-source maintainer, it's a pretty difficult situation right now.

Note that a bot token is a good idea for preventing catastrophe, but I don't think you currently have easy support for it right? One would have to contact you personally and say "please add the bot in addition to me". I already signed up with my personal instead of org account because the org required workarounds. And of course, then people would have to use the bot token everywhere, and they'd also be more likely to share it to solve collaborator problems.

rfay commented 2 years ago

And of course, thank you for this great project, and what a help it is to all of us. I'm glad to see you've been successful with the sponsorship approach, wow! And I hope you continue to be. I think it's probable with that level of success that you can fiddle with the model a little as well.

squidfunk commented 2 years ago

Note that a bot token is a good idea for preventing catastrophe, but I don't think you currently have easy support for it right? One would have to contact you personally and say "please add the bot in addition to me". I already signed up with my personal instead of org account because the org required workarounds. And of course, then people would have to use the bot token everywhere, and they'd also be more likely to share it to solve collaborator problems.

That is correct. I'm working on a self-service solution, and I have some ideas, but I still need to work out the details to provide the best possible workflow. I'm on it.

And of course, thank you for this great project, and what a help it is to all of us. I'm glad to see you've been successful with the sponsorship approach, wow! And I hope you continue to be. I think it's probable with that level of success that you can fiddle with the model a little as well.

Thanks! Yes, I'm experimenting a lot, trying to learn. It's still very new terrain and I hope to share my learnings soon in a conference talk I'm currently preparing.

dmalan commented 1 year ago

Hi @squidfunk, just to chime in belatedly on this thread, GitHub recently announced "fine-grained personal access tokens," which might help as well?

https://github.blog/2022-10-18-introducing-fine-grained-personal-access-tokens-for-github

If you indeed end up moving Insiders into an organization, I think you could then enable fine-grained PATs for it at https://github.com/organizations/{ORG}/settings/personal-access-tokens, per the below:

Screen Shot 2023-01-30 at 11 29 39 PM

And I think sponsors could then create fine-grained PATs with repo scope just for the Insiders repo at https://github.com/settings/personal-access-tokens/new (after selecting your org as the Resource owner), per:

Screen Shot 2023-01-30 at 11 55 10 PM

In case helpful!

squidfunk commented 1 year ago

@dmalan sounds perfect! We'll move to an org later this year, and we'll definitely be opting for the most secure approach possible. Thanks for your research on this topic!

Edit: another small note why this hasn't happened yet: it's complicated. Keeping everything working while just moving the repository turned out to be not possible, at last from my testing. We'll cook up a migration path that is as easy as possible and definitely have a transition period of a few months.

gitressa commented 1 year ago

Thanks @squidfunk, this would be a nice improvement, since it's a blocker for DDEV to phase out third-party fonts, and respect user privacy.

squidfunk commented 1 year ago

Not sure what you're referring to @gitressa. The issue mainly talks about the Insiders workflow, but privacy via self hosting can be achieved with the privacy plugin, which wasn't discussed here. We're still working on a better Insiders workflow, though, albeit progress is currently rather slow.

rfay commented 1 year ago

I guess our belief was that the privacy plugin is only available with insiders... is that belief wrong?

rfay commented 1 year ago

BTW, DDEV has subscribed via GitHub sponsors in order to support this great project, but we aren't willing to use the insiders workflow because of the problems in this thread.

squidfunk commented 1 year ago

Yes, the privacy plugin is currently in Insiders, that's correct. I suspect that the next funding goal is to be hit any time, making it available for free. Thanks for supporting the project! We'll try to solve the problems as soon as possible.

gitressa commented 1 year ago

That sounds great, thanks @squidfunk! Sorry that I didn't provide enough background, thank you @rfay for providing that.

squidfunk commented 1 year ago

To revisit on the limitations mentioned in the OP:

  1. Using mkdocs-material-insiders requires a token with repo scope, which means a hugely powerful token, powerful enough to write every repo the user has access to, including private repos. It (and I) can even write new branches into https://github.com/squidfunk/mkdocs-material-insiders, which seems totally inappropriate. Bottom line: A powerful token like this, if compromised, is a disaster for everything the user owning it has ever touched.

As mentioned in https://github.com/squidfunk/mkdocs-material/issues/4104#issuecomment-1409757401 by @dmalan, you can now use a fine-grained access token. You only need to grant repo scope for the Insiders fork, and you're good. We're currently not advertising this on our docs, as we feel that the process is a little more involved, but it's perfectly possible.

  1. Using mkdocs-material-insiders (with required auth) means that the open-source workflow is completely broken. With my project ddev for example, where forked PRs are the norm (and the way I work as well), it means that forked PR docs can't be properly be built, because of the requirement for authenticated access to mkdocs-material-insiders. Of course on GitHub actions (for good reason) there's no way for a fork to use a secret like the all-powerful token needed here.

You can build your documentation with and without Insiders. We do the same in our repository. The public version builds perfectly fine without Insiders. You can only build with Insiders in CI by installing it in your CI workflow, and if your account has access to the repository, locally as well. There are some features that cannot be previews, but it's essentially limited to things like card grids, navigation subtitles, and related links on blogs. Most of the features in the pipeline are derived from the written content, e.g. document contributors. Additionally, you can opt-into using those features or not, if you fear that it hinders collaboration. They will become free features in the coming months and years anyway.

squidfunk commented 1 year ago

When you're using GitHub Actions, something like the following is sufficient to keep forks from requiring Insiders:

- name: Install Insiders build
  if: github.event.repository.fork == false
  env:
    GH_TOKEN: ${{ secrets.GH_TOKEN }}
  run: pip install git+https://${GH_TOKEN}@github.com/squidfunk/mkdocs-material-insiders.git
brabster commented 6 months ago

Hey, I switched over to fine-grained tokens yesterday and wrote a post on how to do it:

https://tempered.works/posts/2024/04/13/fine-grained-github-access-tokens-with-mkdocs-material-insiders/

@squidfunk would you have a problem linking it out from the docs? If you don't want the why-preamble, there's an anchor that's the start of the howto https://tempered.works/posts/2024/04/13/fine-grained-github-access-tokens-with-mkdocs-material-insiders/#fork-mkdocs-material-insiders

Happy to do you a quick PR if you want :+1: