Skip to content

Add API V2 from DMPonline#3587

Merged
aaronskiba merged 44 commits into
developmentfrom
api_v2_dmponline
Jul 16, 2026
Merged

Add API V2 from DMPonline#3587
aaronskiba merged 44 commits into
developmentfrom
api_v2_dmponline

Conversation

@gjacob24

@gjacob24 gjacob24 commented Nov 26, 2025

Copy link
Copy Markdown
Contributor

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:

Notes

hash_application_secrets is 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.

@github-actions

github-actions Bot commented Nov 26, 2025

Copy link
Copy Markdown
1 Warning
⚠️ This PR is too big! Consider breaking it down into smaller PRs.

Generated by 🚫 Danger

@aaronskiba

Copy link
Copy Markdown
Contributor

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?

@momo3404

Copy link
Copy Markdown
Collaborator

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.

Comment thread app/views/api/v2/datasets/_show.json.jbuilder Outdated
@martaribeiro

Copy link
Copy Markdown
Member

Here is the document with the API calls.
API_v2.pdf

Please let us know if needs changing

@gjacob24

Copy link
Copy Markdown
Contributor Author

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.

@momo3404 apologies we missed this. I can do this and push it back next week, if that's ok.

@martaribeiro

Copy link
Copy Markdown
Member

These two docs are more for developers support. It might be useful
[Authorization_Code_Grant_Flow.pdf](https://github.com/user-attachments/files/23827
doorkeeper_gem_for_OAuth_2.0_flows.pdf
178/Authorization_Code_Grant_Flow.pdf)

@aaronskiba

Copy link
Copy Markdown
Contributor

These two docs are more for developers support. It might be useful [Authorization_Code_Grant_Flow.pdf](https://github.com/user-attachments/files/23827 doorkeeper_gem_for_OAuth_2.0_flows.pdf 178/Authorization_Code_Grant_Flow.pdf)

Awesome, thank you. Sorry, the links got a little bungled up in the comment. :P
https://github.com/user-attachments/files/23827180/doorkeeper_gem_for_OAuth_2.0_flows.pdf works.
Could you just provide the Authorization_Code_Grant_Flow.pdf link again?

@gjacob24

gjacob24 commented Dec 1, 2025

Copy link
Copy Markdown
Contributor Author

These two docs are more for developers support. It might be useful [Authorization_Code_Grant_Flow.pdf](https://github.com/user-attachments/files/23827 doorkeeper_gem_for_OAuth_2.0_flows.pdf 178/Authorization_Code_Grant_Flow.pdf)

Awesome, thank you. Sorry, the links got a little bungled up in the comment. :P https://github.com/user-attachments/files/23827180/doorkeeper_gem_for_OAuth_2.0_flows.pdf works. Could you just provide the Authorization_Code_Grant_Flow.pdf link again?

@aaronskiba Here you go:
Authorization_Code_Grant_Flow.pdf

Let us know if this doesn't work.

@aaronskiba

Copy link
Copy Markdown
Contributor

I'm encountering the following when I test the GET /api/v2/heartbeat endpoint (I can also see that the endpoint is returning a 500 for DMPOnline):

$ curl http://127.0.0.1:3000/api/v2/heartbeat
NoMethodError at /api/v2/heartbeat
==================================

undefined method `name' for nil:NilClass

> To access an interactive console with this error, point your browser to: /__better_errors


app/views/api/v2/_standard_response.json.jbuilder, line 18
----------------------------------------------------------

``` ruby
   13   json.ignore_nil!
   14   
   15   json.server @server
   16   json.source "#{request.method} #{request.path}"
   17   json.time Time.now.to_formatted_s(:iso8601)
>  18   json.client @client.name
   19   json.code response.status
   20   json.message Rack::Utils::HTTP_STATUS_CODES[response.status]
   21   
   22   if response.status == 200
   23   

App backtrace

  • app/views/api/v2/_standard_response.json.jbuilder:18:in `_app_views_api_v___standard_response_json_jbuilder__1455696549825964062_81440'
  • app/views/api/v2/error.json.jbuilder:3:in `_app_views_api_v__error_json_jbuilder__3591388735008983784_81420'
  • app/controllers/api/v2/base_api_controller.rb:69:in `handle_internal_server_error'
  • app/controllers/api/v2/base_api_controller.rb:58:in `handle_exception'

Full backtrace
...

Comment on lines +10 to +13
# 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

@aaronskiba aaronskiba Dec 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread app/controllers/api/v2/templates_controller.rb

# json.items []
json.message @payload[:message]
json.details @payload[:details]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Comment thread app/controllers/api/v2/base_api_controller.rb Outdated
Comment thread app/models/oauth_application.rb Outdated
Comment thread app/policies/api/v2/plans_policy.rb Outdated
Comment on lines +24 to +26
Plan.joins(:roles)
.where(roles: { user_id: @resource_owner.id, active: true })
.distinct

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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))

