-
Notifications
You must be signed in to change notification settings - Fork 3
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
API: using private API endpoints on public listing pages #154
Comments
Completely unauthenticated APIv3 access seems to be the problem here, the mixin I'm not exactly sure what the API needs to allow public access like this, or what exactly we want to change to allow this. I think allowing this in the API makes sense, as the dashboard shows the same data publicly, but this obviously complicates the authorization check. @humitos do you have opinions on what we do here with APIv3? Do we need to add a class to |
We discussed this more and it seems like it shouldn't be too hard to allow unauthed access to APIv3 or some subset of APIv3. The work looks to be primarily in tests. Our initial concerns were performance and tracking users, but performance at least is less of a concern now. We don't yet have an idea of whether we do this change globally or just start with a limited number of views, similar to the notifications authentication changes. |
Related issue/PR that performs a similar change on the notifications API endpoint to list notifications on public builds: readthedocs/readthedocs.org#11449 (comment) |
The places where the dashboard is using API interactions on public views:
I think that about covers things for now, but I might have more on this list. |
Also noting, the work where I was blocked on this is So I think I can continue there and I'll disregard the side effect of public views not working with the API calls for now. |
…ces (#11485) * API V3: change permissions to allow anonymous access to public resources Ref readthedocs/ext-theme#154 * Exclude external versions from API * Fix organizations queryset * Fix tests get_permissions overrides all declarations of permissions, even in actions
With readthedocs/readthedocs.org#11485 anonymous access will be working on .org, on .com, our |
Personally, I find public version/build listing views of projects to be an odd edge case to design around. I get that historically it's been the case on community, but I do feel we could at least do away with these views on commercial (and all of the complexity conditionally public views brings) and it wouldn't be noticed by users. I think the same is mostly true for community, but there might be some users used to using the dashboard logged out at this point. Instead of altering queries, and making this logic/query even more complex, I think we more easily could build a good failure mode into these listing UIs. We could then see if anyone complains about the links on commercial. |
I believe we fixed this on community with a hack and commercial might need some more thought on if we need to provide this feature. Make sure to summarize where we are and what we need in a discussion. |
The main problem here is that we are not allowing users to access public resources that are outside their organizations, this is using API V3, our dashboard does allow access to public resources. Even if a user is logged in, if it doesn't belong to the organization of a project that is public, it can't access to that project using the API. |
I understand there are two possible directions here:
Am I correct? If so, what are the pros/cons of each of them? |
Don't think this is an option, we do have open source projects using .com that want their contributors to have access to the build pages, and I remember other clients expecting that as well (if I remember correctly, that was the reason we added support for public builds if the project and version are public). The option here would be to just keep the API as is. But if our new dashboard uses API v3 in some views that are expected to be public, then we do need to change the API. |
The dashboard is indeed already using APIv3 on public listing views. The dashboard should be using APIv3 more frequently, but I was forced to use APIv2 in a number of cases where we don't have feature parity. |
I feel I'm back to the beginning now. Is there anything to discuss here then? |
Since we want to keep the behavior from the old dashboard, while keep using API v3 on the new dashboard, we are going to allow access to public resources on .com too. We may want to pay a little more attention to any related information from resources we may be leaking (https://github.com/readthedocs/readthedocs-corporate/issues/1845 for example), we are leaking that information to users under the same organization now, but if we made resources public, anyone will be able to access those. |
This is a link to see where we're using expandable fields in the frontend: https://github.com/search?q=repo%3Areadthedocs%2Fext-theme+expand+language%3AJavaScript&type=code&l=JavaScript |
This allows us to override just the v2 querysets on .com, since we want to share the same logic as .org for v3. ref readthedocs/ext-theme#154
The issue here is that for public listing pages, unauthed users are using the project detail v3 API and that requires authentication. Because of this, the list of downloads and the documentation URL buttons are not working correctly. This was discovered in #152
We had a conversation around this already, and I believe we decided to make some of our detail API responses public instead of authorized. We might have to consider how this affects readthedocs.com, but this seems like the easiest fix.
The other option is to hardcode the version URL and version downloads in the template. These are expensive when listing a number of versions however, which is why these are dynamic calls in the first place.
The text was updated successfully, but these errors were encountered: