-
Notifications
You must be signed in to change notification settings - Fork 118
Add API V2 from DMPonline #3587
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
base: main
Are you sure you want to change the base?
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.
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] |
There was a problem hiding this comment.
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.
| # created_at: :datetime | ||
| # updated_at: :datetime | ||
|
|
||
| class OauthApplication < ApplicationRecord |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little worried about directly inheriting from ApplicationRecord and simply mapping to the Doorkeeper tables in app/models/oauth_access_grant.rb, app/models/oauth_access_token.rb, and app/models/oauth_application.rb. These classes will not include Doorkeeper’s behaviour (validations, associations, etc.).
Currently, @client = OauthApplication.find(doorkeeper_token.application_id) if doorkeeper_token appears to be the only code that references these models (see app/controllers/api/v2/base_api_controller.rb), which should be safe. But I worry about issues arising in the future.
Unless this was implemented for a specific reason, I might suggest one of the following:
- Remove these classes altogether
- These tables/models can instead be referenced via
Doorkeeper::Application,Doorkeeper::AccessToken;, andDoorkeeper::AccessGrant. (e.g.@client = Doorkeeper::Application.find(doorkeeper_token.application_id) if doorkeeper_token)
- Update each of the classes to inherit from the appropriate
Doorkeeper::Application,Doorkeeper::AccessToken;, orDoorkeeper::AccessGrant.
class OauthApplication < Doorkeeper::Application; end
class OauthAccessToken < Doorkeeper::AccessToken; end
class OauthAccessGrant < Doorkeeper::AccessGrant; end| Plan.joins(:roles) | ||
| .where(roles: { user_id: @resource_owner.id, active: true }) | ||
| .distinct |
There was a problem hiding this comment.
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))
|
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.
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.
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.)