@momo3404

Copy link
Copy Markdown
Collaborator

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.
curl -i -X GET "http://127.0.0.1:3000/oauth/authorize?response_type=code&client_id=[my_client_id]&redirect_uri=[redirect_uri]" HTTP/1.1 302 Found x-frame-options: SAMEORIGIN x-xss-protection: 0 x-content-type-options: nosniff x-permitted-cross-domain-policies: none referrer-policy: strict-origin-when-cross-origin location: http://127.0.0.1:3000/users/sign_in content-type: text/html; charset=utf-8 cache-control: no-cache

But the command still worked, because after calling Doorkeeper::AccessGrant.first I found my authorization which I used for the POST command, and that worked perfectly. So is this a known error, or something I'm missing? I was also signed in when making the request.

Comment thread app/views/api/v2/templates/index.json.jbuilder
@gjacob24 gjacob24 requested a review from johnpinto1 December 18, 2025 11:17
@gjacob24

Copy link
Copy Markdown
Contributor Author

I'm encountering the following when I test the GET /api/v2/heartbeat endpoint (I can also see that the endpoint is returning a 500 for DMPOnline):

$ curl http://127.0.0.1:3000/api/v2/heartbeat
NoMethodError at /api/v2/heartbeat
==================================

undefined method `name' for nil:NilClass

> To access an interactive console with this error, point your browser to: /__better_errors


app/views/api/v2/_standard_response.json.jbuilder, line 18
----------------------------------------------------------

``` ruby
   13   json.ignore_nil!
   14   
   15   json.server @server
   16   json.source "#{request.method} #{request.path}"
   17   json.time Time.now.to_formatted_s(:iso8601)
>  18   json.client @client.name
   19   json.code response.status
   20   json.message Rack::Utils::HTTP_STATUS_CODES[response.status]
   21   
   22   if response.status == 200
   23   

App backtrace

* app/views/api/v2/_standard_response.json.jbuilder:18:in `_app_views_api_v___standard_response_json_jbuilder__1455696549825964062_81440'

* app/views/api/v2/error.json.jbuilder:3:in `_app_views_api_v__error_json_jbuilder__3591388735008983784_81420'

* app/controllers/api/v2/base_api_controller.rb:69:in `handle_internal_server_error'

* app/controllers/api/v2/base_api_controller.rb:58:in `handle_exception'

Full backtrace ...

@aaronskiba this is fixed and should be working now

@aaronskiba

aaronskiba commented Dec 22, 2025

Copy link
Copy Markdown
Contributor

Full backtrace ...

@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:

$ curl -H "Authorization: Bearer ${BEARER_TOKEN}"      -H "Accept: application/json"      "http://127.0.0.1:3000/api/v2/plans"

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:

