diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index 8a96e99..e5bcdc6 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -27,7 +27,7 @@ jobs: - name: Set up Ruby uses: ruby/setup-ruby@v1 with: - ruby-version: 3.2 + ruby-version: 3.3 bundler-cache: true - name: Installing dependencies run: bundle install diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 5b899bd..b8e45dc 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -11,7 +11,7 @@ jobs: strategy: matrix: os: [ubuntu-latest] - ruby: ['3.0', '3.1', '3.2', '3.3'] + ruby: ['3.3', '3.4', '4.0'] runs-on: ${{ matrix.os }} services: redis: diff --git a/.gitignore b/.gitignore index 5109efd..671150a 100644 --- a/.gitignore +++ b/.gitignore @@ -10,3 +10,5 @@ /spec/reports/ /tmp/ /dump.rdb +/vendor/bundle/ + diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml new file mode 100644 index 0000000..ba0196b --- /dev/null +++ b/.pre-commit-config.yaml @@ -0,0 +1,20 @@ +# Hooks run by prek (https://github.com/j178/prek), or pre-commit. Install them +# with `prek install --hook-type pre-commit`. +repos: + - repo: https://github.com/gitleaks/gitleaks + rev: v8.30.1 + hooks: + - id: gitleaks + stages: [pre-commit] + + - repo: local + hooks: + # Runs on every commit, not just Gemfile.lock changes, so new advisories + # against unchanged gems block the commit too. + - id: security-audit + name: Security Audit + entry: bundle exec rake security:audit + language: system + pass_filenames: false + always_run: true + stages: [pre-commit] diff --git a/.rubocop.yml b/.rubocop.yml index c06ba19..6e9142d 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,9 +1,11 @@ -require: +plugins: - rubocop-rails - rubocop-performance AllCops: - TargetRubyVersion: 2.5 + TargetRubyVersion: 3.3 + NewCops: disable + SuggestExtensions: false Include: - '**/Rakefile' - '**/config.ru' @@ -112,7 +114,7 @@ Metrics/BlockLength: Enabled: false CountComments: true # count full line comments? Max: 25 - ExcludedMethods: [] + AllowedMethods: [] Exclude: - "**/*.rake" - "spec/**/*" @@ -172,7 +174,7 @@ Style/FrozenStringLiteralComment: to help transition from Ruby 2.3.0 to Ruby 3.0. Enabled: false -Style/FlipFlop: +Lint/FlipFlop: Description: 'Checks for flip flops' StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#no-flip-flops' Enabled: false @@ -225,7 +227,7 @@ Style/LineEndConcatenation: line end. Enabled: false -Metrics/LineLength: +Layout/LineLength: Description: 'Limit lines to 80 characters.' StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#80-character-limits' Max: 150 @@ -306,7 +308,7 @@ Style/PerlBackrefs: StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#no-perl-regexp-last-matchers' Enabled: false -Naming/PredicateName: +Naming/PredicatePrefix: Description: 'Check the names of predicate methods.' StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#bool-methods-qmark' ForbiddenPrefixes: @@ -498,7 +500,7 @@ Lint/CircularArgumentReference: Description: "Don't refer to the keyword argument in the default value." Enabled: false -Lint/ConditionPosition: +Layout/ConditionPosition: Description: >- Checks for condition placed in a confusing position relative to the keyword. @@ -606,7 +608,7 @@ Performance/ReverseEach: Reference: 'https://github.com/JuanitoFatas/fast-ruby#enumerablereverseeach-vs-enumerablereverse_each-code' Enabled: false -Performance/Sample: +Style/Sample: Description: >- Use `sample` instead of `shuffle.first`, `shuffle.last`, and `shuffle[Fixnum]`. @@ -675,3 +677,7 @@ Rails/TimeZone: Rails/Validation: Description: 'Use validates :attribute, hash of validations.' Enabled: false + +# This is a gem, not a Rails app: there is no :environment task to depend on. +Rails/RakeEnvironment: + Enabled: false diff --git a/CHANGELOG.md b/CHANGELOG.md index 90bfc77..a55ab3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,34 @@ +## 2.0.0 (Oct 26, 2026) + +* **Breaking:** Require Ruby 3.3 or newer. Ruby 3.0, 3.1 and 3.2 are end-of-life. + +* **Breaking:** Replace rest-client with Faraday (see `docs/adr/0001-replace-rest-client-with-faraday.md` for the upgrade guide): + * `Response#request` is now a `Procore::Request` (`url`, `method`, `headers`, `payload`) instead of a `RestClient::Request`. + * TLS certificate errors, DNS failures and unreachable hosts now raise `Procore::APIConnectionError`. + * `APIConnectionError#cause` and `Requestable::HTTP_EXCEPTIONS` use Faraday exception classes. + * The gem no longer loads rest-client. + +* Fix token revoke failures raising `OAuth2::Error` instead of `Procore::OAuthError`. + +* Fix request logs never including the response's `X-Request-Id`. + +* Support Ruby 3.4 and 4.0: + * Replace `OpenStruct` (no longer bundled with Ruby 4.0) with `Struct` in `Procore::OAuthError`. + * Add `pstore` as a runtime dependency (no longer bundled with Ruby 4.0). + * Fix `Procore::Auth::Stores::File` failing to load saved tokens on Ruby 3.4+ (`Psych::DisallowedClass`). + +* Fix the gem failing outside Rails: the Session, Redis and Dalli stores, `sync` and `OAuthError` relied on ActiveSupport core extensions the gem never required. `Procore::Auth::Token` now defines `to_h` and `to_json`, and the gem requires `activesupport` 7.0 or newer. + +* **Breaking:** `Response#pagination` paths now include the API version (`"rest/v1.1/projects?page=2"`, `"vapid/projects?page=2"`) instead of stripping it, and `get`/`delete` accept such paths without adding `default_version`. Following a next link from a v1.1, v2.0 or vapid response previously requested the default version. + +* Fix the `Accept` header being sent as `Accepts`. + +* Fix empty response bodies (such as 204 No Content) parsing to the String `"{}"` instead of an empty Hash. + +* Fix `delete` ignoring `options:`, so `company_id` was never sent as the `Procore-Company-Id` header on DELETE requests. + +* **Security:** stop logging request headers on DELETE requests. The log line included the `Authorization` header, exposing the access token to anything with access to the logs. All log lines now also replace bearer tokens with `###`. + ## 1.3.1 (Jan 9, 2024) * Fix a bug with the retry logic in `with_response_handling` resulting in performing one more retry than expected . diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f5148f9..cf30553 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -15,7 +15,8 @@ Once a GitHub issue is accepted and assigned to you, please follow general workf - `bundle install` to install the dependencies - Make sure tests pass locally - - You'll need to spin up a few containers (redis and memcached) to run the tests. You can do this by running `docker-compose up -d` in the root of the project. + - You'll need to spin up a few containers (redis and memcached) to run the tests. You can do this by running + `docker compose up -d` in the root of the project. - Run tests with `COVERAGE=true bundle exec rake` 3. Commit your changes in the branch you created in step 2. All commits need to be signed-off. Check the [legal](#legal) section bellow for more details. diff --git a/README.md b/README.md index 2aa5f6e..bd47cdc 100644 --- a/README.md +++ b/README.md @@ -6,6 +6,7 @@ #### Table of Contents - [Installation](#installation) +- [Upgrading to 2.0](#upgrading-to-20) - [Configuration](#configuration) - [Making Requests](#making-requests) - [Usage](#usage) @@ -15,12 +16,14 @@ - [Change Number of Results](#change-number-of-results) - [Sync Actions](#sync-actions) - [Stores](#stores) + - [Choosing a Store](#choosing-a-store) - [Session Store](#session-store) - [Redis Store](#redis-store) - [Dalli Store](#dalli-store) - [ActiveRecord Store](#activerecord-store) - [File Store](#file-store) - [Memory Store](#memory-store) + - [Custom Stores](#custom-stores) - [Full Example](#full-example) - [Contributing](#contributing) @@ -32,6 +35,88 @@ Add this line to your application's Gemfile: gem "procore" ``` +The gem requires Ruby 3.3 or newer. + +## Upgrading to 2.0 + +Version 2.0 requires Ruby 3.3 or newer, and sends requests with +[Faraday](https://lostisland.github.io/faraday/) instead of rest-client. The +request methods (`get`, `post`, `put`, `patch`, `delete`, `sync`), their +arguments, `Procore::Response` and the configuration options are unchanged. +What changes is mostly which exceptions you see. The reasoning is recorded in +[docs/adr/0001-replace-rest-client-with-faraday.md](docs/adr/0001-replace-rest-client-with-faraday.md). + +### Exceptions + +Every failure now raises a `Procore::Error` subclass, so +`rescue Procore::Error` catches anything the client raises for a failed request. + +| Failure | 1.x raises | 2.0 raises | `cause` in 1.x | `cause` in 2.0 | +|-----------------------------------|-----------------------------------------|--------------------------|---------------------------------------|-----------------------------| +| HTTP 401 | `AuthorizationError` | unchanged | – | – | +| HTTP 403 | `ForbiddenError` | unchanged | – | – | +| HTTP 404 | `NotFoundError` | unchanged | – | – | +| HTTP 400, 422 | `InvalidRequestError` | unchanged | – | – | +| HTTP 429 | `RateLimitError` | unchanged | – | – | +| Any other non-2xx status | `ServerError` | unchanged | – | – | +| Connect timeout | `APIConnectionError` | unchanged | `RestClient::Exceptions::OpenTimeout` | `Faraday::ConnectionFailed` | +| Read timeout | `APIConnectionError` | unchanged | `RestClient::Exceptions::ReadTimeout` | `Faraday::TimeoutError` | +| Connection refused | `APIConnectionError` | unchanged | `Errno::ECONNREFUSED` | `Faraday::ConnectionFailed` | +| Connection reset | `APIConnectionError` | unchanged | `Errno::ECONNRESET` | `Faraday::ConnectionFailed` | +| Server closed the connection | `APIConnectionError` | unchanged | `RestClient::ServerBrokeConnection` | `Faraday::ConnectionFailed` | +| Access token could not be fetched | `APIConnectionError` | unchanged | `Procore::OAuthError` | unchanged | +| TLS certificate error | `RestClient::SSLCertificateNotVerified` | **`APIConnectionError`** | – | `Faraday::SSLError` | +| DNS lookup failed | `SocketError` | **`APIConnectionError`** | – | `Faraday::ConnectionFailed` | +| Host unreachable | `Errno::EHOSTUNREACH` | **`APIConnectionError`** | – | `Faraday::ConnectionFailed` | + +Retries work as before: timeouts, refused and reset connections, and token +fetch failures are retried `max_retries` times with backoff before +`APIConnectionError` is raised. HTTP error statuses, including 429 and 5xx, are +not retried. Every request method is retried, including `post` and `patch`. + +To upgrade: + +- **If you rescue rest-client or socket errors,** rescue + `Procore::APIConnectionError` instead. + + ```ruby + # 1.x + rescue RestClient::SSLCertificateNotVerified, SocketError, Errno::EHOSTUNREACH => e + + # 2.0 + rescue Procore::APIConnectionError => e + ``` + +- **If you inspect `cause`,** check for the Faraday class from the table. + Faraday keeps the underlying error, so the original `Errno` or `SocketError` + is still reachable as `e.cause.wrapped_exception`. + + ```ruby + # 1.x + retry_later if e.cause.is_a?(Errno::ECONNREFUSED) + + # 2.0 + retry_later if e.cause.wrapped_exception.is_a?(Errno::ECONNREFUSED) + ``` + +- **If you read `Procore::Requestable::HTTP_EXCEPTIONS`,** it now lists Faraday + exception classes instead of `RestClient::Exceptions::Timeout` and + `RestClient::ServerBrokeConnection`. + +### Other changes + +- **`Response#request` is no longer a `RestClient::Request`.** It is a + `Procore::Request`. `url`, `method` (a lowercase string such as `"get"`) and + `headers` return the same values as before. `payload` now returns the request + body as a string; in 1.x, reading it raised `IOError`. +- **The gem no longer loads rest-client.** If your application uses + `RestClient` directly but only got it through this gem, add it to your + Gemfile: + + ```ruby + gem "rest-client" + ``` + ## Configuration The Procore Gem exposes a configuration with several options. @@ -115,11 +200,11 @@ delete(path, version: "", query: {}, options: {}) All paths are relative, the gem will handle expanding them. An API version can be specified in the `version:` argument, otherwise the `default_version` in the configuration will be used. -| Example | Requested URL | -| --- | --- | -| `client.get("me")` | `https://api.procore.com/rest/v1.0/me` | -| `client.get("me", version: "v1.1")` | `https://api.procore.com/rest/v1.1/me` | -| `client.get("me", version: "vapid")` | `https://api.procore.com/vapid/me` | +| Example | Requested URL | +|--------------------------------------|----------------------------------------| +| `client.get("me")` | `https://api.procore.com/rest/v1.0/me` | +| `client.get("me", version: "v1.1")` | `https://api.procore.com/rest/v1.1/me` | +| `client.get("me", version: "vapid")` | `https://api.procore.com/vapid/me` | #### Requiring `company_id` in the `options` @@ -267,7 +352,7 @@ errors generated by this gem. ```ruby begin client.get("projects") -rescue Procore:Error => e +rescue Procore::Error => e # Something went wrong. # Print the error class @@ -279,8 +364,8 @@ rescue Procore:Error => e # Print the HTTP code puts e.response.code - # Print the json error response - puts e.response.errors + # Print the parsed error response body + puts e.response.body # Print the headers puts e.response.headers @@ -319,8 +404,8 @@ like: response.pagination { - next: "projects?per_page=100&page=2", - last: "projects?per_page=100&page=5" + next: "rest/v1.0/projects?per_page=100&page=2", + last: "rest/v1.0/projects?per_page=100&page=5" } ``` @@ -328,6 +413,9 @@ The `next` value will return the second page of results - which is expected as all paginated responses start on page 1. The `last` value ends on page 5, so there are another 4 pages to consume in order to get all the possible results. +Each path carries the API version the page was requested with, so passing it +back to `client#get` requests the same version without a `version:` argument. + ### Navigating Through Paginated Results To get the next page of results, just pass the url into `client#get`. You may @@ -343,10 +431,10 @@ end puts next_page.pagination { - first: "projects?per_page=100&page=1", - next: "projects?per_page=100&page=3", - prev: "projects?per_page=100&page=1", - last: "projects?per_page=100&page=5" + first: "rest/v1.0/projects?per_page=100&page=1", + next: "rest/v1.0/projects?per_page=100&page=3", + prev: "rest/v1.0/projects?per_page=100&page=1", + last: "rest/v1.0/projects?per_page=100&page=5" } ``` @@ -360,8 +448,8 @@ first_page = client.get("projects", query: { per_page: 250 }) puts first_page.pagination { - next: "projects?per_page=250&page=2", - last: "projects?per_page=250&page=2" + next: "rest/v1.0/projects?per_page=250&page=2", + last: "rest/v1.0/projects?per_page=250&page=2" } ``` @@ -417,6 +505,58 @@ Stores contain logic for accessing, storing, and managing access tokens. The Procore API uses expiring tokens - this gem abstracts away the need to manually refresh tokens. +How the client uses its store: + +- Before every request, the client reads the token from the store. +- When the access token has expired, the client refreshes it with the refresh + token and saves the new access and refresh tokens back to the store. +- If the store has no token, the request raises `Procore::MissingTokenError` + and the user has to sign in again. + +So a store must keep the token for as long as you want the user to stay signed +in, and must be writable from everywhere the client runs, including background +jobs. + +### Choosing a Store + +| Store | Where the token lives | Survives a restart | Shared between processes and servers | Best for | +|---|---|---|---|---| +| [ActiveRecord](#activerecord-store) | A row in your database, usually the user | Yes | Yes | Rails apps with a user model, including background jobs | +| [Redis](#redis-store) | A Redis key | Yes, if Redis persistence is on | Yes | Apps without a suitable model, or tokens not tied to a user | +| [Dalli](#dalli-store) | A memcached key | No; memcached may also evict it | Yes | Only when an occasional forced sign-in is acceptable | +| [Session](#session-store) | The user's session | As long as the session lasts | Only within that user's requests | Not recommended; see the warning below | +| [File](#file-store) | A YAML file on disk | Yes | Same machine only | Command-line tools and scripts | +| [Memory](#memory-store) | A Ruby object | No | No, not even between store instances | Tests | + +Which to use: + +- **Rails app where users connect their Procore account:** use the + [ActiveRecord store](#activerecord-store) on the user model. The token lives + next to the user, survives deploys and is available to background jobs. Use + [Active Record encryption](https://guides.rubyonrails.org/active_record_encryption.html) + (`encrypts :access_token, :refresh_token`) so tokens aren't stored in plain + text. +- **Background jobs or several app servers, without a model to hold the + token** (for example, one integration-wide token): use the + [Redis store](#redis-store) with persistence turned on. +- **Command-line tool or script:** use the [File store](#file-store), so users + don't sign in for every command. +- **Tests:** use the [Memory store](#memory-store). +- **Avoid** the [Session store](#session-store): the token is unavailable to + background jobs, and depending on your session store it may be logged or kept + client-side. Use the [Dalli store](#dalli-store) only if losing a token now + and then is acceptable, because memcached forgets data on restart and evicts + it under memory pressure. + +Every built-in store keeps tokens in plain text: as JSON in Redis, memcached and +the session, as YAML in the File store, and in your table's columns for +ActiveRecord unless you encrypt them. Protect the storage the way you would a +password. + +The gem doesn't lock around token refresh. If several processes share a token +and it expires, more than one may refresh it at the same time, and the last +save wins. + Available stores: ### Session Store @@ -437,6 +577,12 @@ unencrypted. Tokens should not be stored client-side if it can be avoided. store = Procore::Auth::Stores::Session.new(session: session) ``` +- **Use it when:** prototyping a web app where every API call happens inside + the signed-in user's own requests. +- **Watch out for:** background jobs can't reach the token. With Rails' default + cookie session store, the token travels to and from the browser on every + request. + ### Redis Store Options: `redis`: Instance of Redis @@ -451,6 +597,12 @@ The key will usually be the id of the current user. store = Procore::Auth::Stores::Redis.new(redis: Redis.new, key: current_user.id) ``` +- **Use it when:** several processes or servers need the token, for example + web servers and background jobs, and there's no model to hold it. +- **Watch out for:** the token is stored under `procore-redis-{key}` with no + expiry. It only survives a Redis restart if persistence (RDB or AOF) is on, + and an eviction policy such as `allkeys-lru` can remove it. + ### Dalli Store Options: `dalli`: Instance of Dalli @@ -461,10 +613,21 @@ There's two required options, `dalli` which is an instance of a Dalli client, and `key` which is a unique key which will be used to save / retrieve an access token. The key will usually be the id of the current user. +The store saves tokens as JSON strings, so the client doesn't need Marshal. +With Dalli 5 or newer, pass `serializer: Dalli::JSONSerializer` to avoid +deserializing with Marshal and to silence Dalli's security warning. + ```ruby -store = Procore::Auth::Stores::Dalli.new(dalli: Dalli.new, key: current_user.id) +dalli = Dalli::Client.new(servers, serializer: Dalli::JSONSerializer) +store = Procore::Auth::Stores::Dalli.new(dalli: dalli, key: current_user.id) ``` +- **Use it when:** you already run memcached and users signing in again after + a cache restart is acceptable. +- **Watch out for:** memcached is a cache. A restart clears every token, and + under memory pressure it evicts keys, so users are asked to sign in again + without warning. + ### ActiveRecord Store Options: `object`: Instance of an ActiveRecord model. @@ -478,6 +641,14 @@ The following columns **must** exist on the model you pass in: store = Procore::Auth::Stores::ActiveRecord.new(object: current_user) ``` +- **Use it when:** tokens belong to a record you already have, usually the + user. This is the best default for Rails apps. +- **Watch out for:** `expires_at` holds a Unix timestamp, so make it an integer + column. Encrypt `access_token` and `refresh_token`, for example with + `encrypts :access_token, :refresh_token`. Each save runs `update`, so the + model's validations and callbacks run too, and if validation fails the new + token is silently not saved. + ### File Store Options: `path`: Path to a file to store access tokens @@ -491,6 +662,13 @@ authenticate every single command. store = Procore::Auth::Stores::File.new(path: "./tokens.yml", key: current_user.id) ``` +- **Use it when:** building a command-line tool or script that runs on one + machine. One file can hold tokens for several keys, and file locking keeps + concurrent processes on that machine from corrupting it. +- **Watch out for:** the file is plain YAML, created with your default + permissions, which usually lets every user on the machine read it. Restrict + it (`chmod 600 tokens.yml`) and keep it out of version control. + ### Memory Store Options: `key`: Unique identifier to an access token @@ -503,6 +681,62 @@ used in tests. store = Procore::Auth::Stores::Memory.new(key: current_user.id) ``` +- **Use it when:** writing tests. +- **Watch out for:** each store instance keeps its own token. A new + `Memory.new(key: ...)` with the same key starts empty, so reuse the same + instance. + +### Custom Stores + +Any object can be a store if it responds to three methods: + +- `save(token)`: persist a `Procore::Auth::Token`. +- `fetch`: return the saved `Procore::Auth::Token`, or `nil` if there is none. +- `delete`: remove the saved token. + +The client also logs the store with `to_s`, so define it to identify the store +without printing the token. + +This example keeps tokens in HashiCorp Vault with the +[vault](https://github.com/hashicorp/vault-ruby) gem: + +```ruby +class VaultStore + def initialize(key:) + @key = key + end + + def save(token) + Vault.logical.write("secret/procore/#{@key}", **token_attributes(token)) + end + + def fetch + secret = Vault.logical.read("secret/procore/#{@key}") + if secret + Procore::Auth::Token.new(**secret.data.slice(:access_token, :refresh_token, :expires_at)) + end + end + + def delete + Vault.logical.delete("secret/procore/#{@key}") + end + + def to_s + "Vault, Key: #{@key}" + end + + private + + def token_attributes(token) + { + access_token: token.access_token, + refresh_token: token.refresh_token, + expires_at: token.expires_at, + } + end +end +``` + ## Full Example ```ruby @@ -559,15 +793,26 @@ To contribute to the gem, please clone the repo and cut a new branch. In the PR, Please **do not** bump the gem version in your PR. This will be done in a follow up by the gem maintainers. +The repository has pre-commit hooks for secret scanning (Gitleaks), a +vulnerability audit of `Gemfile.lock` (bundler-audit), and a guard against +committing directly to `main`. Install them with +[prek](https://github.com/j178/prek) (or `pre-commit`): + +```sh +prek install --hook-type pre-commit +``` + ## Releasing Gem maintainer should follow these steps to release a new version: 1. Merge all PRs into the `main` branch that you want included in the release. -2. Update `version.rb` to a new semantic version, and move unreleased items in `CHANGELOG.md` into a section for that version. -3. Stage and commit changes. `git commit -a` -4. Create a tag for the new version. `git tag -a {version}` -5. Push the tagged commit `git push --follow-tags` and a new version will be published to RubyGems. +2. Create a release branch from `main`. `git switch -c release-{version}` +3. Update `version.rb` to a new semantic version, and move unreleased items in `CHANGELOG.md` into a section for that version. +4. Stage and commit changes. `git commit -a` +5. Open a PR for the release branch and merge it into `main`. Direct commits to `main` are blocked by the pre-commit hook. +6. Once the Test workflow passes on `main`, the Release workflow sees the change to `version.rb` and publishes the new version to RubyGems. +7. Tag the merge commit for the new version. `git switch main && git pull && git tag -a {version} && git push --tags` ### Tests diff --git a/Rakefile b/Rakefile index 70cbfa7..6b2c1b7 100644 --- a/Rakefile +++ b/Rakefile @@ -1,14 +1,24 @@ +require "bundler/audit/task" require "bundler/gem_tasks" -require "rake/testtask" +require "minitest/test_task" require "rubocop/rake_task" RuboCop::RakeTask.new(:rubocop) do |t| t.options = ["--display-cop-names"] end -Rake::TestTask.new do |t| - t.libs << "test" - t.test_files = FileList["test/**/*_test.rb"] +Bundler::Audit::Task.new + +namespace :security do + desc "Update the vulnerability database and audit Gemfile.lock" + task :audit do + Rake::Task["bundle:audit:update"].invoke + Rake::Task["bundle:audit"].invoke + end +end + +Minitest::TestTask.create do |t| + t.test_globs = ["test/**/*_test.rb"] t.warning = false end diff --git a/SECURITY.md b/SECURITY.md index c009fd7..2f97c00 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -6,11 +6,10 @@ Ruby versions that are currently being supported with security updates. | Version | Supported | | ------- | ------------------ | -| <=2.7 | :x: | -| 3.0 | :white_check_mark: | -| 3.1 | :white_check_mark: | -| 3.2 | :white_check_mark: | +| <=3.2 | :x: | | 3.3 | :white_check_mark: | +| 3.4 | :white_check_mark: | +| 4.0 | :white_check_mark: | ## Reporting a Vulnerability diff --git a/docs/adr/0001-replace-rest-client-with-faraday.md b/docs/adr/0001-replace-rest-client-with-faraday.md new file mode 100644 index 0000000..9947641 --- /dev/null +++ b/docs/adr/0001-replace-rest-client-with-faraday.md @@ -0,0 +1,220 @@ +# 1. Replace rest-client with Faraday + +Date: 2026-10-14 + +## Status + +Accepted + +## Context + +`Procore::Requestable` sends every API request through `rest-client`. Its last +release, 2.1.0, was in 2019, and the project is effectively unmaintained. It +holds back the dependency tree: `http-accept` is pinned below 2.0 because of +it. + +rest-client is used almost entirely in `lib/procore/requestable.rb`, but its +types show through the public API in a few places: + +- `Response#headers` is rest-client's header hash, with keys converted to + symbols (`:link`, `:content_type`). The README tells users to inspect + `e.response.headers`, and pagination reads `headers[:link]`. +- `Response#request` returns a `RestClient::Request`. It is public but + undocumented. +- File uploads work by passing a `File` in `body:`. rest-client detects it and + sends `multipart/form-data`. +- The cause attached to `APIConnectionError` is a rest-client exception. + +Two existing bugs come from the rest-client coupling: + +- `AccessTokenCredentials#revoke` rescues `RestClient::ExceptionWithResponse`, + but oauth2 makes its calls through Faraday, so the rescue never runs. It also + refers to an unbound `e`. +- The request logs look up `headers["x-request-id"]` with a string key, but + rest-client stores headers under symbol keys (`:x_request_id`), so the + logged request ID is likely always `nil`. + +Options considered: + +- **Faraday.** Actively maintained, and already installed because oauth2 + depends on it. Each client gets its own connection object, and the HTTP + adapter can be swapped. Multipart and retries come from the + `faraday-multipart` and `faraday-retry` gems, both from the same maintainers. +- **HTTParty.** Maintained, with multipart built in. It would be a new gem with + its own dependencies (`multi_xml`, `csv`, `mini_mime`), and it is built around + configuring a class, which suits applications more than libraries. +- **Net::HTTP.** Ships with Ruby, so no new gems. Multipart encoding, retries and + nested query encoding would all be code we write and maintain. + +## Decision + +Replace rest-client with Faraday, using `faraday-multipart` for uploads and +`faraday-retry` for retries. Keep the public API of `Procore::Client` +unchanged: + +- **Request methods:** `get`, `post`, `put`, `patch`, `delete` and `sync` keep + their signatures and return types. +- **Errors:** responses keep mapping to the existing `Procore::*` errors. + Connection failures that remain after retries still raise + `Procore::APIConnectionError`. +- **Configuration:** `Procore.configuration.timeout` maps to Faraday's + `request.timeout`. `max_retries` maps to the retry middleware's `max`. +- **Headers:** `Response#headers` keeps its symbol-keyed shape (`:link`, + `:x_request_id`). The gem converts Faraday's headers into that shape inside + `Procore::Response`. +- **Request:** `Response#request` returns a small Procore-owned struct instead + of a `RestClient::Request`, following the stand-in structs `OAuthError` + already uses. It answers the same calls with the same values rest-client + gives today, so code reading them keeps working: + - `url`: the full URL including the query string + (`http://…/rest/v1.0/projects?per_page=5`). + - `method`: the HTTP method as a lowercase string (`"get"`), not a symbol. + - `headers`: the request headers as sent, keyed by header name + (`"Authorization"`). + - `payload`: the request body as a string. With rest-client this is a + `RestClient::Payload` whose stream is already closed, so reading it raises + `IOError`; no caller can depend on it, and it is not a compatibility + promise. + + The gem does not define any `RestClient::*` classes to imitate rest-client. + An app that also loads the real rest-client would get clashing definitions. +- **Uploads:** the gem detects `File`/`IO` values in `body:` and wraps them in + `Faraday::Multipart::FilePart`, so upload calls do not change. Each file + part's content type is guessed from its extension with `mime-types`, as + rest-client did, which keeps `mime-types` as a direct dependency. +- **Connection failures:** DNS failures, unreachable hosts and TLS errors raise + `Procore::APIConnectionError`. Today they escape as `SocketError`, + `Errno::EHOSTUNREACH` and `RestClient::SSLCertificateNotVerified`. +- **Redirects:** GET requests keep following redirects, as rest-client does. + Faraday does not follow them by default, so this needs either the + `faraday-follow_redirects` gem or a small redirect loop in `Requestable`. +- **`HTTP_EXCEPTIONS`:** this public constant lists only Ruby and Faraday + exception classes, with no rest-client ones. +- **Loading the gem:** `require "procore"` no longer defines `RestClient` in the + caller's process. + +`test/procore/rest_client_leakage_test.rb` holds tests for these points. Nine of +them fail until the migration is done; that file is the migration's acceptance +check. + +The retry middleware is configured to match the current retry behaviour, not +Faraday's defaults: + +- **Methods:** retry all methods, including `post` and `patch`. Faraday retries + only idempotent methods by default. The current code retries every method, + and `post`/`patch` can carry idempotency tokens. +- **Exceptions:** retry on timeouts and connection failures + (`Faraday::TimeoutError`, `Faraday::ConnectionFailed`, `Errno::ECONNREFUSED`, + `Errno::ECONNRESET`). +- **Statuses:** do not retry on HTTP status codes. The current code does not + retry 429 or 5xx responses, and changing that is a separate decision. +- **Backoff:** `interval: 1.5` and `backoff_factor: 1.5`, which gives the + current waits of 1.5 s, 2.25 s, 3.375 s and so on. + +`Procore::OAuthError` is also in today's retry list. It is raised while the +access token is fetched, before Faraday sends the request, so the retry +middleware cannot see it. Token fetching keeps its own retry inside +`Requestable`. + +## Consequences + +- One unmaintained runtime dependency (`rest-client`, and with it + `http-accept`, `http-cookie` and `netrc`) is replaced by `faraday-multipart` + and `faraday-retry`. `faraday` and `mime-types` are now declared directly; + before, they arrived through oauth2 and rest-client. +- `Response#request` changes type. Code that calls rest-client-specific methods + on it breaks. This ships in 2.0.0 alongside the Ruby 3.3 minimum and is listed + in the CHANGELOG. +- `APIConnectionError#cause` becomes a Faraday exception instead of a + rest-client one. +- The `revoke` rescue and the request-ID logging are fixed as part of the + migration. +- Before the switch, tests pin the behaviour users depend on: the shape of + `Response#headers`, nested query encoding (`filters[foo]=1`, + `ids[]=1&ids[]=2`), multipart uploads, and the retry count and backoff. + +## Upgrade guide + +This section is written for users of the gem and is meant to be copied into the +README or the 2.0.0 release notes once the migration ships. + +### Upgrading to 2.0: rest-client replaced by Faraday + +The gem now sends requests with [Faraday](https://lostisland.github.io/faraday/) +instead of rest-client. The request methods (`get`, `post`, `put`, `patch`, +`delete`, `sync`), their arguments, `Procore::Response` and the configuration +options are unchanged. What changes is mostly which exceptions you see. + +#### Exceptions + +Every failure now raises a `Procore::Error` subclass, so +`rescue Procore::Error` catches anything the client raises for a failed request. + +| Failure | 1.x raises | 2.0 raises | `cause` in 1.x | `cause` in 2.0 | +|-----------------------------------|-----------------------------------------|--------------------------|-------------------------------------------------------|-----------------------------| +| HTTP 401 | `AuthorizationError` | unchanged | – | – | +| HTTP 403 | `ForbiddenError` | unchanged | – | – | +| HTTP 404 | `NotFoundError` | unchanged | – | – | +| HTTP 400, 422 | `InvalidRequestError` | unchanged | – | – | +| HTTP 429 | `RateLimitError` | unchanged | – | – | +| Any other non-2xx status | `ServerError` | unchanged | – | – | +| Connect timeout | `APIConnectionError` | unchanged | `RestClient::Exceptions::OpenTimeout` | `Faraday::ConnectionFailed` | +| Read timeout | `APIConnectionError` | unchanged | `RestClient::Exceptions::ReadTimeout` | `Faraday::TimeoutError` | +| Connection refused | `APIConnectionError` | unchanged | `Errno::ECONNREFUSED` | `Faraday::ConnectionFailed` | +| Connection reset | `APIConnectionError` | unchanged | `Errno::ECONNRESET` | `Faraday::ConnectionFailed` | +| Server closed the connection | `APIConnectionError` | unchanged | `RestClient::ServerBrokeConnection` | `Faraday::ConnectionFailed` | +| Access token could not be fetched | `APIConnectionError` | unchanged | `Procore::OAuthError` | unchanged | +| TLS certificate error | `RestClient::SSLCertificateNotVerified` | **`APIConnectionError`** | – | `Faraday::SSLError` | +| DNS lookup failed | `SocketError` | **`APIConnectionError`** | – | `Faraday::ConnectionFailed` | +| Host unreachable | `Errno::EHOSTUNREACH` | **`APIConnectionError`** | – | `Faraday::ConnectionFailed` | + +The retry behaviour is unchanged: timeouts, refused and reset connections, and +token fetch failures are retried `max_retries` times with backoff before +`APIConnectionError` is raised. HTTP error statuses, including 429 and 5xx, are +not retried. Every request method is retried, including `post` and `patch`. + +**What to change** + +- **Rescuing rest-client or socket errors:** rescue `Procore::APIConnectionError` + instead. + + ```ruby + # 1.x + rescue RestClient::SSLCertificateNotVerified, SocketError, Errno::EHOSTUNREACH => e + + # 2.0 + rescue Procore::APIConnectionError => e + ``` + +- **Inspecting `cause`:** code that checks for a specific rest-client or `Errno` + class needs the Faraday class from the table. Faraday keeps the underlying + error, so the original `Errno` is still reachable as + `e.cause.wrapped_exception`. + + ```ruby + # 1.x + retry_later if e.cause.is_a?(Errno::ECONNREFUSED) + + # 2.0 + retry_later if e.cause.wrapped_exception.is_a?(Errno::ECONNREFUSED) + ``` + +- **`Procore::Requestable::HTTP_EXCEPTIONS`:** if you read this constant, it now + lists Faraday exception classes instead of `RestClient::Exceptions::Timeout` + and `RestClient::ServerBrokeConnection`. + +#### Other changes + +- **`Response#request` is no longer a `RestClient::Request`.** It is a small + Procore object. `url`, `method` (a lowercase string such as `"get"`) and + `headers` return the same values as before. `payload` now returns the request + body as a string; in 1.x, reading it raised `IOError`. +- **Integer header values are sent as strings.** `company_id` and + `idempotency_token` may still be passed as Integers. +- **The gem no longer loads rest-client.** If your application uses + `RestClient` directly but only got it through this gem, add it to your + Gemfile: + + ```ruby + gem "rest-client" + ``` diff --git a/docs/todos.md b/docs/todos.md new file mode 100644 index 0000000..5ec9f39 --- /dev/null +++ b/docs/todos.md @@ -0,0 +1,233 @@ +# TODOs + +Run each command from the repository root. The first three bugs are already +fixed in the working tree (the first two by the Faraday migration) and only +need filing for the record. The fourth is a security issue: report it +privately, not as a public issue. + +The commands use bash heredocs, which fish does not support. Start `bash` +first, then paste a command. Each command adds the `bug` label; drop +`--label bug` if the repository has no such label. + +## - [ ] File: `revoke` raises `OAuth2::Error` instead of `Procore::OAuthError` + +~~~sh +gh issue create --label bug --title 'Token revoke failure raises OAuth2::Error instead of Procore::OAuthError' --body-file - <<'EOF' +## Summary + +When the token revoke request fails, `Procore::Client#revoke` raises a raw +`OAuth2::Error` instead of `Procore::OAuthError`, so callers who rescue the +gem's documented errors don't catch it. + +## Steps to reproduce + +```ruby +credentials = Procore::Auth::AccessTokenCredentials.new( + client_id: "id", + client_secret: "secret", + host: "https://login.example.com", +) +# With POST /oauth/revoke returning HTTP 500: +credentials.revoke(token: token) +``` + +## Expected + +`Procore::OAuthError`, with the failed response attached. + +## Actual + +`OAuth2::Error` (a `StandardError`). + +## Cause + +`lib/procore/auth/access_token_credentials.rb`: + +```ruby +def revoke(token:) + # ... + client.request(:post, "/oauth/revoke", body: request) +rescue RestClient::ExceptionWithResponse + raise OAuthError.new(e.description, response: e.response) +end +``` + +- The request is made by the oauth2 gem, which uses Faraday and raises + `OAuth2::Error`, never `RestClient::ExceptionWithResponse`, so the rescue + never matches. +- If it did match, it would raise `NameError`, because the exception isn't + bound (`=> e` is missing). +- `Procore::Client#revoke` only rescues `RuntimeError`, which `OAuth2::Error` + is not, so the error reaches the caller unchanged. + +## Suggested fix + +Rescue `OAuth2::Error => e` and raise `OAuthError.new(e.description, response: e.response)`, +matching `#refresh` in the same class. Add a test for a failed revoke. +EOF +~~~ + +## - [ ] File: request ID is never logged + +~~~sh +gh issue create --label bug --title 'Request logs never include the X-Request-Id' --body-file - <<'EOF' +## Summary + +The "API Request Finished" and "API Request Failed" log lines are meant to +include the response's `X-Request-Id`, but the value is always empty, so logs +can't be matched to Procore's server-side request IDs. + +## Steps to reproduce + +1. Set `Procore.configuration.logger` to a `Logger`. +2. Make any request whose response includes `X-Request-Id: req-123`. + +## Expected + +The log line contains `request_id: req-123`. + +## Actual + +`request_id` is empty. `response.headers[:x_request_id]` returns `"req-123"`, +so the header is received; only the log lookup is wrong. + +## Cause + +`lib/procore/requestable.rb`, in `with_response_handling`: + +```ruby +request_id: result.headers["x-request-id"], +``` + +rest-client stores response headers under underscored symbol keys +(`:x_request_id`), so looking up the string `"x-request-id"` always returns +`nil`. + +## Suggested fix + +Read `response.headers[:x_request_id]` from the `Procore::Response`, which +keeps the same key once the HTTP client is replaced. Add a test that captures +the log output and checks for the request ID. +EOF +~~~ + +## - [ ] File: `delete` ignores `company_id` + +~~~sh +gh issue create --label bug --title 'delete ignores options:, so company_id is never sent' --body-file - <<'EOF' +## Summary + +`Procore::Client#delete` ignores its `options:` argument, so +`options: { company_id: ... }` never produces the `Procore-Company-Id` header. +Every other request method sends it. On multi-company (MPZ) accounts, DELETE +requests that need the header can fail or hit the wrong company. + +## Steps to reproduce + +```ruby +client.delete("projects/1", options: { company_id: 42 }) +``` + +## Expected + +The request includes `Procore-Company-Id: 42`. + +## Actual + +The header is missing. + +## Cause + +`lib/procore/requestable.rb`, in `#delete`: + +```ruby +headers: headers.merge(params: query), +``` + +It calls `headers` without `options`, so the company and idempotency headers +are never added. The other methods call `headers(options)`. + +## Fix + +Use `headers(options)`, as the other methods do. Covered by +`test_delete_with_company_id` in `test/procore/requestable_test.rb`. +EOF +~~~ + +## - [ ] Report privately: DELETE requests log the access token + +`SECURITY.md` asks for vulnerabilities to be reported privately. Don't open a +public issue for this one. Use **Report a vulnerability** at +https://github.com/procore-oss/ruby-sdk/security and paste the text below. +The fix is already in the working tree. + +**Title:** Access token written to logs on DELETE requests + +**Weakness:** CWE-532: Insertion of Sensitive Information into Log File + +**Affected versions:** 1.3.1 and earlier (the first affected version has not +been checked) + +**Description:** + +> When a logger is configured (`Procore.configuration.logger`), every +> `Procore::Client#delete` call writes the full request headers to the log, +> including `Authorization: Bearer `. Anyone who can read the +> application's logs, or any log aggregation service they're shipped to, can +> obtain a valid Procore access token and call the API as that user until the +> token expires. No other request method logs headers. +> +> **Steps to reproduce** +> +> ```ruby +> Procore.configure { |config| config.logger = Logger.new($stdout) } +> client.delete("projects/1") +> # The "API Request Initiated" line includes +> # "Authorization" => "Bearer " +> ``` +> +> **Cause:** `lib/procore/requestable.rb`, `#delete`, passes +> `headers: headers(options)` to `Util.log_info`. +> +> **Fix:** remove `headers:` from that log call. A test that captures the log +> output and checks the token is absent +> (`test_delete_does_not_log_access_token`) covers it. +> +> **Advice for users:** after upgrading, revoke or rotate access tokens that may +> have been logged, and purge DELETE request lines from existing logs. + +## Bugs + +Found in the 2026-10-26 review against `reference/combined_OAS.json`. Fix +test-first; each item names the test that should fail before the fix. + +- [x] **Gem breaks without a full ActiveSupport load.** `lib/procore.rb` only + requires `active_support/hash_with_indifferent_access`, but the code uses + three core extensions it never requires. Outside Rails: the Session, Redis + and Dalli stores serialize a token as `"#"` + (`Object#to_json`), so every request raises `MissingTokenError`; `sync` + raises `NoMethodError` on `deep_merge`; `OAuthError` with a response raises + `NoMethodError` on `presence`. Require the extensions where they are used + and tighten the gemspec's `activesupport > 2.4` constraint. + Test: `test/procore/standalone_load_test.rb` (subprocess, like + `test_requiring_procore_does_not_load_rest_client`). +- [x] **Empty response bodies parse to the String `"{}"`.** + `lib/procore/response.rb` defaults the raw body to `"{}".to_json`, which is + the JSON string literal `"\"{}\""`. The spec has 235 operations returning + 204 No Content; `response.body` on those is a String, not a Hash. + Test: `test/procore/response_test.rb`, empty and nil bodies give `{}`. +- [x] **Pagination drops the API version.** `Response#parse_pagination` + strips the `rest/vX.Y` or `vapid` prefix, so `client.get(pagination[:next])` + uses `default_version`. About half the spec's paths are v1.1, v2.0 or v2.1, + and following their links silently hits the wrong version. Decide the + public shape first (keep the version in the URL, or accept absolute URLs). + Test: `test/procore/requestable_test.rb`, following a v1.1 next link + requests `/rest/v1.1/...`. +- [x] **README documents methods that do not exist.** `e.response.errors` + raises `NoMethodError` (`Response` has `body`, not `errors`), and + `rescue Procore:Error` has a single colon. No test; documentation only. +- [x] **`Accepts` is not an HTTP header.** `Requestable#headers` sends + `Accepts: application/json`; the header is `Accept`. Harmless today because + Procore defaults to JSON, but it is wrong and the spec has an endpoint that + requires `Accept: text/csv`. + Test: `test/procore/requestable_test.rb` header expectations use `Accept`. diff --git a/lib/procore.rb b/lib/procore.rb index 91b1f65..0b02ef7 100644 --- a/lib/procore.rb +++ b/lib/procore.rb @@ -17,6 +17,7 @@ module Procore require "procore/configuration" require "procore/defaults" require "procore/errors" +require "procore/request" require "procore/response" require "procore/util" require "procore/version" diff --git a/lib/procore/auth/access_token_credentials.rb b/lib/procore/auth/access_token_credentials.rb index fc53ab6..eca3051 100644 --- a/lib/procore/auth/access_token_credentials.rb +++ b/lib/procore/auth/access_token_credentials.rb @@ -39,7 +39,7 @@ def revoke(token:) token: token.access_token, } client.request(:post, "/oauth/revoke", body: request) - rescue RestClient::ExceptionWithResponse + rescue OAuth2::Error => e raise OAuthError.new(e.description, response: e.response) end diff --git a/lib/procore/auth/stores/file.rb b/lib/procore/auth/stores/file.rb index 244f579..9628ecb 100644 --- a/lib/procore/auth/stores/file.rb +++ b/lib/procore/auth/stores/file.rb @@ -1,14 +1,25 @@ require "yaml/store" +require "procore/auth/token" module Procore module Auth module Stores class File + # YAML::Store only permits Symbol when loading on Ruby 3.4+, which + # rejects the serialized Token. Allow the classes a token file holds. + class TokenStore < YAML::Store + PERMITTED_CLASSES = [Symbol, Time, Procore::Auth::Token].freeze + + def load(content) + YAML.safe_load(content, permitted_classes: PERMITTED_CLASSES) || {} + end + end + attr_reader :key, :path def initialize(key:, path:) @key = key @path = path - @store = YAML::Store.new(path) + @store = TokenStore.new(path) end def save(token) diff --git a/lib/procore/auth/token.rb b/lib/procore/auth/token.rb index 67da303..80baf33 100644 --- a/lib/procore/auth/token.rb +++ b/lib/procore/auth/token.rb @@ -1,3 +1,5 @@ +require "json" + module Procore module Auth class Token @@ -8,6 +10,21 @@ def initialize(access_token:, refresh_token:, expires_at:) @expires_at = expires_at end + # The shape the Session, Redis and Dalli stores persist and read back. + # + # @return [Hash{Symbol => Object}] + def to_h + { + access_token: access_token, + refresh_token: refresh_token, + expires_at: expires_at, + } + end + + def to_json(*args) + to_h.to_json(*args) + end + def invalid? access_token.nil? end diff --git a/lib/procore/errors.rb b/lib/procore/errors.rb index 7941964..1f8e888 100644 --- a/lib/procore/errors.rb +++ b/lib/procore/errors.rb @@ -1,4 +1,4 @@ -require 'ostruct' +require "active_support/core_ext/object/blank" module Procore # Generic / catch all error class. All other errors generated by the gem @@ -39,15 +39,20 @@ def initialize(message, response: nil) # Raised whenever there is a problem with OAuth. Possible causes: required # credentials are missing or an access token failed to refresh. class OAuthError < Error + # Minimal stand-ins exposing the parts of a Procore::Response that callers + # read from an error, built from the OAuth2 library's response. + DuckRequest = Struct.new(:options, :path, keyword_init: true) + DuckResponse = Struct.new(:code, :body, :headers, :request, keyword_init: true) + def initialize(message, response: nil) @message = message duck_response = if response - OpenStruct.new( + DuckResponse.new( code: response.status, body: response.parsed.presence || response.body, headers: response.headers, - request: OpenStruct.new( + request: DuckRequest.new( options: {}, path: nil, ), diff --git a/lib/procore/request.rb b/lib/procore/request.rb new file mode 100644 index 0000000..acee5d6 --- /dev/null +++ b/lib/procore/request.rb @@ -0,0 +1,20 @@ +module Procore + # The request that produced a Response. Exposes the same values callers read + # from RestClient::Request before the gem moved to Faraday. + # + # @example + # response = client.get("projects", query: { per_page: 5 }) + # response.request.url #=> "https://api.procore.com/rest/v1.0/projects?per_page=5" + # response.request.method #=> "get" + # + # @!attribute [r] method + # @return [String] HTTP method, lowercase. + # @!attribute [r] url + # @return [String] Full URL, including the query string. + # @!attribute [r] headers + # @return [Hash] Request headers as sent. + # @!attribute [r] payload + # @return [String, nil] Request body, or nil when there was none or it was + # a multipart upload. + Request = Struct.new(:method, :url, :headers, :payload, keyword_init: true) +end diff --git a/lib/procore/requestable.rb b/lib/procore/requestable.rb index fe0e025..3dd9b03 100644 --- a/lib/procore/requestable.rb +++ b/lib/procore/requestable.rb @@ -1,5 +1,10 @@ -require "rest-client" +require "active_support/core_ext/hash/deep_merge" +require "faraday" +require "faraday/multipart" +require "faraday/retry" +require "mime/types" require "procore/errors" +require "procore/request" module Procore # Module which defines HTTP verbs GET, POST, PUT, PATCH and DELETE. Is @@ -11,13 +16,41 @@ module Procore # @example Using #post: # client.post("projects", name: "New Project") module Requestable + # Failures raised as APIConnectionError once retries are used up. HTTP_EXCEPTIONS = [ + Faraday::ConnectionFailed, + Faraday::SSLError, + Faraday::TimeoutError, + Procore::OAuthError, + ].freeze + + # Connection failures worth retrying: the connection timed out, or the + # server refused, reset or closed it. DNS, unreachable-host and TLS + # failures are not retried. faraday-net_http raises connect timeouts + # (Net::OpenTimeout) as ConnectionFailed, hence Timeout::Error here. + RETRIABLE_CONNECTION_ERRORS = [ Errno::ECONNREFUSED, Errno::ECONNRESET, - Procore::OAuthError, - RestClient::Exceptions::Timeout, - RestClient::ServerBrokeConnection, + EOFError, + Timeout::Error, ].freeze + + RETRY_IF = lambda do |_env, exception| + if exception.is_a?(Faraday::ConnectionFailed) + RETRIABLE_CONNECTION_ERRORS.any? do |error| + exception.wrapped_exception.is_a?(error) + end + else + true + end + end + private_constant :RETRIABLE_CONNECTION_ERRORS, :RETRY_IF + + # GET follows redirects, as it did with rest-client. + REDIRECT_STATUSES = [301, 302, 303, 307, 308].freeze + MAX_REDIRECTS = 10 + private_constant :REDIRECT_STATUSES, :MAX_REDIRECTS + # @param path [String] URL path # @param version [String] API version # @param query [Hash] Query options to pass along with the request @@ -38,12 +71,7 @@ def get(path, version: nil, query: {}, options: {}) ) with_response_handling do - RestClient::Request.execute( - method: :get, - url: full_path, - headers: headers(options).merge(params: query), - timeout: Procore.configuration.timeout, - ) + get_following_redirects(full_path, query, headers(options)) end end @@ -72,13 +100,7 @@ def post(path, version: nil, body: {}, options: {}) ) with_response_handling(request_body: body) do - RestClient::Request.execute( - method: :post, - url: full_path, - payload: payload(body), - headers: headers(options), - timeout: Procore.configuration.timeout, - ) + perform(:post, full_path, body: payload(body), headers: headers(options)) end end @@ -103,13 +125,7 @@ def put(path, version: nil, body: {}, options: {}) ) with_response_handling(request_body: body) do - RestClient::Request.execute( - method: :put, - url: full_path, - payload: payload(body), - headers: headers(options), - timeout: Procore.configuration.timeout, - ) + perform(:put, full_path, body: payload(body), headers: headers(options)) end end @@ -138,13 +154,7 @@ def patch(path, version: nil, body: {}, options: {}) ) with_response_handling(request_body: body) do - RestClient::Request.execute( - method: :patch, - url: full_path, - payload: payload(body), - headers: headers(options), - timeout: Procore.configuration.timeout, - ) + perform(:patch, full_path, body: payload(body), headers: headers(options)) end end @@ -193,12 +203,11 @@ def sync(path, version: nil, body: {}, options: {}) responses = groups.map do |group| batched_body = body.merge(updates: group) with_response_handling(request_body: batched_body) do - RestClient::Request.execute( - method: :patch, - url: full_path, - payload: payload(batched_body), + perform( + :patch, + full_path, + body: payload(batched_body), headers: headers(options), - timeout: Procore.configuration.timeout, ) end end @@ -230,17 +239,11 @@ def delete(path, version: nil, query: {}, options: {}) "API Request Initiated", path: full_path, method: "DELETE", - headers: headers(options), query: query.to_s, ) with_response_handling do - RestClient::Request.execute( - method: :delete, - url: full_path, - headers: headers.merge(params: query), - timeout: Procore.configuration.timeout, - ) + perform(:delete, full_path, params: query, headers: headers(options)) end end @@ -252,52 +255,43 @@ def with_response_handling(request_body: nil) begin result = yield - rescue *HTTP_EXCEPTIONS => e + rescue Procore::OAuthError + # The access token is fetched before Faraday sends the request, so the + # retry middleware never sees this failure. Retry it here instead. if retries < Procore.configuration.max_retries retries += 1 sleep 1.5**retries retry else - raise APIConnectionError.new( - "Cannot connect to the Procore API. Double check your timeout " \ - "settings to ensure requests are not being cancelled before they " \ - "can complete. Try setting the timeout and max_retries to larger " \ - "values.", - ), e + raise_connection_error end - rescue RestClient::ExceptionWithResponse => e - result = e.response + rescue *HTTP_EXCEPTIONS + raise_connection_error end - response = Procore::Response.new( - body: result.body, - headers: result.headers, - code: result.code, - request: result.request, - request_body: request_body, - ) + response = build_response(result, request_body) - case result.code + case response.code when 200..299 Util.log_info( "API Request Finished ", - path: result.request.url, - status: result.code.to_s, + path: response.request.url, + status: response.code.to_s, duration: "#{((Time.now - request_start_time) * 1000).round(0)}ms", - request_id: result.headers["x-request-id"], + request_id: response.headers[:x_request_id], ) else Util.log_error( "API Request Failed", - path: result.request.url, - status: result.code.to_s, + path: response.request.url, + status: response.code.to_s, duration: "#{((Time.now - request_start_time) * 1000).round(0)}ms", - request_id: result.headers["x-request-id"], + request_id: response.headers[:x_request_id], retries: retries, ) end - case result.code + case response.code when 200..299 response when 401 @@ -340,7 +334,7 @@ def with_response_handling(request_body: nil) def headers(options = {}) { - "Accepts" => "application/json", + "Accept" => "application/json", "Authorization" => "Bearer #{access_token}", "Content-Type" => "application/json", "Procore-Sdk-Version" => Procore::VERSION, @@ -348,30 +342,145 @@ def headers(options = {}) "User-Agent" => Procore.configuration.user_agent, }.tap do |headers| if options[:idempotency_token] - headers["Idempotency-Token"] = options[:idempotency_token] + headers["Idempotency-Token"] = options[:idempotency_token].to_s end if options[:company_id] - headers["procore-company-id"] = options[:company_id] + headers["procore-company-id"] = options[:company_id].to_s end end end + # Raised from inside a rescue, so the original error becomes the cause. + def raise_connection_error + raise APIConnectionError.new( + "Cannot connect to the Procore API. Double check your timeout " \ + "settings to ensure requests are not being cancelled before they " \ + "can complete. Try setting the timeout and max_retries to larger " \ + "values.", + ) + end + + def connection + Faraday.new do |conn| + conn.options.timeout = Procore.configuration.timeout + # Retry runs before multipart so each attempt re-encodes the upload. + conn.request :retry, + max: Procore.configuration.max_retries, + interval: 1.5, + backoff_factor: 1.5, + exceptions: [Faraday::TimeoutError, Faraday::ConnectionFailed], + methods: [], + retry_if: RETRY_IF + conn.request :multipart + conn.adapter Faraday.default_adapter + end + end + + def perform(method, url, headers:, params: nil, body: nil) + # Multipart bodies arrive as a Hash; let the middleware set the + # multipart Content-Type and boundary. + if body.is_a?(Hash) + headers = headers.except("Content-Type") + end + + connection.run_request(method, url, body, headers) do |request| + request.params.update(params) if params + end + end + + def get_following_redirects(url, query, headers) + response = perform(:get, url, params: query, headers: headers) + MAX_REDIRECTS.times do + location = response.headers["location"] + break unless REDIRECT_STATUSES.include?(response.status) && location + + response = perform( + :get, + URI.join(response.env.url.to_s, location).to_s, + headers: headers, + ) + end + response + end + + def build_response(result, request_body) + env = result.env + + Procore::Response.new( + body: result.body, + headers: response_headers(result.headers), + code: result.status, + request: Procore::Request.new( + method: env.method.to_s, + url: env.url.to_s, + headers: env.request_headers.to_h, + payload: (env.request_body if env.request_body.is_a?(String)), + ), + request_body: request_body, + ) + end + + # Keys headers the way rest-client did (:x_request_id, :content_type), + # which callers and Response#pagination rely on. + def response_headers(headers) + headers.to_h.to_h do |name, value| + [name.to_s.downcase.tr("-", "_").to_sym, value] + end + end + def payload(body) if multipart?(body) - body + multipart_body(body) else body.to_json end end def multipart?(body) - RestClient::Payload::has_file?(body) + case body + when Hash then body.values.any? { |value| multipart?(value) } + when Array then body.any? { |value| multipart?(value) } + else file?(body) + end + end + + def multipart_body(body) + case body + when Hash then body.transform_values { |value| multipart_body(value) } + when Array then body.map { |value| multipart_body(value) } + else + if file?(body) + Faraday::Multipart::FilePart.new( + body, + content_type(body.path), + File.basename(body.path), + ) + else + body + end + end + end + + # Guesses from the file extension, as rest-client did. + def content_type(path) + MIME::Types.type_for(path).first&.content_type || + "application/octet-stream" end + def file?(value) + value.respond_to?(:path) && value.respond_to?(:read) + end + + # A path that already names its version, as Response#pagination returns. + VERSIONED_PATH = %r{\A/?(?:vapid|rest/v\d+\.\d+)(?:/|\z)} + private_constant :VERSIONED_PATH + def full_path(path, version) version ||= options[:default_version] - if version == "vapid" + if VERSIONED_PATH.match?(path) + File.join(base_api_path, path) + elsif version == "vapid" File.join(base_api_path, "vapid", path) elsif /\Av\d+\.\d+\z/.match?(version) File.join(base_api_path, "rest", version, path) diff --git a/lib/procore/response.rb b/lib/procore/response.rb index d13c008..9c612c4 100644 --- a/lib/procore/response.rb +++ b/lib/procore/response.rb @@ -27,12 +27,13 @@ module Procore # first_page.pagination[:first] # #=> nil # first_page.pagination[:next] - # #=> "projects?per_page=20&page=2" + # #=> "rest/v1.0/projects?per_page=20&page=2" # - # # Any other page will have all keys + # # Any other page will have all keys. The path carries the API version, + # # so #get requests the same version without a version: argument. # next_page = client.get(first_page.pagination[:next]) # next_page.pagination[:first] - # #=> "projects?per_page=20&page=1" + # #=> "rest/v1.0/projects?per_page=20&page=1" # # # The last page will only have URLs for :first & :next # last_page = client.get(first_page.pagination[:last]) @@ -53,7 +54,7 @@ def initialize(body:, headers:, code:, request:, request_body:) @pagination = parse_pagination @request = request @request_body = request_body - @raw_body = !body.to_s.empty? ? body : "{}".to_json + @raw_body = body.to_s.empty? ? "{}" : body end # @return [Array, Hash] Ruby representation of JSON response. Hashes are @@ -70,9 +71,14 @@ def parse_body JSON.parse(raw_body, object_class: HashWithIndifferentAccess) end + # Keeps the version segment ("rest/v1.1/..." or "vapid/...") so the + # returned path requests the same API version when passed back to #get. + LINK = %r{((?:vapid|rest/v\d+\.\d+)/.*?)>; rel="(\w+)"} + private_constant :LINK + def parse_pagination headers[:link].to_s.split(", ").map(&:strip).reduce({}) do |links, link| - url, name = link.match(/(?:vapid|rest\/.*?)\/(.*?)>; rel="(\w+)"/).captures + url, name = link.match(LINK).captures links.merge!(name.to_sym => url) end end diff --git a/lib/procore/util.rb b/lib/procore/util.rb index 72751c1..833a6b0 100644 --- a/lib/procore/util.rb +++ b/lib/procore/util.rb @@ -9,8 +9,10 @@ def self.log_info(message, meta = {}) end.join(" ") Procore.configuration.logger.info( - "#{colorize('Procore', :yellow)} <<- " \ - "#{colorize(message.ljust(22), :cyan)} <<- #{meta_string}", + redact( + "#{colorize('Procore', :yellow)} <<- " \ + "#{colorize(message.ljust(22), :cyan)} <<- #{meta_string}", + ), ) end @@ -22,11 +24,24 @@ def self.log_error(message, meta = {}) end.join(" ") Procore.configuration.logger.info( - "#{colorize('Procore', :red)} <<- " \ - "#{colorize(message.ljust(22), :red)} <<- #{meta_string}", + redact( + "#{colorize('Procore', :red)} <<- " \ + "#{colorize(message.ljust(22), :red)} <<- #{meta_string}", + ), ) end + # Replaces bearer tokens with "###" so access tokens never reach the logs, + # whichever caller passes them in. + # + # @example + # redact('"Authorization" => "Bearer abc123"') + # #=> '"Authorization" => "Bearer ###"' + def self.redact(text) + text.gsub(BEARER_TOKEN, '\1###') + end + private_class_method :redact + def self.colorize(text, color, bold: false) mode = bold ? 1 : 0 foreground = 30 + COLOR_CODES.fetch(color) @@ -36,6 +51,10 @@ def self.colorize(text, color, bold: false) end private_class_method :colorize + # The token characters allowed by RFC 6750 (b64token). + BEARER_TOKEN = %r{(bearer\s+)[A-Za-z0-9\-._~+/]+=*}i + private_constant :BEARER_TOKEN + COLOR_CODES = { black: 0, red: 1, diff --git a/lib/procore/version.rb b/lib/procore/version.rb index 1538020..f1c0bff 100644 --- a/lib/procore/version.rb +++ b/lib/procore/version.rb @@ -1,3 +1,3 @@ module Procore - VERSION = "1.3.1".freeze + VERSION = "2.0.0".freeze end diff --git a/procore.gemspec b/procore.gemspec index cdde3f6..6cdd7a2 100644 --- a/procore.gemspec +++ b/procore.gemspec @@ -14,7 +14,7 @@ Gem::Specification.new do |spec| spec.description = "Procore Ruby Gem" spec.homepage = "https://github.com/procore-oss/ruby-sdk" spec.license = "MIT" - spec.required_ruby_version = Gem::Requirement.new('>= 3.0') + spec.required_ruby_version = Gem::Requirement.new('>= 3.3') spec.files = `git ls-files -z`.split("\x0").reject do |f| f.match(%r{^(test|spec|features)/}) @@ -26,9 +26,12 @@ Gem::Specification.new do |spec| spec.add_development_dependency "actionpack" spec.add_development_dependency "activerecord" spec.add_development_dependency "bundler" + spec.add_development_dependency "bundler-audit" spec.add_development_dependency "dalli" spec.add_development_dependency "fakefs" + spec.add_development_dependency "irb" spec.add_development_dependency "minitest" + spec.add_development_dependency "minitest-mock" spec.add_development_dependency "pry" spec.add_development_dependency "rake" spec.add_development_dependency "redis" @@ -39,9 +42,13 @@ Gem::Specification.new do |spec| spec.add_development_dependency "sqlite3" spec.add_development_dependency "webmock" - spec.add_dependency "activesupport", "> 2.4" + spec.add_dependency "activesupport", ">= 7.0" + spec.add_dependency "faraday", "~> 2.0" + spec.add_dependency "faraday-multipart", "~> 1.0" + spec.add_dependency "faraday-retry", "~> 2.0" + spec.add_dependency "mime-types", "~> 3.0" spec.add_dependency "oauth2", "~> 2.0" - spec.add_dependency "rest-client", "~> 2.0" + spec.add_dependency "pstore" spec.metadata['allowed_push_host'] = 'https://rubygems.org' spec.metadata['rubygems_mfa_required'] = 'true' diff --git a/test/procore/auth/access_token_credentials_test.rb b/test/procore/auth/access_token_credentials_test.rb index 205d506..0851bab 100644 --- a/test/procore/auth/access_token_credentials_test.rb +++ b/test/procore/auth/access_token_credentials_test.rb @@ -1,6 +1,6 @@ require "test_helper" -class Procore::Auth::ClientCredentialsTest < Minitest::Test +class Procore::Auth::AccessTokenCredentialsTest < Minitest::Test def test_refresh_token stub_request(:post, "https://procore.example.com/oauth/token") .with(body: { @@ -58,11 +58,11 @@ def test_oauth_client_error_with_html credentials.refresh(token: "token", refresh: "refresh") end - assert_equal error.response.code, 500 - assert_equal error.response.body, "

