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 accesstoken and ThrottledApplication to admin panel #3836

Conversation

bjornthiberg
Copy link
Contributor

Fixes

Fixes #3711 by @AetherUnbound

Description

This PR adds oauth_provider.models.AccessToken and api.models.oauth.ThrottledApplication tables to the Django Admin UI.

All fields are set as read-only. We are not sure if this is done in the best way. Also, in the issue (#3711), it was said that:

While I initially thought this would be good as read-only, we might even want to enable editing of certain fields so that access can be revoked by maintainers using the Admin UI as well.

We did not know which fields should be set as editable. Hence the draft PR.

Testing Instructions

Test by opening the Django admin panel and looking at the new entries in the sidebar.

Checklist

  • My pull request has a descriptive title (not a vague title likeUpdate index.md).
  • My pull request targets the default branch of the repository (main) or a parent feature branch.
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added or updated tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no visible errors.
  • I ran the DAG documentation generator (if applicable).

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

Co-authored-by: Tore Nylén <toreny@kth.se>
Co-authored-by: Oozna <66669398+Oozna@users.noreply.github.com>
Co-authored-by: Sam Shahriari <62112476+samshahriari@users.noreply.github.com>
Co-authored-by: Sam <80268884+Samkth123@users.noreply.github.com>
@openverse-bot openverse-bot added 🟨 priority: medium Not blocking but should be addressed soon 🌟 goal: addition Addition of new feature 💻 aspect: code Concerns the software code in the repository 🚦 status: awaiting triage Has not been triaged & therefore, not ready for work labels Feb 27, 2024
@samshahriari samshahriari force-pushed the add/accesstoken-throttledapplication-adminpanel branch from eb27147 to 852dd0a Compare February 27, 2024 15:21
@obulat obulat added 🧱 stack: api Related to the Django API and removed 🚦 status: awaiting triage Has not been triaged & therefore, not ready for work labels Feb 29, 2024
@bjornthiberg bjornthiberg marked this pull request as ready for review March 1, 2024 11:56
@bjornthiberg bjornthiberg requested a review from a team as a code owner March 1, 2024 11:56
@sarayourfriend
Copy link
Contributor

Dhruv and I are at WordCamp Asia next week, so pinging @WordPress/openverse-api to reassign reviews on this to folks who are available.

Regarding the editable fields, please make rate_limit_model editable. It's probably also a good one to add to search fields, so we can easily see client IDs in each rate limit model.

I'm not 100% sure how revoking access would work. Deleting the ThrottledApplication instance doesn't seem like the right way to go (just thinking of that being an option), but if we had a new rate limit model called "deny", or something along those lines, that could work. From my perspective this is something for a separate issue, though, as it needs a bit of thought from @WordPress/openverse-maintainers to decide what the technical and decision making process would be.

@zackkrida
Copy link
Member

Hi @bjornthiberg, Openverse maintainers are going to take a look at your PR this week! Thank you for the contribution 🎉

I did want to make sure you saw the notes from @sarayourfriend concerning editable fields. Thank you!

@AetherUnbound AetherUnbound changed the title Add accesstoken and ThrottledApplication to admin panel (#3711) Add accesstoken and ThrottledApplication to admin panel Mar 11, 2024
Copy link
Contributor

@AetherUnbound AetherUnbound left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I have one code request change, in addition to Sara's note about editable fields above.

api/api/admin/__init__.py Outdated Show resolved Hide resolved
@AetherUnbound AetherUnbound marked this pull request as draft March 11, 2024 15:22
@WordPress WordPress deleted a comment from openverse-bot Mar 11, 2024
Copy link
Member

@krysal krysal left a comment

Choose a reason for hiding this comment

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

Thank you for starting this @bjornthiberg. I'm adding just a quick comment to the previous reviews.

api/api/admin/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@krysal krysal left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thank you for working on this, @bjornthiberg and Co 👏

@krysal krysal marked this pull request as ready for review March 18, 2024 22:08
Copy link
Contributor

@AetherUnbound AetherUnbound left a comment

Choose a reason for hiding this comment

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

This looks great and works well locally, thanks for addressing all the changes!

@AetherUnbound AetherUnbound merged commit 697c178 into WordPress:main Mar 19, 2024
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💻 aspect: code Concerns the software code in the repository 🌟 goal: addition Addition of new feature 🟨 priority: medium Not blocking but should be addressed soon 🧱 stack: api Related to the Django API
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add access token and throttled application models to Django admin
7 participants