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

Changes on 404, robots, and sitemap #6798

Merged
merged 5 commits into from Mar 25, 2020
Merged

Changes on 404, robots, and sitemap #6798

merged 5 commits into from Mar 25, 2020

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Mar 19, 2020

  • Drop check for project privacy level, follow the privacy level from the version instead.
  • Only serve robots.txt if the version is public
  • Only serve the sitemap if there is at least one public version
  • Serve the custom 404 page from a version if the current user has permissions over that version to allow leaking anything.

These changes aren't ported to the serve app, only on proxito. There is a PR on .com adapting the code to this new changes.

Ref #6194

@@ -323,7 +322,7 @@ def get(self, request, project):
include_file=False,
version_type=self.version_type,
)
path = os.path.join(storage_path, 'robots.txt')
path = f'{storage_path}/robots.txt'
Copy link
Member Author

@stsewd stsewd Mar 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't use path.join for storage paths, there are more in this file

@stsewd stsewd requested a review from Mar 19, 2020
@humitos
Copy link
Member

@humitos humitos commented Mar 20, 2020

These changes aren't ported to the serve app, only on proxito. There is a PR on .com adapting the code to this new changes.

Should we wait until we are fully on El Proxito to merge this? (we are really close to that)

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 23, 2020

Should we wait until we are fully on El Proxito to merge this? (we are really close to that)

Don't think is necessary, I mean, there isn't a big difference from what we have and these changes

Copy link
Member

@ericholscher ericholscher left a comment

This looks like a good change 👍

@@ -266,14 +264,17 @@ def get(self, request, proxito_path, template_name='404.html'):
if default_version_slug != version_slug:
versions.append(default_version_slug)
for version_slug_404 in versions:
if not self.allowed_user(request, final_project, version_slug_404):
Copy link
Member

@ericholscher ericholscher Mar 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. This is going to add even more queries to our slow 404's. I'm a bit worried about that. I guess it won't cause a query on the .org, which is where i'm most worried about it, so it should be fine.

Copy link
Member Author

@stsewd stsewd Mar 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, on .org that function just returns True, it doesn't query the db.

@stsewd stsewd merged commit 0c0e75e into master Mar 25, 2020
2 checks passed
@stsewd stsewd deleted the 404-robots-sitemap branch Mar 25, 2020
@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 25, 2020

This PR is needed in .com to math this changes readthedocs/readthedocs-corporate#875

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.

None yet

3 participants