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 Users to Register Client Credentials #132

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mureytasroc
Copy link
Contributor

This PR aims to add a developer page to https://platform.pennlabs.org/ where users can register client credentials for applications to access API resources on their behalf. This is useful for student developers who want to access authenticated routes on Penn Labs APIs, e.g. Penn Course Review routes.

@codecov
Copy link

codecov bot commented Jun 1, 2022

Codecov Report

Merging #132 (93e7da8) into master (8545148) will decrease coverage by 0.08%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master     #132      +/-   ##
==========================================
- Coverage   97.60%   97.51%   -0.09%     
==========================================
  Files          15       15              
  Lines         709      724      +15     
==========================================
+ Hits          692      706      +14     
- Misses         17       18       +1     
Flag Coverage Δ
backend 97.51% <93.33%> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
backend/accounts/views.py 99.50% <83.33%> (-0.50%) ⬇️
backend/accounts/serializers.py 100.00% <100.00%> (ø)
backend/accounts/urls.py 100.00% <100.00%> (ø)

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 8545148...93e7da8. Read the comment docs.

@joyliu-q
Copy link
Contributor

joyliu-q commented Jun 1, 2022

As per our last conversation on the call, we should limit the amount of account created. For security reasons/preventing people to abuse this, I think it might be better if we do a whitelist process where users have to request to register for access. Later on if it works out, we can switch over to the model where any user can create one, but only one account.

What are your thoughts on this?

@joyliu-q
Copy link
Contributor

joyliu-q commented Jun 18, 2022

Summary of convo today

We should prioritize security with features like these. Next steps are looking into using scopes to limit access, opening up feature to Penn Course Review (and maybe also Course Plan?), and add more products as we go.

Three options discussed for limiting access:

  1. Who: manual approval via an extra layer of authorization to this PR. Not ideal because PR would be overkill: this can be done by manually creating applications associated with users. Also in the future, it would be cool to automate process.
  2. Where: use scopes to limit the products and features that a user can access with API keys. Will need to get more context but seems like the best solution. Whenever we open up new features or products, we will audit or ensure the features are safe to expose (e.g. rate limiting).
  3. When: We want to implement rate limiting features (if possible). Rate limiting includes: limiting the user requests (throttling, django ratelimit), limiting the objects created (maybe good stackoverflow?).

Additional points related to PR:

  • Can we add some documentation/README on how a user would be able to register Client Credentials?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants