Initial v2 API Implementation & Doorkeeper OAuth Integration#1276
Merged
aaronskiba merged 76 commits intointegrationfrom Mar 3, 2026
Merged
Initial v2 API Implementation & Doorkeeper OAuth Integration#1276aaronskiba merged 76 commits intointegrationfrom
aaronskiba merged 76 commits intointegrationfrom
Conversation
Co-Authored-By: gjacob24 <[email protected]>
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 <[email protected]> Co-Authored-By: John Pinto <[email protected]>
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.
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`)
…ests Add v2 API Test Coverage and Address Uncovered Bugs
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.
- Edit plan query to check for user role to prevent eager loading - Add set_complete_param function that sets complete flag according to its value if its set to true in the API call
- Edit initialize to include complete flag, which is set to false as default - If flag is true in call, call fetch_all_q_and_a - Add fetch_all_q_and_a function that fetches questions and answers Update plan_presenter
- 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%
Commit e44740b0257e64a467b5483895ca4747bb0f6185 added `doorkeeper` to Gemfile. However, the `Gemfile.lock` changes were not accepted there. Instead, we update `Gemfile.lock` by applying `bundle install` here.
With `hash_token_secrets` enabled, `token.plaintext_token` is only available at creation or rotation time and cannot be retrieved later. This change ensures secure handling of API tokens in line with best practices for hashed token storage. - Update `InternalUserAccessTokenService#rotate!` to return `token.plaintext_token` at creation time - Pass the plaintext token from InternalUserAccessTokensController#create to `app/views/devise/registrations/_v2_api_token.html.erb` - In `_v2_api_token.html.erb`, render the plaintext token when available and display a warning to users to copy and store the token securely, as it will not be shown again after leaving or refreshing the page. - Updated all affected Spec files as well. - The `context 'when user is not authenticated' do` test has been updated. It now enables CSRF protection, enabling the test to accurately capture the behaviour that will be captured in production.
Replace the "Regenerate token" link with a real `<button>` for improved accessibility and native disabled styling. The button is disabled when a token is present, preventing users from generating multiple tokens in rapid succession. This change improves user experience and prevents token spamming.
aaronskiba
commented
Feb 23, 2026
…en-for-internal-users-copy Add Internal v2 API Access Token Generation for Users
13839b5 to
c5a2863
Compare
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)
c5a2863 to
a66339f
Compare
- 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.
b284a24 to
a0a566c
Compare
This commit largely reverts changes to spec/requests/api/v2/internal_user_access_tokens_controller_spec.rb in commit 16c72d4. Also, removed the memoization code from `spec/views/devise/registrations/_api_token.html.erb_spec.rb`. (Memoization is not used in Api::V2::InternalUserAccessTokenService)
ba27573 to
4c9a0b6
Compare
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'
aaronskiba
commented
Feb 25, 2026
| end | ||
| end | ||
|
|
||
| unless @minimal |
Collaborator
Author
There was a problem hiding this comment.
@minimal is referenced here, but not assigned anywhere. I can see the following usage of it in Api:V1:
# app/controllers/api/v1/plans_controller.rb
def index
plans = Api::V1::PlansPolicy::Scope.new(client, Plan).resolve
if plans.present? && plans.any?
@items = paginate_response(results: plans)
@minimal = true
render 'api/v1/plans/index', status: :okAnd then a unless @minimal is performed in app/views/api/v1/plans/_show.json.jbuilder.
Collaborator
Author
There was a problem hiding this comment.
I know that this code is largely based off of https://github.com/CDLUC3/dmptool. They have the following in Api::V2::PlansController#index:
def index
# Scope here is not the Doorkeeper scope, its just to refine the results
@scope = 'mine'
@scope = params[:scope].to_s.downcase if %w[mine public both].include?(params[:scope].to_s.downcase)
# See the Policy for details on what Plans are returned to the Caller based on the AccessToken
plans = Api::V2::PlansPolicy::Scope.new(@client, @resource_owner, @scope).resolve
if plans.present? && plans.any?
plans = plans.sort { |a, b| b.updated_at <=> a.updated_at }
@items = paginate_response(results: plans)
@minimal = true
render 'api/v2/plans/index', status: :ok
else
render_error(errors: _('No Plans found'), status: :not_found)
end
endThis 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.
update html
- The button initially displays 'Copy', then a check mark after it is clicked for two seconds
- 'code' was replaced by `api-token-val`
Add copy button next to V2 API Token
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview:
This PR introduces the initial implementation of DMP Assistant's v2 API, 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 DMPRoadmap/roadmap#3595).