{
  "source": "GET /api/v2/plans",
  "time": "2025-12-22T12:03:00-07:00",
  "code": 500,
  "message": [
    "There was a problem in the server."
  ]

I also get the following in the rails console:

Processing by Api::V2::PlansController#index as JSON
  Doorkeeper::AccessToken Load (1.3ms)  SELECT "oauth_access_tokens".* FROM "oauth_access_tokens" WHERE "oauth_access_tokens"."token" = $1 LIMIT $2  [["token", "lp5znDvJAOZndbJRWAZ1eeZ_5NR8ZZs_2raswR-YsgQ"], ["LIMIT", 1]]
Exception message: undefined method `include?' for nil:NilClass

        raise Pundit::NotAuthorizedError unless @scopes.include?('read')
                                                       ^^^^^^^^^
  Rendering api/v2/error.json.jbuilder
  Rendered api/v2/_standard_response.json.jbuilder (Duration: 0.7ms | Allocations: 400)
  Rendered api/v2/error.json.jbuilder (Duration: 1.8ms | Allocations: 728)
Completed 500 Internal Server Error in 10ms (Views: 3.8ms | ActiveRecord: 0.9ms | Allocations: 7697)

This must be related to @scopes = doorkeeper_token.scopes.to_a if doorkeeper_token being removed in commit 3645cae

I am also looking at the @application = ApplicationService.application_name change in app/controllers/api/v2/base_api_controller.rb. I like this renaming because it is consistent with app/controllers/api/v1/base_api_controller.rb. However, json.server @server still exists in app/views/api/v2/_standard_response.json.jbuilder. Changing it to json.application @application should fix things and again align with the api/v1 approach.

The assignments to @client and @resource_owner were removed in commit 3645cae. This will need to be addressed as well.

@gjacob24

Copy link
Copy Markdown
Contributor Author

Full backtrace ...

@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:

$ curl -H "Authorization: Bearer ${BEARER_TOKEN}"      -H "Accept: application/json"      "http://127.0.0.1:3000/api/v2/plans"

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:

{
  "source": "GET /api/v2/plans",
  "time": "2025-12-22T12:03:00-07:00",
  "code": 500,
  "message": [
    "There was a problem in the server."
  ]

I also get the following in the rails console:

Processing by Api::V2::PlansController#index as JSON
  Doorkeeper::AccessToken Load (1.3ms)  SELECT "oauth_access_tokens".* FROM "oauth_access_tokens" WHERE "oauth_access_tokens"."token" = $1 LIMIT $2  [["token", "lp5znDvJAOZndbJRWAZ1eeZ_5NR8ZZs_2raswR-YsgQ"], ["LIMIT", 1]]
Exception message: undefined method `include?' for nil:NilClass

        raise Pundit::NotAuthorizedError unless @scopes.include?('read')
                                                       ^^^^^^^^^
  Rendering api/v2/error.json.jbuilder
  Rendered api/v2/_standard_response.json.jbuilder (Duration: 0.7ms | Allocations: 400)
  Rendered api/v2/error.json.jbuilder (Duration: 1.8ms | Allocations: 728)
Completed 500 Internal Server Error in 10ms (Views: 3.8ms | ActiveRecord: 0.9ms | Allocations: 7697)

This must be related to @scopes = doorkeeper_token.scopes.to_a if doorkeeper_token being removed in commit 3645cae

I am also looking at the @application = ApplicationService.application_name change in app/controllers/api/v2/base_api_controller.rb. I like this renaming because it is consistent with app/controllers/api/v1/base_api_controller.rb. However, json.server @server still exists in app/views/api/v2/_standard_response.json.jbuilder. Changing it to json.application @application should fix things and again align with the api/v1 approach.

The assignments to @client and @resource_owner were removed in commit 3645cae. This will need to be addressed as well.

Hey @aaronskiba, so sorry about this. I have changed @server to @application, to keep it consistent with V1. I've also put back @scopes and @resource_owner. That was removed unintentionally, so thanks for spotting that. You might still get other errors because we haven't fixed and tested them all, but I think this might be the last change I push before the long break to avoid causing more errors :D

Also, please feel free to commit any changes you feel are necessary (some of which you have already added as comments in this PR).

@aaronskiba

This comment was marked as resolved.

gjacob24 and others added 2 commits January 27, 2026 13:10
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>
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.
momo3404 and others added 10 commits February 10, 2026 09:49
- 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%
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`
@aaronskiba aaronskiba marked this pull request as draft March 12, 2026 16:35
@aaronskiba aaronskiba marked this pull request as draft March 12, 2026 16:35
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.
@aaronskiba aaronskiba changed the base branch from main to development July 13, 2026 17:52
@aaronskiba aaronskiba marked this pull request as ready for review July 14, 2026 22:41
aaronskiba

This comment was marked as resolved.

@aaronskiba

Copy link
Copy Markdown
Contributor

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 momo3404 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Everything looks as expected to me!

@aaronskiba aaronskiba merged commit c7167ab into development Jul 16, 2026
9 checks passed
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.

4 participants