This is very bad

" - assert_equal error.response.headers, "content-type" => "text/html" + assert_equal 500, error.response.code + assert_equal "

This is very bad

", error.response.body + assert_equal({ "content-type" => "text/html" }, error.response.headers) assert_nil error.response.request.path - assert_equal error.response.request.options, {} + assert_equal({}, error.response.request.options) end def test_oauth_client_error_with_json @@ -89,11 +89,11 @@ def test_oauth_client_error_with_json credentials.refresh(token: "token", refresh: "refresh") end - assert_equal error.response.code, 500 - assert_equal error.response.body, "error" => "Some bad error" - assert_equal error.response.headers, "content-type" => "application/json" + assert_equal 500, error.response.code + assert_equal({ "error" => "Some bad error" }, error.response.body) + assert_equal({ "content-type" => "application/json" }, error.response.headers) assert_nil error.response.request.path - assert_equal error.response.request.options, {} + assert_equal({}, error.response.request.options) end def test_connection_failed_error @@ -136,6 +136,31 @@ def test_bad_uri_error credentials.refresh(token: "token", refresh: "refresh") end - assert_equal error.message, "Host is not a valid URI. Check your host option to make sure it is a properly formed url" + assert_equal "Host is not a valid URI. Check your host option to make sure it is a properly formed url", error.message + end + + def test_revoke_failure_raises_oauth_error + stub_request(:post, "https://procore.example.com/oauth/revoke") + .to_return( + status: 500, + body: { error: "Some bad error" }.to_json, + headers: { "Content-Type" => "application/json" }, + ) + credentials = Procore::Auth::AccessTokenCredentials.new( + client_id: "id", + client_secret: "secret", + host: "https://procore.example.com", + ) + token = Procore::Auth::Token.new( + access_token: "token", + refresh_token: "refresh", + expires_at: 0, + ) + + error = assert_raises Procore::OAuthError do + credentials.revoke(token: token) + end + + assert_equal 500, error.response.code end end diff --git a/test/procore/auth/client_credentials_test.rb b/test/procore/auth/client_credentials_test.rb index f475f69..4657100 100644 --- a/test/procore/auth/client_credentials_test.rb +++ b/test/procore/auth/client_credentials_test.rb @@ -46,11 +46,11 @@ def test_oauth_client_error_with_html token.refresh end - assert_equal error.response.code, 500 - assert_equal error.response.body, "

