Skip to content

Add API V2 from DMPonline#3587

Open
gjacob24 wants to merge 24 commits intomainfrom
api_v2_dmponline
Open

Add API V2 from DMPonline#3587
gjacob24 wants to merge 24 commits intomainfrom
api_v2_dmponline

Conversation

@gjacob24
Copy link
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.)

@github-actions
Copy link

github-actions bot commented Nov 26, 2025

</tr>
1 Error
🚫

Please include a CHANGELOG entry.

You can find it at [CHANGELOG.md](https://github.com/DMPRoadmap/roadmap/blob/main/CHANGELOG.md).
1 Warning
⚠️ This PR is too big! Consider breaking it down into smaller PRs.

Generated by 🚫 Danger

@aaronskiba
Copy link
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
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.

@martaribeiro
Copy link
Member

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

Please let us know if needs changing

@gjacob24
Copy link
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
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
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
Copy link
Contributor Author

gjacob24 commented Dec 1, 2025

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
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
Copy link
Contributor

@aaronskiba aaronskiba Dec 8, 2025

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.


# json.items []
json.message @payload[:message]
json.details @payload[:details]
Copy link
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 on lines 24 to 26
Plan.joins(:roles)
.where(roles: { user_id: @resource_owner.id, active: true })
.distinct
Copy link
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
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.

@gjacob24 gjacob24 requested a review from johnpinto1 December 18, 2025 11:17
@gjacob24
Copy link
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
Copy link
Contributor

aaronskiba commented Dec 22, 2025

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
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.
aaronskiba and others added 4 commits January 27, 2026 13:57
These changes resolve the following issues uncovered via the API v2 tests:

- app/views/api/v2/datasets/_show.json.jbuilder
  - Failure rendering output.doi_url when output is present (undefined method `doi_url`)
- config/routes.rb
  - Spec failures due to undefined route helpers in test context (e.g., `api_v2_templates_path`)
Add v2 API Test Coverage and Address Uncovered Bugs
Executed `bundle exec rake translation:sync` while `config.disable_yaml = true`, allowing us to sync the doorkeeper translations.
Add navigation link for Doorkeeper OAuth Applications management access in the admin dropdown menu.
- Link only renders for super admins.
Allow HTTP OAuth redirects in test/dev for easier local development, while maintaining HTTPS enforcement in all other environments.
Remove OauthApplication, OauthAccessToken, and OauthAccessGrant model files.

These classes were empty wrappers around Doorkeeper models that inherited directly from ApplicationRecord, bypassing Doorkeeper's validations, associations, and behavior.

These wrapper classes were never referenced and posed a maintenance risk if used inadvertently in the future. Reference Doorkeeper's models directly instead (Doorkeeper::Application, Doorkeeper::AccessToken, and Doorkeeper::AccessGrant).
`a.present? && !a.blank?` was redundant.
- Changed to simply `a.present?`

Replaces map + post-filter with filter_map
- Eliminates the need for `ret.select { |item| item[:description].present? }`

Extracts question-theme lookup and q & a formatting into private helper methods.
- Improves readability and helps address rubocop offences
Rescuing from Exception would also catch system-level exceptions, which would likely be taking things too far (e.g. system-level errors like `Interrupt` and
`SystemExit`). StandardError covers all application-level errors while still ensuring clients get consistent JSON responses.
Prior to this change, the `GET /me` endpoint exposed potentially sensitive user attributes (e.g. `api_token`) as well as  internal-only fields (e.g. `other_organisation`, `department_id`, `dmponline3`, etc.).

Now the endpoint only returns useful information (note the language.name rather than language_id and org.name rather than org_id improvements as well).
Replaced raw SQL with ActiveRecord query methods + enum scopes
- Improves readability and eliminates SQL injection risk
Move theme filtering from Ruby to SQL using joins for better performance and scalability.
Previously, `fetch_q_and_a` iterated over all plan questions and searched for matching answers in Ruby, performing theme filtering and Q&A formatting in memory. This caused unnecessary loops, potential N+1 lookups, and inefficient handling of multi-theme questions. Accessing questions directly via `Plan.questions` also triggered joins through sections, phases, and  templates, along with ordering (due to default scope), which added query overhead.

The refactored version:

- Queries answers directly via a join to question themes, filtering at the DB level.
- Accesses questions through the answers (`answer.question`) instead of `plan.questions`, avoiding heavy joins and unnecessary default-scoped sorting.
Previously, the /api/v2/plans endpoint triggered many N+1 queries when rendering plan associations, causing slow response times. This change adds .includes() for all associations that were previously triggering Bullet N+1 warnings.

Benchmark comparison (local dev, 10 sequential requests):

| Metric            | Before  | After  |
| ----------------- | ------- | ------ |
| Mean request time | 1909 ms | 492 ms |
| Requests/sec      | 0.52    | 2.03   |
| Max request time  | 2628 ms | 641 ms |

- This simple comparison shows that these changes make the  `GET api/v2/plans` ~4x faster.
- Use warden.authenticate! (instead of manually storing session[:user_return_to] + explicitly redirecting to login page)
- Simplify conditional logic by checking "native" path first
- Replace hardcoded "/oauth/authorize/native path" string with native_oauth_authorization_path

When warden.authenticate! is invoked, Warden both automatically stores the request URL in the session and reroutes the unauthenticated user to the login page.

The existing session[:user_return_to] checks in ApplicationController continue to work. This reduces custom code and leverages Devise's built-in authentication handling for better maintainability.

NOTE: For better alignment with Devise conventions and automatic session cleanup, we could refactor ApplicationController to use stored_location_for(resource) instead of session[:user_return_to].
- Use start_with?(oauth_authorization_path) instead of include?('/oauth/authorize') for stricter path matching
- Use stored_location_for(:user) instead of session[:user_return_to] for automatic cleanup and explicit Devise scope usage
  - OAuth flow detection is tied to session[:user_return_to], so the :user scope is explicit and correct
- Add ? suffix to predicate method name

Improves security (prevents substring matches), maintainability (uses framework APIs with explicit scope), and follows Ruby conventions.
With the exception of specific 'disallowed paths' (i.e. `new_user_session_path` for `after_sign_in_path_for` vs `new_user_session_path` and `new_user_registration_path` for `after_sign_up_path_for`, these helpers are otherwise identical. This change refactors the code so that these helpers better adhere to DRY principles.

We introduce `def after_auth_path(disallowed_paths:)`. `after_sign_in_path_for` and `after_sign_up_path_for` each call `after_auth_path` with an array containing their aforementioned disallowed paths. Some small additional refactoring is also performed. `return root_path if request.referer.nil? || from_external_domain?` allows for possible early return before assigning `referer_path`. Also, repeated `referer_path.eql?(path)` statements have been replaced with a single `disallowed_paths.include?(referer_path)` check.
Amongst its changes, this commit removes `warden.authenticate!(scope: :user)`.

When warden.authenticate! is invoked, the OAuth flow is preserved while the user signs in. However, our current custom implementation of ApplicationController#after_sign_in_path_for only returns `stored_location_for(resource)` for the `oauth/authorize` path.
- We could further modify after_sign_in_path_for to also return `stored_location_for(resource)` for the `oauth/applications` path. However, it is simple enough for a super admin to navigate to `oauth/applications` via the admin panel.
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