Add API V2 from DMPonline#3587
Conversation
Generated by 🚫 Danger |
|
Thank you for this PR. During today's DMPRoadmap Monthly all-team meeting I mentioned how there are also related DMPTool v2 API test files. Would you like me to add them to this PR? |
|
I'm seeing a lot of rubocop errors in here. This might be a difference in the configuration of Rubocop between the Roadmap repo and DMP Online. Let me know how you'd want to approach this. If possible, I could start a PR attempting to fix these. |
|
Here is the document with the API calls. Please let us know if needs changing |
@momo3404 apologies we missed this. I can do this and push it back next week, if that's ok. |
|
These two docs are more for developers support. It might be useful |
Awesome, thank you. Sorry, the links got a little bungled up in the comment. :P |
@aaronskiba Here you go: Let us know if this doesn't work. |
|
I'm encountering the following when I test the App backtrace
Full backtrace |
| # Remove `null: false` if you are planning to use grant flows | ||
| # that doesn't require redirect URI to be used during authorization | ||
| # like Client Credentials flow or Resource Owner Password. | ||
| t.text :redirect_uri, null: false |
There was a problem hiding this comment.
Since we will be implementing the v2 API Client Credentials Flow in a future piece of work, maybe we should remove null: false on redirect_uri, as the code comment advises.
|
|
||
| # json.items [] | ||
| json.message @payload[:message] | ||
| json.details @payload[:details] |
There was a problem hiding this comment.
I can see @payload[:message] being assigned within app/controllers/api/v2/base_api_controller.rb. But is @payload[:details] being set anywhere? Maybe it can be removed.
| Plan.joins(:roles) | ||
| .where(roles: { user_id: @resource_owner.id, active: true }) | ||
| .distinct |
There was a problem hiding this comment.
It might be even better to reuse the Plan.active(@resource_owner) scope for this query. Note that a bit of refactoring could be performed in that scope to make this more efficient (i.e. don't use includes(:template, :roles))
|
Please let me know if I've done something wrong here, but I keep getting 302s when executing the GET command from the Authorization Code Grant Flow. But the command still worked, because after calling |
@aaronskiba this is fixed and should be working now |
Cool. I can get the 200 via the heartbeat whether I'm signed in or signed in now. Unfortunately, I think the fix may have introduced other bugs. I am testing the following curl request: This works and returns the expected JSON response on #3589, which is not up-to-date with these lastest heartbeat changes. However, when I test on this PR, I get the following response: I also get the following in the rails console: This must be related to I am also looking at the The assignments to |
Hey @aaronskiba, so sorry about this. I have changed Also, please feel free to commit any changes you feel are necessary (some of which you have already added as comments in this PR). |
This comment was marked as resolved.
This comment was marked as resolved.
Co-Authored-By: gjacob24 <97518971+gjacob24@users.noreply.github.com>
Fix crashes when `@client` is nil (e.g., on heartbeat endpoint where doorkeeper_token is not present). - `@client = doorkeeper_token&.application` simplifies `@client` assignment. - Guard log_access and add `@caller` for safe JSON output Align JSON response keys with V1 - server -> application - client -> caller Co-Authored-By: gjacob24 <97518971+gjacob24@users.noreply.github.com> Co-Authored-By: John Pinto <8876215+johnpinto1@users.noreply.github.com>
8ae4b5d to
a04513b
Compare
This test suite was adapted from the v2 API tests in CDLUC3/dmptool. The original request and view specs were copied and then refactored to align with our v2 API. Supporting factories and spec helper files were updated as needed to accommodate the new coverage.
- Add complete flag data to extension in json
- Pre-fetch the answers, questions, and sections as part of the initial plans query (but only when we are in fact fetching all of the questions and answers). - This speeds up the mean request time by 25%
Add complete plan flag to V2 API
More information regarding these Doorkeeper rake tasks can be found at https://doorkeeper.gitbook.io/guides/internals/rake
Generate and apply migration to remove NOT NULL constraint from `oauth_applications.redirect_uri`. This works in conjunction with `allow_blank_redirect_uri true` in `config/initializers/doorkeeper.rb`, allowing us to save `OAuthApplications` with blank `redirect_uri` values for URI-less flows like client_credentials or password grant flows. NOTE: Applications using `authorization_code` (or other flows that require a redirect URI) must still have a non-blank `redirect_uri`. Otherwise, `/oauth/authorize` requests with `response_type=code` will fail. More info is available here: https://github.com/doorkeeper-gem/doorkeeper/wiki/Allow-blank-redirect-URI-for-Applications
- Enable `hash_application_secrets` and `hash_token_secrets` in Doorkeeper initializer for improved security. - This stores application secrets and access tokens as hashes in the database, reducing risk if the database is compromised. - Note: `hash_token_secrets` is incompatible with `reuse_access_token`, so token reuse is now disabled/removed (see warning in Doorkeeper docs). - For more details, see: https://doorkeeper.gitbook.io/guides/security/token-and-application-secrets
Move the "read" scope authorization check to a shared `before_action` in `BaseApiController` - Removes redundant checks from individual actions in `PlansController` and `TemplatesController` - Allows for the safe removal of `@scopes` from `base_response_content`
Prior to this change, `LocaleService.default_locale` was always assigned to `json.language`. - Now we use `plan.owner&.language&.abbreviation` (or `LocaleService.default_locale` as a fallback)
- Updated `it 'includes the :language'` test to reflect changes made in b238c8128e0fefb9ea25cc7528e5be2c499d9318. - Updated `Api::V1` references within spec files with `Api::V2`
NOTE: The prior comment stated the following: # Attach the first data_curation role as the data_contact, otherwise # add the contributor to the contributors array However, the contributor was ALWAYS added to the contributors array
Moved complex query logic from policies to `for_api_v2` scopes in Template and Plan models.
Addresses the following Bullet warning: user: aaron GET /api/v2/templates USE eager loading detected Identifier => [:identifier_scheme] Add to your query: .includes([:identifier_scheme]) Call stack /home/aaron/Documents/GitHub/roadmap/app/presenters/api/v2/org_presenter.rb:9:in `block in affiliation_id' /home/aaron/Documents/GitHub/roadmap/app/presenters/api/v2/org_presenter.rb:9:in `affiliation_id' /home/aaron/Documents/GitHub/roadmap/app/views/api/v2/orgs/_show.json.jbuilder:11:in `block in _app_views_api_v__orgs__show_json_jbuilder__1237609272914133563_48080' /home/aaron/Documents/GitHub/roadmap/app/views/api/v2/orgs/_show.json.jbuilder:10:in `_app_views_api_v__orgs__show_json_jbuilder__1237609272914133563_48080' /home/aaron/Documents/GitHub/roadmap/app/views/api/v2/templates/index.json.jbuilder:16:in `block (3 levels) in _app_views_api_v__templates_index_json_jbuilder___4313931085157922975_47640' /home/aaron/Documents/GitHub/roadmap/app/views/api/v2/templates/index.json.jbuilder:15:in `block (2 levels) in _app_views_api_v__templates_index_json_jbuilder___4313931085157922975_47640' /home/aaron/Documents/GitHub/roadmap/app/views/api/v2/templates/index.json.jbuilder:8:in `block in _app_views_api_v__templates_index_json_jbuilder___4313931085157922975_47640' /home/aaron/Documents/GitHub/roadmap/app/views/api/v2/templates/index.json.jbuilder:5:in `_app_views_api_v__templates_index_json_jbuilder___4313931085157922975_47640' /home/aaron/Documents/GitHub/roadmap/app/controllers/api/v2/templates_controller.rb:13:in `index'
This change further addresses Bullet warnings that were earlier addressed in commit 6ed969b
This change updates `Api::V2::PlansController#show` to use the `.for_api_v2` scope, and applies eager loading of answers (`.includes(answers: { question: :section })`) based on the `complete` param--making the `show` action consistent with `index`.
A new `plans_scope` helper DRYs up the controller by centralizing the shared scope and eager loading logic. Since `.for_api_v2` already filters by `.where(roles: { user_id: user_id, active: true })`, only a presence check is now required in the policy.
|
I've approved this PR, but considering I've done a lot of work on it, at least one other approval would be nice. I've set @martaribeiro and @momo3404 as reviewers. Because @gjacob24 put up the PR, I don't think I can set her as a reviewer. However, if she wants to review the additional changes, or even merge it into development, that would be great. Thank you. |
momo3404
left a comment
There was a problem hiding this comment.
Everything looks as expected to me!
PR for issue #3586
This issue branch uses the doorkeeper gem to implement the Authorization Code Grant Flow (ACGF) part of the v2 API.
(The Client Credentials Flow (CCF) part of the v2 API (which is what the org-admins will use) will be implemented separately in a future piece of work.)
Overview:
This PR introduces the initial v2 API implementation, with OAuth 2.0 authentication powered by the Doorkeeper gem. This is a foundation for further v2 API enhancements and ensures secure, standards-based authentication for future integrations.
Key Features:
GET /api/v2/plansGET /api/v2/plans/:idGET /api/v2/templatesGET /api/v2/meGET /api/v2/heartbeatGET /plansandGET /plans/:idcalls serialize plan data according to the RDA DMP Common Standard JSON schema.GET /plansandGET /plans/:id(Add complete plan flag to V2 API #3595).Notes
hash_application_secretsis enabled. However, when an OAuth application is generated through the Doorkeeper form, the user will not have access to the pre-hashed secret. This limitation will be addressed in a future PR.