This is very bad

" - assert_equal error.response.headers, "content-type" => "text/html" + assert_equal 500, error.response.code + assert_equal "

This is very bad

", error.response.body + assert_equal({ "content-type" => "text/html" }, error.response.headers) assert_nil error.response.request.path - assert_equal error.response.request.options, {} + assert_equal({}, error.response.request.options) end def test_oauth_client_error_with_json @@ -76,11 +76,11 @@ def test_oauth_client_error_with_json token.refresh end - assert_equal error.response.code, 500 - assert_equal error.response.body, "error" => "Some bad error" - assert_equal error.response.headers, "content-type" => "application/json" + assert_equal 500, error.response.code + assert_equal({ "error" => "Some bad error" }, error.response.body) + assert_equal({ "content-type" => "application/json" }, error.response.headers) assert_nil error.response.request.path - assert_equal error.response.request.options, {} + assert_equal({}, error.response.request.options) end def test_connection_failed_error @@ -124,7 +124,7 @@ def test_bad_uri_error token.refresh end - assert_equal error.message, "Host is not a valid URI. Check your host option to make sure it is a properly formed url" + assert_equal "Host is not a valid URI. Check your host option to make sure it is a properly formed url", error.message end def test_procore_oauth_error diff --git a/test/procore/auth/stores/dalli_store_test.rb b/test/procore/auth/stores/dalli_store_test.rb index 71b3e9f..99f51ba 100644 --- a/test/procore/auth/stores/dalli_store_test.rb +++ b/test/procore/auth/stores/dalli_store_test.rb @@ -3,7 +3,10 @@ class Procore::Auth::Stores::DalliTest < Minitest::Test def setup - @store = Procore::Auth::Stores::Dalli.new(key: "key", dalli: Dalli::Client.new) + @store = Procore::Auth::Stores::Dalli.new( + key: "key", + dalli: Dalli::Client.new(nil, serializer: Dalli::JSONSerializer), + ) @store.save( Procore::Auth::Token.new( access_token: "token", diff --git a/test/procore/requestable_test.rb b/test/procore/requestable_test.rb index a40ffb5..bc37d54 100644 --- a/test/procore/requestable_test.rb +++ b/test/procore/requestable_test.rb @@ -21,9 +21,29 @@ def options end end + def with_max_retries(count) + max_retries_old = Procore.configuration.max_retries + Procore.configuration.max_retries = count + yield + ensure + Procore.configuration.max_retries = max_retries_old + end + + # Runs the block with retry backoff recorded instead of slept. Covers both + # the retry middleware and the access-token retry in Requestable. + # + # @return [Array] the waits requested, in order + def record_sleeps(request) + RetrySleeps.recorded.clear + request.stub(:sleep, ->(seconds) { RetrySleeps.recorded << seconds }) do + yield + end + RetrySleeps.recorded.dup + end + def headers { - "Accepts" => "application/json", + "Accept" => "application/json", "Authorization" => "Bearer token", "Content-Type" => "application/json", "Procore-Sdk-Version" => Procore::VERSION, @@ -44,6 +64,16 @@ def test_get assert_requested request end + def test_get_does_not_send_a_misspelled_accept_header + request = stub_request(:get, "http://test.com/rest/v1.0/home") + .with { |req| !req.headers.key?("Accepts") } + .to_return(status: 200, body: "", headers: {}) + + Request.new(token: "token").get("home") + + assert_requested request + end + def test_rest_get request = stub_request(:get, "http://test.com/rest/v1.1/home") .with( @@ -217,7 +247,7 @@ def test_sync errors: { id: ["Entity with this ID not found"] }, }], } - assert response.body, expected_body + assert_equal expected_body.deep_stringify_keys, response.body end def test_post_with_idempotency_token @@ -268,6 +298,53 @@ def test_post_with_company_id assert_requested request end + def test_delete_with_company_id + request = stub_request(:delete, "http://test.com/rest/v1.0/home") + .with( + headers: headers.merge("procore-company-id" => "1") + ) + .to_return(status: 200, body: "", headers: {}) + + Request.new(token: "token").delete( + "home", + options: { company_id: 1 }, + ) + + assert_requested request + end + + def test_delete_does_not_log_access_token + stub_request(:delete, "http://test.com/rest/v1.0/home") + .to_return(status: 200, body: "", headers: {}) + output = StringIO.new + logger_old = Procore.configuration.logger + Procore.configuration.logger = Logger.new(output) + + begin + Request.new(token: "secret-token").delete("home") + ensure + Procore.configuration.logger = logger_old + end + + refute_includes output.string, "secret-token" + end + + def test_logs_request_id + stub_request(:get, "http://test.com/rest/v1.0/home") + .to_return(status: 200, body: "", headers: { "X-Request-Id" => "req-123" }) + output = StringIO.new + logger_old = Procore.configuration.logger + Procore.configuration.logger = Logger.new(output) + + begin + Request.new(token: "token").get("home") + ensure + Procore.configuration.logger = logger_old + end + + assert_includes output.string, "req-123" + end + def test_post_with_multipart_body request = stub_request(:post, "http://test.com/rest/v1.0/home") .with( @@ -326,11 +403,11 @@ def test_error_body headers: {}, ) - begin + error = assert_raises Procore::AuthorizationError do Request.new(token: "token").get("") - rescue Procore::AuthorizationError => e - assert_equal({ "errors" => "Unauthorized" }, e.response.body) end + + assert_equal({ "errors" => "Unauthorized" }, error.response.body) end def test_no_retry_logic @@ -340,7 +417,7 @@ def test_no_retry_logic begin Procore.configuration.max_retries = 0 stub_request(:get, "http://test.com/rest/v1.0/retry_test") - .to_raise(RestClient::Exceptions::Timeout) + .to_timeout # Attempt to perform the request and assert the correct exception is raised assert_raises(Procore::APIConnectionError) do @@ -352,4 +429,414 @@ def test_no_retry_logic Procore.configuration.max_retries = max_retries_old end end + + def test_get_encodes_nested_query + request = stub_request(:get, "http://test.com/rest/v1.0/home") + .with(query: { filters: { status: "open", ids: [1, 2] }, per_page: 5 }) + .to_return(status: 200, body: "", headers: {}) + + Request.new(token: "token").get( + "home", + query: { filters: { status: "open", ids: [1, 2] }, per_page: 5 }, + ) + + assert_requested request + end + + def test_delete_with_query + request = stub_request(:delete, "http://test.com/rest/v1.0/home") + .with(query: { force: true }, headers: headers) + .to_return(status: 200, body: "", headers: {}) + + Request.new(token: "token").delete("home", query: { force: true }) + + assert_requested request + end + + def test_post_multipart_sends_file_and_fields + request = stub_request(:post, "http://test.com/rest/v1.0/home") + .with { |req| + req.body.include?('name="file"; filename="pixel.png"') && + req.body.include?('name="description"') && + req.body.include?("A pixel") + } + .to_return(status: 200, body: "", headers: {}) + + pixel = File.new("test/support/pixel.png", "r") + + Request.new(token: "token").post( + "home", + body: { file: pixel, description: "A pixel" }, + ) + + assert_requested request + end + + def test_post_multipart_sends_file_content_type + request = stub_request(:post, "http://test.com/rest/v1.0/home") + .with { |req| + req.body.match?(/filename="pixel.png"\r\nContent-Length: \d+\r\nContent-Type: image\/png/) + } + .to_return(status: 200, body: "", headers: {}) + + pixel = File.new("test/support/pixel.png", "r") + + Request.new(token: "token").post("home", body: { file: pixel }) + + assert_requested request + end + + def test_post_multipart_sends_file_arrays + request = stub_request(:post, "http://test.com/rest/v1.0/home") + .with { |req| req.body.scan('filename="pixel.png"').size == 2 } + .to_return(status: 200, body: "", headers: {}) + + files = [ + File.new("test/support/pixel.png", "r"), + File.new("test/support/pixel.png", "r"), + ] + + Request.new(token: "token").post("home", body: { files: files }) + + assert_requested request + end + + def test_response_headers_use_symbol_keys + stub_request(:get, "http://test.com/rest/v1.0/home") + .to_return( + status: 200, + body: "", + headers: { "X-Request-Id" => "abc", "Content-Type" => "application/json" }, + ) + + response = Request.new(token: "token").get("home") + + assert_equal "abc", response.headers[:x_request_id] + assert_equal "application/json", response.headers[:content_type] + end + + def test_response_pagination_from_link_header + stub_request(:get, "http://test.com/rest/v1.0/projects") + .to_return( + status: 200, + body: "[]", + headers: { + "Link" => '; rel="next", ' \ + '; rel="last"', + }, + ) + + response = Request.new(token: "token").get("projects") + + assert_equal( + { next: "rest/v1.0/projects?page=2", last: "rest/v1.0/projects?page=9" }, + response.pagination, + ) + end + + def test_following_a_next_link_keeps_the_requested_version + stub_request(:get, "http://test.com/rest/v1.1/projects") + .to_return( + status: 200, + body: "[]", + headers: { "Link" => '; rel="next"' }, + ) + next_page = stub_request(:get, "http://test.com/rest/v1.1/projects?page=2") + .to_return(status: 200, body: "[]", headers: {}) + client = Request.new(token: "token") + + first_page = client.get("projects", version: "v1.1") + client.get(first_page.pagination[:next]) + + assert_requested next_page + end + + def test_following_a_vapid_next_link_stays_on_vapid + stub_request(:get, "http://test.com/vapid/projects") + .to_return( + status: 200, + body: "[]", + headers: { "Link" => '; rel="next"' }, + ) + next_page = stub_request(:get, "http://test.com/vapid/projects?page=2") + .to_return(status: 200, body: "[]", headers: {}) + client = Request.new(token: "token") + + first_page = client.get("projects", version: "vapid") + client.get(first_page.pagination[:next]) + + assert_requested next_page + end + + def test_versioned_path_is_not_prefixed_again + request = stub_request(:delete, "http://test.com/rest/v2.0/companies/1/projects/2") + .to_return(status: 204, body: "", headers: {}) + + Request.new(token: "token").delete("rest/v2.0/companies/1/projects/2") + + assert_requested request + end + + def test_response_exposes_code_request_and_request_body + stub_request(:post, "http://test.com/rest/v1.0/home") + .to_return(status: 201, body: { id: 1 }.to_json, headers: {}) + + response = Request.new(token: "token").post("home", body: { name: "Name" }) + + assert_equal 201, response.code + assert_equal({ "id" => 1 }, response.body) + assert_equal({ name: "Name" }, response.request_body) + assert_equal "http://test.com/rest/v1.0/home", response.request.url + assert_equal "post", response.request.method.to_s + end + + # Response#request keeps the values RestClient::Request gave callers. + def test_response_request_matches_rest_client_values + stub_request(:get, "http://test.com/rest/v1.0/home") + .with(query: { per_page: 5 }) + .to_return(status: 200, body: "", headers: {}) + + request = Request.new(token: "token").get("home", query: { per_page: 5 }).request + + assert_equal "http://test.com/rest/v1.0/home?per_page=5", request.url + assert_equal "get", request.method + assert_equal "Bearer token", request.headers["Authorization"] + end + + def test_sync_caps_batch_size_at_1000 + updates = (1..1001).map { |id| { id: id } } + stub_request(:patch, "http://test.com/rest/v1.0/projects/sync") + .to_return(status: 200, body: { entities: [], errors: [] }.to_json) + + Request.new(token: "token").sync( + "projects/sync", + body: { updates: updates }, + options: { batch_size: 5000 }, + ) + + assert_requested :patch, "http://test.com/rest/v1.0/projects/sync", times: 2 + end + + def test_invalid_version_raises + assert_raises Procore::InvalidRequestError do + Request.new(token: "token").get("home", version: "v1") + end + end + + def test_bad_request_error + stub_request(:get, "http://test.com/rest/v1.0/") + .to_return(status: 400, body: "", headers: {}) + + error = assert_raises Procore::InvalidRequestError do + Request.new(token: "token").get("") + end + + assert_equal 400, error.response.code + end + + def test_unprocessable_entity_error + stub_request(:get, "http://test.com/rest/v1.0/") + .to_return(status: 422, body: "", headers: {}) + + error = assert_raises Procore::InvalidRequestError do + Request.new(token: "token").get("") + end + + assert_equal 422, error.response.code + end + + def test_rate_limit_error + stub_request(:get, "http://test.com/rest/v1.0/") + .to_return(status: 429, body: "", headers: {}) + + error = assert_raises Procore::RateLimitError do + Request.new(token: "token").get("") + end + + assert_equal 429, error.response.code + end + + def test_retries_timeout_with_backoff_then_raises + stub_request(:get, "http://test.com/rest/v1.0/retry_test").to_timeout + request = Request.new(token: "token") + + sleeps = with_max_retries(2) do + record_sleeps(request) do + assert_raises(Procore::APIConnectionError) do + request.get("retry_test") + end + end + end + + assert_requested :get, "http://test.com/rest/v1.0/retry_test", times: 3 + assert_equal [1.5, 2.25], sleeps + end + + def test_retry_succeeds_after_transient_failure + stub_request(:get, "http://test.com/rest/v1.0/retry_test") + .to_timeout + .then + .to_return(status: 200, body: { ok: true }.to_json, headers: {}) + + request = Request.new(token: "token") + + response = nil + + with_max_retries(1) do + record_sleeps(request) do + response = request.get("retry_test") + end + end + + assert_equal({ "ok" => true }, response.body) + assert_requested :get, "http://test.com/rest/v1.0/retry_test", times: 2 + end + + def test_retries_read_timeout + stub_request(:get, "http://test.com/rest/v1.0/retry_test") + .to_raise(Net::ReadTimeout) + request = Request.new(token: "token") + + with_max_retries(1) do + record_sleeps(request) do + assert_raises(Procore::APIConnectionError) do + request.get("retry_test") + end + end + end + + assert_requested :get, "http://test.com/rest/v1.0/retry_test", times: 2 + end + + def test_access_token_failure_raises_connection_error_after_retries + client = Request.new(token: "token") + failing_token = -> { raise Procore::OAuthError.new("Token refresh failed") } + + error = nil + + with_max_retries(1) do + record_sleeps(client) do + client.stub(:access_token, failing_token) do + error = assert_raises(Procore::APIConnectionError) do + client.get("home") + end + end + end + end + + assert_instance_of Procore::OAuthError, error.cause + end + + def test_retries_connection_refused + stub_request(:get, "http://test.com/rest/v1.0/retry_test") + .to_raise(Errno::ECONNREFUSED) + request = Request.new(token: "token") + + with_max_retries(1) do + record_sleeps(request) do + assert_raises(Procore::APIConnectionError) do + request.get("retry_test") + end + end + end + + assert_requested :get, "http://test.com/rest/v1.0/retry_test", times: 2 + end + + def test_retries_connection_reset + stub_request(:get, "http://test.com/rest/v1.0/retry_test") + .to_raise(Errno::ECONNRESET) + request = Request.new(token: "token") + + with_max_retries(1) do + record_sleeps(request) do + assert_raises(Procore::APIConnectionError) do + request.get("retry_test") + end + end + end + + assert_requested :get, "http://test.com/rest/v1.0/retry_test", times: 2 + end + + def test_post_is_retried + stub_request(:post, "http://test.com/rest/v1.0/retry_test").to_timeout + request = Request.new(token: "token") + + with_max_retries(1) do + record_sleeps(request) do + assert_raises(Procore::APIConnectionError) do + request.post("retry_test", body: { name: "Name" }) + end + end + end + + assert_requested :post, "http://test.com/rest/v1.0/retry_test", times: 2 + end + + def test_patch_is_retried + stub_request(:patch, "http://test.com/rest/v1.0/retry_test").to_timeout + request = Request.new(token: "token") + + with_max_retries(1) do + record_sleeps(request) do + assert_raises(Procore::APIConnectionError) do + request.patch("retry_test", body: { name: "Name" }) + end + end + end + + assert_requested :patch, "http://test.com/rest/v1.0/retry_test", times: 2 + end + + def test_server_error_is_not_retried + stub_request(:get, "http://test.com/rest/v1.0/retry_test") + .to_return(status: 500, body: "", headers: {}) + + with_max_retries(2) do + assert_raises(Procore::ServerError) do + Request.new(token: "token").get("retry_test") + end + end + + assert_requested :get, "http://test.com/rest/v1.0/retry_test", times: 1 + end + + def test_rate_limit_is_not_retried + stub_request(:get, "http://test.com/rest/v1.0/retry_test") + .to_return(status: 429, body: "", headers: {}) + + with_max_retries(2) do + assert_raises(Procore::RateLimitError) do + Request.new(token: "token").get("retry_test") + end + end + + assert_requested :get, "http://test.com/rest/v1.0/retry_test", times: 1 + end + + def test_access_token_failure_is_retried + request = stub_request(:get, "http://test.com/rest/v1.0/home") + .to_return(status: 200, body: "", headers: {}) + client = Request.new(token: "token") + token_attempts = 0 + flaky_token = -> { + token_attempts += 1 + if token_attempts == 1 + raise Procore::OAuthError.new("Token refresh failed") + else + "token" + end + } + + with_max_retries(1) do + record_sleeps(client) do + client.stub(:access_token, flaky_token) do + client.get("home") + end + end + end + + assert_requested request, times: 1 + end end diff --git a/test/procore/response_test.rb b/test/procore/response_test.rb index 6ca89d3..fe36e2a 100644 --- a/test/procore/response_test.rb +++ b/test/procore/response_test.rb @@ -13,6 +13,30 @@ def test_response_body assert_equal({ "key" => "value" }, response.body) end + def test_empty_body_parses_to_an_empty_hash + response = Procore::Response.new( + body: "", + code: 204, + headers: {}, + request: nil, + request_body: nil, + ) + + assert_equal({}, response.body) + end + + def test_nil_body_parses_to_an_empty_hash + response = Procore::Response.new( + body: nil, + code: 204, + headers: {}, + request: nil, + request_body: nil, + ) + + assert_equal({}, response.body) + end + def test_response_body_as_collection response_body = [{ "key" => "value" }] @@ -27,7 +51,7 @@ def test_response_body_as_collection assert_equal(response_body, response.body) end - def shared_response_pagination_parsing_test(base_path) + def shared_response_pagination_parsing_test(base_path, version_path) links = %W( <#{base_path}/projects?page=1>; rel="first", <#{base_path}/projects?page=173>; rel="last", @@ -45,24 +69,28 @@ def shared_response_pagination_parsing_test(base_path) assert_equal( { - first: "projects?page=1", - last: "projects?page=173", - next: "projects?page=6", - prev: "projects?page=4", + first: "#{version_path}/projects?page=1", + last: "#{version_path}/projects?page=173", + next: "#{version_path}/projects?page=6", + prev: "#{version_path}/projects?page=4", }, response.pagination, ) end def test_rest_response_pagination_parsing - shared_response_pagination_parsing_test("http://localhost:3000/rest/v1.0") + shared_response_pagination_parsing_test("http://localhost:3000/rest/v1.0", "rest/v1.0") + end + + def test_rest_v2_response_pagination_parsing + shared_response_pagination_parsing_test("http://localhost:3000/rest/v2.0", "rest/v2.0") end def test_vapid_response_pagination_parsing - shared_response_pagination_parsing_test("http://localhost:3000/vapid") + shared_response_pagination_parsing_test("http://localhost:3000/vapid", "vapid") end - def shared_response_pagination_parsing_on_first_page(base_path) + def shared_response_pagination_parsing_on_first_page(base_path, version_path) links = %W( <#{base_path}/projects?page=173>; rel="last", <#{base_path}/projects?page=6>; rel="next" @@ -78,19 +106,19 @@ def shared_response_pagination_parsing_on_first_page(base_path) assert_equal( { - last: "projects?page=173", - next: "projects?page=6", + last: "#{version_path}/projects?page=173", + next: "#{version_path}/projects?page=6", }, response.pagination, ) end def test_vapid_response_pagination_parsing_on_first_page - shared_response_pagination_parsing_on_first_page("http://localhost:3000/vapid") + shared_response_pagination_parsing_on_first_page("http://localhost:3000/vapid", "vapid") end def test_rest_response_pagination_parsing_on_first_page - shared_response_pagination_parsing_on_first_page("http://localhost:3000/rest/v1.0") + shared_response_pagination_parsing_on_first_page("http://localhost:3000/rest/v1.0", "rest/v1.0") end def test_response_pagination_no_links diff --git a/test/procore/rest_client_leakage_test.rb b/test/procore/rest_client_leakage_test.rb new file mode 100644 index 0000000..3b4c2d6 --- /dev/null +++ b/test/procore/rest_client_leakage_test.rb @@ -0,0 +1,149 @@ +require "test_helper" + +# The HTTP library behind Procore::Requestable is an implementation detail. +# These tests fail when rest-client types reach callers of the client, and pin +# rest-client behaviour that callers rely on. See +# docs/adr/0001-replace-rest-client-with-faraday.md. +class Procore::RestClientLeakageTest < Minitest::Test + class Request + include Procore::Requestable + + def access_token + "token" + end + + def base_api_path + "http://test.com" + end + + def options + { + default_version: "v1.0", + } + end + end + + def setup + @max_retries_old = Procore.configuration.max_retries + Procore.configuration.max_retries = 0 + end + + def teardown + Procore.configuration.max_retries = @max_retries_old + end + + def assert_not_rest_client(object) + refute object.class.name.to_s.start_with?("RestClient"), + "#{object.class} leaked to the caller" + end + + def test_response_request_is_not_a_rest_client_object + stub_request(:get, "http://test.com/rest/v1.0/home") + .to_return(status: 200, body: "", headers: {}) + + response = Request.new.get("home") + + assert_not_rest_client response.request + end + + def test_error_response_request_is_not_a_rest_client_object + stub_request(:get, "http://test.com/rest/v1.0/home") + .to_return(status: 404, body: "", headers: {}) + + error = assert_raises Procore::NotFoundError do + Request.new.get("home") + end + + assert_not_rest_client error.response.request + end + + def test_response_headers_are_a_plain_hash_with_symbol_keys + stub_request(:get, "http://test.com/rest/v1.0/home") + .to_return(status: 200, body: "", headers: { "X-Request-Id" => "abc" }) + + response = Request.new.get("home") + + assert_instance_of Hash, response.headers + assert response.headers.keys.all?(Symbol) + end + + def test_timeout_cause_is_not_a_rest_client_exception + stub_request(:get, "http://test.com/rest/v1.0/home").to_timeout + + error = assert_raises Procore::APIConnectionError do + Request.new.get("home") + end + + assert_not_rest_client error.cause + end + + def test_broken_connection_cause_is_not_a_rest_client_exception + stub_request(:get, "http://test.com/rest/v1.0/home").to_raise(EOFError) + + error = assert_raises Procore::APIConnectionError do + Request.new.get("home") + end + + assert_not_rest_client error.cause + end + + def test_ssl_failure_raises_procore_error + stub_request(:get, "http://test.com/rest/v1.0/home") + .to_raise(OpenSSL::SSL::SSLError.new("certificate verify failed")) + + assert_raises Procore::APIConnectionError do + Request.new.get("home") + end + end + + def test_dns_failure_raises_procore_error + stub_request(:get, "http://test.com/rest/v1.0/home").to_raise(SocketError) + + assert_raises Procore::APIConnectionError do + Request.new.get("home") + end + end + + def test_unreachable_host_raises_procore_error + stub_request(:get, "http://test.com/rest/v1.0/home") + .to_raise(Errno::EHOSTUNREACH) + + assert_raises Procore::APIConnectionError do + Request.new.get("home") + end + end + + def test_http_exceptions_constant_has_no_rest_client_classes + names = Procore::Requestable::HTTP_EXCEPTIONS.map(&:name) + + assert_empty names.grep(/\ARestClient\b/) + end + + def test_requiring_procore_does_not_load_rest_client + loaded = system( + RbConfig.ruby, + "-Ilib", + "-e", + 'require "procore"; exit(defined?(RestClient) ? 1 : 0)', + ) + + assert loaded, "require \"procore\" defined RestClient" + end + + # rest-client follows redirects on GET. Faraday does not unless configured, + # so this pins the behaviour across the switch. + def test_get_follows_redirects + stub_request(:get, "http://test.com/rest/v1.0/old") + .to_return( + status: 301, + headers: { "Location" => "http://test.com/rest/v1.0/new" }, + ) + stub_request(:get, "http://test.com/rest/v1.0/new") + .to_return(status: 200, body: { moved: true }.to_json, headers: {}) + + response = Request.new.get("old") + + assert_equal 200, response.code + assert_equal({ "moved" => true }, response.body) + end +end diff --git a/test/procore/standalone_load_test.rb b/test/procore/standalone_load_test.rb new file mode 100644 index 0000000..b8d4291 --- /dev/null +++ b/test/procore/standalone_load_test.rb @@ -0,0 +1,91 @@ +require "test_helper" + +# The test suite loads ActiveRecord and ActionPack, which pull in every +# ActiveSupport core extension. An application that is not Rails only gets +# what the gem itself requires, so these tests run a script in a fresh +# process with nothing but `require "procore"`. +class Procore::StandaloneLoadTest < Minitest::Test + def run_standalone(script) + output = IO.popen( + [RbConfig.ruby, "-Ilib", "-e", script], + err: [:child, :out], + &:read + ) + + [$?.success?, output] + end + + def test_session_store_round_trips_a_token + passed, output = run_standalone(<<~RUBY) + require "procore" + + store = Procore::Auth::Stores::Session.new(session: {}) + store.save( + Procore::Auth::Token.new( + access_token: "access", + refresh_token: "refresh", + expires_at: 123, + ), + ) + token = store.fetch + + exit(token.access_token == "access" && token.expires_at == 123) + RUBY + + assert passed, output + end + + def test_sync_combines_batched_responses + passed, output = run_standalone(<<~RUBY) + require "procore" + + class Request + include Procore::Requestable + + def access_token = "token" + def base_api_path = "http://test.com" + def options = { default_version: "v1.0" } + + private + + # Answers each batch without a network, with the shape a sync returns. + def perform(_method, _url, headers:, params: nil, body: nil) + Faraday::Response.new( + Faraday::Env.from( + status: 200, + body: { entities: [JSON.parse(body)["updates"].first], errors: [] }.to_json, + response_headers: {}, + request_headers: headers, + url: URI("http://test.com/rest/v1.0/projects/sync"), + method: :patch, + ), + ) + end + end + + response = Request.new.sync( + "projects/sync", + body: { updates: [{ id: 1 }, { id: 2 }] }, + options: { batch_size: 1 }, + ) + + exit(response.body["entities"].map { |e| e["id"] } == [1, 2]) + RUBY + + assert passed, output + end + + def test_oauth_error_keeps_the_failed_response + passed, output = run_standalone(<<~RUBY) + require "procore" + + oauth_response = Struct.new(:status, :parsed, :body, :headers) + .new(401, nil, "invalid_grant", {}) + error = Procore::OAuthError.new("refresh failed", response: oauth_response) + + exit(error.response.code == 401 && error.response.body == "invalid_grant") + RUBY + + assert passed, output + end +end diff --git a/test/procore/util_test.rb b/test/procore/util_test.rb index 49018a8..39ce1f4 100644 --- a/test/procore/util_test.rb +++ b/test/procore/util_test.rb @@ -6,8 +6,45 @@ def setup Procore.configuration.logger = Logger.new(@output) end + def teardown + Procore.configuration.logger = nil + end + def test_log_info Procore::Util.log_info("Hello") assert_match /Hello/, @output.string end + + def test_log_info_redacts_bearer_token + Procore::Util.log_info( + "Request", + headers: { "Authorization" => "Bearer abc.DEF-123_xyz=" }, + ) + + refute_includes @output.string, "abc.DEF-123_xyz" + assert_includes @output.string, "Bearer ###" + end + + def test_log_error_redacts_bearer_token + Procore::Util.log_error( + "Request failed", + headers: { "Authorization" => "Bearer abc.DEF-123_xyz=" }, + ) + + refute_includes @output.string, "abc.DEF-123_xyz" + assert_includes @output.string, "Bearer ###" + end + + def test_log_info_redacts_bearer_token_in_message + Procore::Util.log_info("Sent Authorization: bearer secret-token") + + refute_includes @output.string, "secret-token" + assert_includes @output.string, "bearer ###" + end + + def test_log_info_keeps_text_without_bearer_token + Procore::Util.log_info("Request", path: "/rest/v1.0/projects?per_page=5") + + assert_includes @output.string, "/rest/v1.0/projects?per_page=5" + end end diff --git a/test/support/retry_sleeps.rb b/test/support/retry_sleeps.rb new file mode 100644 index 0000000..78dbbd9 --- /dev/null +++ b/test/support/retry_sleeps.rb @@ -0,0 +1,15 @@ +require "faraday/retry" + +# Records faraday-retry's backoff instead of sleeping, so retry tests run fast +# and can assert on the waits. +module RetrySleeps + def self.recorded + Thread.current[:procore_retry_sleeps] ||= [] + end + + def sleep(seconds) + RetrySleeps.recorded << seconds + end +end + +Faraday::Retry::Middleware.prepend(RetrySleeps) diff --git a/test/test_helper.rb b/test/test_helper.rb index 400c91b..a238f27 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -9,16 +9,18 @@ ]) SimpleCov.start do - track_files "lib/**/*.{rb}" - add_filter "test/" + cover "lib/**/*.{rb}" + skip "test/" end end require "minitest/autorun" +require "minitest/mock" require "webmock/minitest" require "support/auth_stubs" require "support/database" +require "support/retry_sleeps" require "procore"