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 support for package upload from frontend to API #268

Merged
merged 24 commits into from
Mar 30, 2021

Conversation

nihaals
Copy link
Member

@nihaals nihaals commented Feb 28, 2021

  • CORS
  • Add auth endpoint to get token for frontend
  • Support token for package upload endpoint
  • Add API for fetching current user's teams
  • Add API for fetching all communities
  • Add API for fetching all categories
  • Add current user's username to current user (blocks Add header/nav bar component thunderstore-ui#44)
  • Add upload limits (e.g. max upload size)

@codecov
Copy link

codecov bot commented Feb 28, 2021

Codecov Report

Merging #268 (ce66a86) into master (e680345) will decrease coverage by 0.60%.
The diff coverage is 91.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #268      +/-   ##
==========================================
- Coverage   83.77%   83.17%   -0.61%     
==========================================
  Files          93       94       +1     
  Lines        2971     2900      -71     
  Branches      307      312       +5     
==========================================
- Hits         2489     2412      -77     
- Misses        399      403       +4     
- Partials       83       85       +2     
Impacted Files Coverage Δ
django/thunderstore/account/authentication.py 85.71% <80.00%> (-7.62%) ⬇️
...o/thunderstore/community/api/experimental/views.py 93.93% <93.93%> (ø)
...derstore/community/api/experimental/serializers.py 100.00% <100.00%> (ø)
django/thunderstore/core/api_urls.py 100.00% <100.00%> (ø)
.../thunderstore/repository/api/experimental/views.py 94.28% <100.00%> (+0.25%) ⬆️
...ango/thunderstore/social/api/experimental/views.py 93.33% <100.00%> (+3.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e680345...ce66a86. Read the comment docs.

@nihaals
Copy link
Member Author

nihaals commented Feb 28, 2021

CORS might only be required for when the frontend is hosted on another domain (so not during the frontend migration period) but it eases development and will be needed at some point (this PR should set most of the foundations needed for the full transition).

@nihaals
Copy link
Member Author

nihaals commented Feb 28, 2021

Categories might be a bit complicated, at least on the frontend side. If you give a list of categories when uploading a package, can't those categories only be applied to a single community's listing as a category has a single parent community? Is it expected to fetch the list of categories for the current community and just not add any categories for the others? Maybe this could be made more clear on the frontend or change how communities/categories are supplied when uploading a package.

Comment on lines 11 to 28
api_experimental_urls = [
path(
"",
include((experimental_urls, "api-experimental"), namespace="api-experimental"),
include(
(repository_experimental_urls, "api-experimental"),
namespace="api-experimental",
),
),
path(
"",
include(community_experimental_urls),
),
]
Copy link
Member Author

Choose a reason for hiding this comment

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

This could be improved

@nihaals nihaals force-pushed the feature/frontend/package-upload-api branch from 696e376 to d8200f3 Compare March 7, 2021 17:59
@nihaals nihaals requested a review from MythicManiac March 12, 2021 18:12
@nihaals
Copy link
Member Author

nihaals commented Mar 12, 2021

Somehow the new push (7ee8917) hasn't triggered anything (CI or an update of this PR)

@nihaals
Copy link
Member Author

nihaals commented Mar 13, 2021

It looks like this PR managed to break due to me updating it at the same time as the GitHub outage

@nihaals nihaals force-pushed the feature/frontend/package-upload-api branch from 7ee8917 to a13df8c Compare March 13, 2021 17:21
@nihaals
Copy link
Member Author

nihaals commented Mar 13, 2021

It looks like this PR managed to break due to me updating it at the same time as the GitHub outage

Looks like this was fixed after rebasing

@nihaals
Copy link
Member Author

nihaals commented Mar 13, 2021

I'm not sure what tests are needed for the API and if old tokens need clearing (see OP) but the functionality is complete and I've tested each endpoint manually

@nihaals nihaals marked this pull request as ready for review March 13, 2021 17:28
@nihaals
Copy link
Member Author

nihaals commented Mar 14, 2021

A possible issue is that tokens can't be revoked. Clearing all tokens on log out doesn't really make sense since there isn't any metadata stored linking a session and a token.

@MythicManiac
Copy link
Member

We could change to our own Token model, it's relatively straight forward to extend the one provided by DRF

@nihaals
Copy link
Member Author

nihaals commented Mar 14, 2021

We could change to our own Token model, it's relatively straight forward to extend the one provided by DRF

And store the session ID with the token then validate the session ID is valid?

@MythicManiac
Copy link
Member

I wouldn't even tie sessions into tokens to be honest, we'll get rid of sessions entirely eventually and just use tokens in their place

@nihaals
Copy link
Member Author

nihaals commented Mar 14, 2021

This would just be while React components are used in templates.

Right now you're able to generate a token using a session but they're not tied to anything but using a session would mean if you logged out it would be revoked and all the other session features would apply. I'm not really sure what else to use without over complicating things for just a transition patch.

Another idea, again tied to sessions, could be sign the session ID with something like itsdangerous which means an extra model isn't even needed and one session can only have one token without any extra lookups.

@nihaals
Copy link
Member Author

nihaals commented Mar 19, 2021

This uses Session as the prefix and simply uses the session ID. I realised there might not be much point doing any kind of encryption as the privileges of a session token and a session ID will be the same as the API coverage increases. The endpoint now essentially behaves like a HTTP Only bypass for the cookie. If we decide not to do anything to the session ID before giving it to the client (e.g. sign/encrypt), we could remove this endpoint and make the session cookie not HTTP only. This would also make it much easier for the frontend to sync with the logged in status and make authenticating much simpler.

@nihaals
Copy link
Member Author

nihaals commented Mar 19, 2021

The slight hackiness of the internal value:

user_id = session.get("_auth_user_id")

Doesn't actually make me feel too uncomfortable as with a few tests this will be very reliable and updating Django shouldn't be something concerning. There might also be a better way to get the user. I'll look at the session middleware and see what it uses.

@nihaals nihaals marked this pull request as draft March 20, 2021 12:40
@nihaals
Copy link
Member Author

nihaals commented Mar 20, 2021

I'll add tests for the create token endpoint once we decide if we're keeping it

@nihaals nihaals requested a review from MythicManiac March 20, 2021 13:05
@nihaals nihaals marked this pull request as ready for review March 20, 2021 13:08
Copy link
Member

@MythicManiac MythicManiac left a comment

Choose a reason for hiding this comment

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

I like the session token solution here 👍

@nihaals nihaals requested a review from MythicManiac March 21, 2021 15:07
@nihaals nihaals marked this pull request as draft March 21, 2021 15:20
@nihaals nihaals marked this pull request as ready for review March 21, 2021 16:32
@nihaals nihaals requested a review from MythicManiac March 26, 2021 22:11
@MythicManiac MythicManiac merged commit eaaa4f0 into master Mar 30, 2021
@MythicManiac MythicManiac deleted the feature/frontend/package-upload-api branch March 30, 2021 23:02
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.

2 participants