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

Allow project badges for private version #6252

Merged
merged 1 commit into from Oct 9, 2019

Conversation

@davidfischer
Copy link
Contributor

commented Oct 7, 2019

  • Refactored the badge view into a class-based view
  • Private versions require a token. This PR also adds the token to the badge examples/directions in the dashboard for private versions. This PR doesn't do anything for protected versions. Should it?
  • Token is generated from the project_slug and the secret key which has a few implications:
    • A token can't be invalidated without changing the project slug or secret key. If somebody leaks their token, it's basically leaked. All it leaks is the status of the build, but it does leak information.
    • The token is not based on the version so leaking the token for a project would leak the existence of all versions for that project. However, this makes changing the badge to show a different version as easy as changing the ?version=latest parameter.
- Private versions require a token
- Token is based on the project_slug and the secret key
@davidfischer davidfischer requested a review from readthedocs/core Oct 7, 2019
@stsewd
stsewd approved these changes Oct 7, 2019
Copy link
Member

left a comment

This PR doesn't do anything for protected versions. Should it?

We are already deprecating those, so no.

The token is not based on the version so leaking the token for a project would leak the existence of all versions for that project. However, this makes changing the badge to show a different version as easy as changing the ?version=latest parameter.

I think that's fine

A token can't be invalidated without changing the project slug or secret key. If somebody leaks their token, it's basically leaked. All it leaks is the status of the build, but it does leak information.

Guess we could put that as a future improvement? Not really sure how important is that for users


def serve_badge(self, request, status):
style = self.get_style(request)
if status not in self.STATUSES:

This comment has been minimized.

Copy link
@stsewd

stsewd Oct 7, 2019

Member

We always pass a valid status here and looks like just a method used in this class.

This comment has been minimized.

Copy link
@davidfischer

davidfischer Oct 8, 2019

Author Contributor

It's possible this is overkill as it isn't used anywhere else. However, it is nice to double check.

@humitos
humitos approved these changes Oct 8, 2019
Copy link
Member

left a comment

LGTM!

Token is generated from the project_slug and the secret key which has a few implications

We could have a token per project and version maybe. Although, I think it may be over-killing --but looks like the code should not be more complicated since it's prepared for that. Maybe the UI is the problem, though.

@ericholscher

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

We could have a token per project and version maybe. Although, I think it may be over-killing --but looks like the code should not be more complicated since it's prepared for that. Maybe the UI is the problem, though.

I think the bigger issue is that users will simply copy the badge URL from their README and change the version slug, and it will break in a non-obvious way. I think per-project is fine for now. 👍

@ericholscher ericholscher merged commit a893ca3 into master Oct 9, 2019
3 checks passed
3 checks passed
continuous-documentation/read-the-docs Read the Docs build succeeded!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pyup.io/safety-ci No dependencies with known security vulnerabilities.
Details
@ericholscher ericholscher deleted the davidfischer/project-badges-private-versions branch Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.