diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index a60773c598..861978829e 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -49,7 +49,7 @@ def authenticated? end def oauth_grant? - Current.access_token&.oauth_client_id? + Current.access_token&.oauth? || false end def forbid_oauth_grant @@ -105,7 +105,7 @@ def authenticate_with_bearer_token Current.access_token = access_token Current.identity = access_token.identity elsif access_token - request_bearer_authentication status: access_token.oauth_client_id? ? :forbidden : :unauthorized, + request_bearer_authentication status: access_token.connected_app ? :forbidden : :unauthorized, error: "insufficient_scope", error_description: "The access token is read-only", scope: "write" else request_bearer_authentication error: "invalid_token", error_description: "The access token is expired, revoked, or invalid" diff --git a/app/controllers/my/access_tokens_controller.rb b/app/controllers/my/access_tokens_controller.rb index 50917ae5fd..593b5c26e8 100644 --- a/app/controllers/my/access_tokens_controller.rb +++ b/app/controllers/my/access_tokens_controller.rb @@ -15,7 +15,7 @@ def new # The raw token is shown in this response and on no page after it, so the # response must not be cached. def create - @access_token = my_access_tokens.create!(access_token_params) + @access_token = my_access_tokens.create!(access_token_params.merge(oauth_client: Oauth::Client.personal_access_issuer)) no_store respond_to do |format| diff --git a/app/controllers/oauth/authorizations_controller.rb b/app/controllers/oauth/authorizations_controller.rb index 3fbc864dc3..bbaea58071 100644 --- a/app/controllers/oauth/authorizations_controller.rb +++ b/app/controllers/oauth/authorizations_controller.rb @@ -83,9 +83,16 @@ def save_oauth_return_url end end + # A client not registered for the code grant, like the personal access + # token issuer, has nothing to authorize here. def set_client @client = Oauth::Client.find_by(client_id: params[:client_id]) - oauth_error("invalid_request", "Unknown client") unless @client + + if @client.nil? + oauth_error "invalid_request", "Unknown client" + elsif !@client.allows_grant_type?("authorization_code") + oauth_error "unauthorized_client", "Client may not use the authorization code grant" + end end def validate_redirect_uri diff --git a/app/controllers/oauth/revocations_controller.rb b/app/controllers/oauth/revocations_controller.rb index a8bc10babf..189001bc3d 100644 --- a/app/controllers/oauth/revocations_controller.rb +++ b/app/controllers/oauth/revocations_controller.rb @@ -8,9 +8,11 @@ # with a missing or wrong secret, a Basic header that doesn't authenticate, # or a client secret that authenticates no client. # -# A personal access token was issued to no client, so possession of it is the -# credential: whoever presents one revokes it, client or not. Client -# credentials presented beside it must still authenticate, by the same rules. +# A personal access token is issued to no client that can authenticate here +# (none at all, or the personal access token issuer, which has no secret and no +# grant to revoke), so possession of it is the credential: whoever presents one +# revokes it, client or not. Client credentials presented beside it must still +# authenticate, by the same rules. class Oauth::RevocationsController < Oauth::BaseController include Oauth::ClientAuthentication diff --git a/app/controllers/oauth/tokens_controller.rb b/app/controllers/oauth/tokens_controller.rb index 5e388777a1..2af3f43f21 100644 --- a/app/controllers/oauth/tokens_controller.rb +++ b/app/controllers/oauth/tokens_controller.rb @@ -23,6 +23,7 @@ class Oauth::TokensController < Oauth::BaseController # the answer must not depend on whether the code or refresh token is live, # or it tells whoever holds one without the secret. before_action :authenticate_client + before_action :validate_client_grant_type before_action :require_params with_options if: :authorization_code_grant? do @@ -90,6 +91,15 @@ def validate_grant_type end end + # A client is refused a grant type it isn't registered for (RFC 6749 §5.2), + # so the personal access token issuer, registered for none, can never + # mint, refresh or exchange a token here. + def validate_client_grant_type + if requesting_client && !requesting_client.allows_grant_type?(params[:grant_type]) + oauth_error "unauthorized_client", "Client may not use the #{params[:grant_type]} grant" + end + end + # A missing parameter is a malformed request (invalid_request), not a dead # grant (invalid_grant), which a client would act on by discarding it. # client_id is required of every client, in the body unless a Basic diff --git a/app/controllers/users/push_subscriptions_controller.rb b/app/controllers/users/push_subscriptions_controller.rb index 7521300c76..2498100a0a 100644 --- a/app/controllers/users/push_subscriptions_controller.rb +++ b/app/controllers/users/push_subscriptions_controller.rb @@ -9,8 +9,8 @@ def index def create # A new subscription records its app as it's inserted, so no crash between # two writes can leave an app's endpoint looking like a browser's. - subscription = @push_subscriptions.create_with(user_agent: request.user_agent, oauth_client: Current.access_token&.oauth_client).create_or_find_by!(push_subscription_params) - subscription.claim_for Current.access_token&.oauth_client + subscription = @push_subscriptions.create_with(user_agent: request.user_agent, oauth_client: Current.access_token&.connected_app).create_or_find_by!(push_subscription_params) + subscription.claim_for Current.access_token&.connected_app respond_to do |format| format.html { head :no_content } diff --git a/app/models/identity/access_token.rb b/app/models/identity/access_token.rb index 0719f0af35..f6c97c7b75 100644 --- a/app/models/identity/access_token.rb +++ b/app/models/identity/access_token.rb @@ -23,8 +23,15 @@ class Identity::AccessToken < ApplicationRecord # revocation waits rather than deadlocking against the retired-token delete. before_destroy :lock_grant, prepend: true - scope :personal, -> { where oauth_client_id: nil } - scope :oauth, -> { where.not oauth_client_id: nil } + # A personal access token is issued by the personal access client + # (Oauth::Client::PERSONAL_ACCESS_CLIENT_ID). Ones made before that client + # existed, and ones production code that predates it still makes, have no + # client, and are just as personal. Every rule that sets personal tokens + # apart (honoring while OAuth is dark, never lapsing, no expiry or refresh + # token, revocation by possession, Connected Apps) reads one of these, or + # #personal?, so it holds for both forms alike. + scope :personal, -> { where(oauth_client_id: nil).or(where(oauth_client_id: Oauth::Client.personal_access.select(:id))) } + scope :oauth, -> { where.not(oauth_client_id: nil).where.not(oauth_client_id: Oauth::Client.personal_access.select(:id)) } scope :active, -> { where(expires_at: nil).or(where(expires_at: Time.current..)) } scope :unlapsed, -> { where(updated_at: REFRESH_IDLE_LIMIT.ago..) } scope :lapsed, -> { oauth.where(updated_at: ...REFRESH_IDLE_LIMIT.ago) } @@ -64,14 +71,14 @@ class Identity::AccessToken < ApplicationRecord attribute :raw_refresh_token, :string before_create :set_token - before_create :set_expiry_and_refresh_token, if: :oauth_client_id? + before_create :set_expiry_and_refresh_token, if: :oauth? # Issuing or destroying a grant restarts its client's retention period # (Oauth::Client.cleanup). The touch waits until the grant's own transaction # commits. A revocation locks the grant, and touching the client in the same # transaction would then wait on the client's lock. A replayed code exchange # holds that lock while it waits on the grant, so the two would deadlock. - after_commit :touch_oauth_client, on: %i[ create destroy ] + after_commit :touch_oauth_client, on: %i[ create destroy ], if: :oauth? # Every way a grant ends destroys it: disconnecting the app, revocation, the # idle-lapse sweep, a replayed refresh token or authorization code, and @@ -83,7 +90,7 @@ class Identity::AccessToken < ApplicationRecord # # Checked after commit, so when two grants to one client end at once, # whichever commits second finds none left. - after_destroy_commit :end_app_plumbing, if: :oauth_client_id? + after_destroy_commit :end_app_plumbing, if: :oauth? # Audit events (Oauth::Audit) are reported once the change commits, so an # event always describes a grant that was really issued or really ended. @@ -187,9 +194,24 @@ def find_and_digest_undigested(raw) # OAuth tokens are honored only while OAuth acceptance is on (see # Oauth::Availability); personal tokens always are. The client is loaded only - # when the server is dark and a pilot exemption might apply. + # when the server is dark, to tell a personal token from a grant and to see + # whether a pilot exemption applies. def honored? - oauth_client_id.nil? || Oauth::Availability.acceptance_enabled? || Oauth::Availability.acceptance_enabled?(oauth_client&.client_id) + oauth_client_id.nil? || Oauth::Availability.acceptance_enabled? || personal? || Oauth::Availability.acceptance_enabled?(oauth_client&.client_id) + end + + def personal? + oauth_client_id.nil? || oauth_client&.personal_access? || false + end + + def oauth? + !personal? + end + + # The app this token acts through, if it is a grant. A personal access token + # acts for its holder alone, so what it sets up is attributed to no app. + def connected_app + oauth_client if oauth? end def allows?(method) @@ -205,7 +227,7 @@ def expires_in end def lapsed? - oauth_client_id? && (updated_at + REFRESH_IDLE_LIMIT).past? + oauth? && (updated_at + REFRESH_IDLE_LIMIT).past? end # Ends the grant, recording why and who ended it (see #revocation). @@ -257,7 +279,7 @@ def refresh(permission: self.permission) private def record_issuance - if oauth_client_id? + if oauth? Oauth::Audit.record "code.redeemed", grant: self, actor: "client" if authorization_code_jti? Oauth::Audit.record "grant.created", grant: self, actor: "client", permission: permission Oauth::Audit.record "token.issued", grant: self, actor: "client", grant_type: "authorization_code", permission: permission @@ -268,7 +290,7 @@ def record_issuance def record_revocation ending = revocation || { reason: cascade_reason, actor: "system" } - Oauth::Audit.record oauth_client_id? ? "grant.revoked" : "pat.revoked", grant: self, **ending + Oauth::Audit.record oauth? ? "grant.revoked" : "pat.revoked", grant: self, **ending end # A grant destroyed along with its identity or its client, which only the @@ -288,7 +310,7 @@ def end_app_plumbing end def touch_oauth_client - Oauth::Client.where(id: oauth_client_id).touch_all if oauth_client_id? + Oauth::Client.where(id: oauth_client_id).touch_all end def lock_grant @@ -299,9 +321,9 @@ def lock_grant # for production code that reads only plaintext. An explicitly given # token is digested like a generated one. def set_token - self.raw_token ||= token.presence || self.class.generate_token(oauth_client_id? ? :access : :personal) + self.raw_token ||= token.presence || self.class.generate_token(personal? ? :personal : :access) self.token_digest = TokenDigester.digest(raw_token) - self.token = oauth_client_id? ? nil : raw_token + self.token = personal? ? raw_token : nil end def set_expiry_and_refresh_token diff --git a/app/models/oauth/audit.rb b/app/models/oauth/audit.rb index bc21f28122..ebe0e1cf8c 100644 --- a/app/models/oauth/audit.rb +++ b/app/models/oauth/audit.rb @@ -48,8 +48,12 @@ def record(event, grant: nil, client: grant&.oauth_client, identity_id: grant&.i end private + # A personal access token is a "pat" whether it predates the personal + # access client (no client) or was issued by it. def client_class(grant:, client:) - if client + if grant&.personal? || client&.personal_access? + "pat" + elsif client if client.dynamically_registered? "dcr" elsif client.trusted? @@ -57,8 +61,6 @@ def client_class(grant:, client:) else "partner" end - elsif grant && !grant.oauth_client_id? - "pat" end end end diff --git a/app/models/oauth/client.rb b/app/models/oauth/client.rb index 7b6a322da2..f1cdedac26 100644 --- a/app/models/oauth/client.rb +++ b/app/models/oauth/client.rb @@ -9,6 +9,22 @@ class Oauth::Client < ApplicationRecord # this long is abandoned (RFC 7591 §5). UNUSED_RETENTION = 30.days + # The grants a client may obtain at the authorization and token endpoints. + # An allowlist, so a client is refused any grant type it doesn't list. + GRANT_TYPES = %w[ authorization_code refresh_token ] + + # Personal access tokens are issued by this first-party client, as bc3's are + # by bc-personal-access, so they're one-off access tokens of the same kind + # as every other. It is provisioned by a migration and on first use, never + # registered: the sweep, which takes only self-registered clients, leaves it + # alone. It is listed for no grant type, so the authorization and token + # endpoints never mint, refresh or exchange its tokens: only + # My::AccessTokensController issues them. Its client_id has a hyphen, which + # no generated client_id (base58) can, so no other client can ever claim it. + PERSONAL_ACCESS_CLIENT_ID = "fizzy-personal-access" + PERSONAL_ACCESS_ATTRIBUTES = { client_id: PERSONAL_ACCESS_CLIENT_ID, name: "Personal Access Tokens", + trusted: true, dynamically_registered: false, redirect_uris: [], scopes: %w[ read write ], token_endpoint_auth_method: "none" } + has_many :access_tokens, class_name: "Identity::AccessToken", foreign_key: :oauth_client_id, inverse_of: :oauth_client, dependent: :destroy has_secure_token :client_id, length: 32 @@ -18,6 +34,10 @@ class Oauth::Client < ApplicationRecord # same order, so no grant can land after the cascade and be stranded. before_destroy :lock_client, prepend: true + # Destroying the issuer would cascade to every client-backed personal access + # token on the install. + before_destroy :refuse_to_destroy_personal_access, prepend: true + validates :name, presence: true, length: { maximum: 255 } validates :client_id, uniqueness: true, allow_nil: true # Registration is open and unauthenticated, so what it stores is bounded: @@ -25,7 +45,7 @@ class Oauth::Client < ApplicationRecord MAXIMUM_REDIRECT_URIS = 10 MAXIMUM_REDIRECT_URI_LENGTH = 2000 - validates :redirect_uris, presence: true, length: { maximum: MAXIMUM_REDIRECT_URIS } + validates :redirect_uris, presence: { unless: :personal_access? }, length: { maximum: MAXIMUM_REDIRECT_URIS } validates :token_endpoint_auth_method, inclusion: { in: AUTH_METHODS } validate :redirect_uris_are_valid @@ -51,9 +71,25 @@ class Oauth::Client < ApplicationRecord # swept for having registered long ago. scope :stale, -> { dynamically_registered.where.missing(:access_tokens).where(updated_at: ...UNUSED_RETENTION.ago) } - def self.cleanup - Oauth.cleanup_step :stale_clients do - stale.find_each(&:destroy_if_still_unused) + scope :personal_access, -> { where client_id: PERSONAL_ACCESS_CLIENT_ID } + + class << self + def cleanup + Oauth.cleanup_step :stale_clients do + stale.find_each(&:destroy_if_still_unused) + end + end + + # A fresh install loads the schema rather than running migrations, so the + # issuer may not exist yet when the first personal access token is made. + # Two first uses can race: the loser either hits the unique index, which + # create_or_find_by! answers by finding on client_id alone, or, when the + # winner committed before its uniqueness validation ran, fails that + # validation, and then finds the winner's row the same way. + def personal_access_issuer + personal_access.take || create_or_find_by!(client_id: PERSONAL_ACCESS_CLIENT_ID) { it.assign_attributes PERSONAL_ACCESS_ATTRIBUTES } + rescue ActiveRecord::RecordInvalid + personal_access.take or raise end end @@ -88,6 +124,18 @@ def redeem(authorization_code, identity:, permission:) end end + def personal_access? + client_id == PERSONAL_ACCESS_CLIENT_ID + end + + def grant_types + personal_access? ? [] : GRANT_TYPES + end + + def allows_grant_type?(grant_type) + grant_type.in?(grant_types) + end + def confidential? token_endpoint_auth_method.in?(SECRET_AUTH_METHODS) end @@ -126,6 +174,15 @@ def lock_client access_tokens.reset end + # Reads the stored client_id: lock_client no longer reloads the row, so an + # unsaved change to the issuer's client_id mustn't let it cascade. + def refuse_to_destroy_personal_access + if client_id_in_database == PERSONAL_ACCESS_CLIENT_ID + errors.add :base, "The personal access token issuer can't be deleted" + throw :abort + end + end + def generate_client_secret self.raw_client_secret ||= Identity::AccessToken.generate_token(:client_secret) self.client_secret_digest = TokenDigester.digest(raw_client_secret) diff --git a/app/models/webhook.rb b/app/models/webhook.rb index 59c3efc85c..cdb0a86509 100644 --- a/app/models/webhook.rb +++ b/app/models/webhook.rb @@ -87,7 +87,7 @@ def for_slack? private def attribute_creation self.creator ||= Current.user - self.created_via ||= Current.access_token&.oauth_client + self.created_via ||= Current.access_token&.connected_app end def validate_url diff --git a/db/migrate/20261015120100_backfill_token_digests.rb b/db/migrate/20261015120100_backfill_token_digests.rb index 483514ddf8..b66da22e47 100644 --- a/db/migrate/20261015120100_backfill_token_digests.rb +++ b/db/migrate/20261015120100_backfill_token_digests.rb @@ -6,8 +6,10 @@ # restart it. # # Personal access tokens keep their plaintext: production still reads it until -# this code reaches it. OAuth access tokens are digest-only, so any plaintext -# one is cleared once digested. Refresh tokens and client secrets are moved +# this code reaches it. That's any token without a client, or with the personal +# access client (CreatePersonalAccessClient), since a rerun may come after +# that client exists. OAuth access tokens are digest-only, so any plaintext one +# is cleared once digested. Refresh tokens and client secrets are moved # wholesale: a refresh token's plaintext is cleared once digested, and the # client secret column is dropped by the next migration. The refresh token # column stays, empty, until it's dropped beside `token`. @@ -31,10 +33,11 @@ class Client < ActiveRecord::Base def up refuse_duplicates AccessToken, :token + personal_client_ids = [ nil, *Client.where(client_id: "fizzy-personal-access").pluck(:id) ] each_undigested AccessToken, :token, :token_digest do |row, digest| AccessToken.where(id: row.id, token_digest: nil).update_all \ - token_digest: digest, token: (row.oauth_client_id ? nil : row.token) + token_digest: digest, token: (row.oauth_client_id.in?(personal_client_ids) ? row.token : nil) end # Each is gone once dropped, so a rerun skips it. diff --git a/db/migrate/20261016120000_create_personal_access_client.rb b/db/migrate/20261016120000_create_personal_access_client.rb new file mode 100644 index 0000000000..d3f7b61470 --- /dev/null +++ b/db/migrate/20261016120000_create_personal_access_client.rb @@ -0,0 +1,44 @@ +# Provisions the first-party client that issues personal access tokens +# (Oauth::Client::PERSONAL_ACCESS_CLIENT_ID), on every install that migrates, +# SaaS or self-hosted. Oauth::Client.personal_access_issuer makes it on first +# use where the schema was loaded instead. It's never registered, so the sweep +# of abandoned registrations passes it by. +# +# Down hands its tokens back to the client-less form, which every version of +# the code reads as personal, and then removes it. Neither direction moves +# updated_at. Run it only once no code with the client is serving: that code +# issues the client's tokens, takes no lock doing so, and makes the client +# again on first use, so nothing down could do would keep up with it. With +# that settled, the order makes down safe to retry: the client stays until +# every token is handed back, so a down that stops partway finds it again. +class CreatePersonalAccessClient < ActiveRecord::Migration[8.2] + disable_ddl_transaction! + + CLIENT_ID = "fizzy-personal-access" + BATCH_SIZE = 500 + + class Client < ActiveRecord::Base + self.table_name = "oauth_clients" + end + + class AccessToken < ActiveRecord::Base + self.table_name = "identity_access_tokens" + end + + def up + unless Client.exists?(client_id: CLIENT_ID) + Client.create! client_id: CLIENT_ID, name: "Personal Access Tokens", trusted: true, dynamically_registered: false, + redirect_uris: [], scopes: %w[ read write ], token_endpoint_auth_method: "none" + end + end + + def down + if client = Client.find_by(client_id: CLIENT_ID) + AccessToken.where(oauth_client_id: client.id).in_batches(of: BATCH_SIZE) do |batch| + batch.update_all oauth_client_id: nil + end + + client.delete + end + end +end diff --git a/db/migrate/20261016120100_backfill_personal_access_client.rb b/db/migrate/20261016120100_backfill_personal_access_client.rb new file mode 100644 index 0000000000..fd1152cc05 --- /dev/null +++ b/db/migrate/20261016120100_backfill_personal_access_client.rb @@ -0,0 +1,39 @@ +# Gives every client-less personal access token the personal access client, +# so each is a one-off access token of that first-party client, as in bc3. +# Only oauth_client_id changes: the token string, its digest, its permission +# and its lack of expiry all stay, and nothing is re-issued. +# +# update_all leaves updated_at alone. It's no lapse clock for a personal token, +# but it's what people see as the token's history, and the backfill isn't use. +# +# Rerunnable: it touches only client-less rows, and its down is a no-op (the +# client's own migration, CreatePersonalAccessClient, undoes the assignment). +# Production code that predates the client keeps writing client-less tokens, +# which every version reads as personal, so +# `db:migrate:redo:primary VERSION=20261016120100` picks them up later. +class BackfillPersonalAccessClient < ActiveRecord::Migration[8.2] + disable_ddl_transaction! + + CLIENT_ID = "fizzy-personal-access" + BATCH_SIZE = 500 + + class Client < ActiveRecord::Base + self.table_name = "oauth_clients" + end + + class AccessToken < ActiveRecord::Base + self.table_name = "identity_access_tokens" + end + + def up + client_id = Client.where(client_id: CLIENT_ID).pick(:id) or raise "#{CLIENT_ID} isn't provisioned: run CreatePersonalAccessClient first" + + AccessToken.where(oauth_client_id: nil).in_batches(of: BATCH_SIZE) do |batch| + batch.update_all oauth_client_id: client_id + end + end + + def down + # CreatePersonalAccessClient's down returns the tokens to the client-less form. + end +end diff --git a/docs/oauth/spec_conformance.md b/docs/oauth/spec_conformance.md index 19c8b748f1..8e46fbf267 100644 --- a/docs/oauth/spec_conformance.md +++ b/docs/oauth/spec_conformance.md @@ -91,6 +91,7 @@ Status key: | Requirement | Status | |---|---| | Tokens are stored as HMAC-SHA256 digests under a server pepper, byte-compatible with bc3's `Oauth::TokenDigester`, and looked up by a unique index on the digest. Raw values are shown once: in the token response, the registration response, or the response that creates a personal access token | **Fixed** #3192 `oauth-token-digests`, step 1 of 3. OAuth access and refresh tokens and client secrets are digest-only, and a refresh retry within the grace window is answered from an encrypted copy of the successor pair that expires with the window. Personal access tokens are digested and, until step 3, still also kept in plaintext, because production code that reads only plaintext shares the database while beta runs this. One written without a digest is found by plaintext once and digested. Step 2 stops reading and writing plaintext; step 3 drops the column. | +| A personal access token is a one-off access token of a first-party client, `fizzy-personal-access` ("Personal Access Tokens"), in the same digest-stored table as every grant, as bc3's are of `bc-personal-access`. That client is provisioned by migration and on first use, is never self-registered (so the registration sweep skips it), and is listed for no grant type, so the authorization and token endpoints refuse it with `unauthorized_client`. A personal token is honored while OAuth is dark, never lapses, gets no expiry or refresh token, keeps the `fizzy_pat_` prefix and is revoked by possession, whichever form it takes. Existing tokens are backfilled in place: same string, reach and non-expiry, nothing re-issued | **Fixed** #3205 `oauth-pats-as-grants` (#3164 steps 1 to 3). Tokens with no client still read as personal, because production code that predates the client keeps writing them until it's deployed. Dropping that branch and making `oauth_client_id` NOT NULL wait until the backfill has run in production. Personal access tokens are still listed under personal access tokens, not Connected Apps, as in bc3 | | Newly issued tokens carry a distinctive prefix naming their kind: `fizzy_pat_` (personal access token), `fizzy_at_` (OAuth access token), `fizzy_rt_` (OAuth refresh token). Tokens issued earlier have no prefix and still work | **Fixed** #3163. bc3 prefixes similarly (`bc_at_`, `bc_rt_`, and PATs as `bc_at_`). Client secrets carry `fizzy_cs_`, as bc3's carry `bc_cs_`. New tokens and secrets carry 44 base58 characters after the prefix, 257 bits, at least bc3's 32 bytes (rubric TK-1); earlier ones are 24 characters and keep working. Nobody has registered with GitHub's secret-scanning partner program, either here or in bc3. | ## Refresh tokens (RFC 6749 §6, OAuth 2.1 §4.3, RFC 9700 §4.14) @@ -112,7 +113,7 @@ Status key: | Missing token answers `invalid_request` JSON (§2.2.1) | **Fixed** #2296 `51155c5e3` | | Confidential clients authenticate, and the token must have been issued to the requesting client (§2.1) | **Fixed** #3082 `e80b5b615` "Authenticate clients at revocation and revoke only their own tokens". A public client names its `client_id`. Unknown tokens, and tokens belonging to another client, get 200 and nothing is revoked. | | A presented client secret that authenticates no client is refused with 400 `invalid_client` (401 + Basic if it came in a Basic header), whatever the token | **Deliberate**, and stricter than bc3: #3082 `c9375f186`, `29a5d118c`. A 200 would tell a client with a mistyped `client_id` that a live grant was dead. bc3 is being aligned to this. | -| A personal access token can be revoked here by whoever holds it. It has no client, so holding it is the credential; client credentials sent alongside must still authenticate | **Fixed** #3082 `98619ab8f` "Revoke personal access tokens at the revocation endpoint again". Revocation works the same for every kind of token, which helps incident response such as revoking a leaked token. It also keeps the endpoint ready if personal access tokens become OAuth tokens, as in bc3. | +| A personal access token can be revoked here by whoever holds it. It's issued by no client that can authenticate (none, or the first-party `fizzy-personal-access` client, which has no secret), so holding it is the credential; client credentials sent alongside must still authenticate | **Fixed** #3082 `98619ab8f` "Revoke personal access tokens at the revocation endpoint again". Revocation works the same for every kind of token, which helps incident response such as revoking a leaked token. It held unchanged when personal access tokens became tokens of the `fizzy-personal-access` client (#3205). | | Revocation is throttled like the token endpoint: 20 per minute per IP, then 429 `slow_down` (RFC 6749 §2.3.1, RFC 7009 §5) | **Fixed** #3082 `11e8e1ad8` "Throttle revocation like the token endpoint". Once revocation authenticated confidential clients, it became a place to guess secrets | ## RFC 8414 / RFC 9728: metadata diff --git a/test/fixtures/oauth/clients.yml b/test/fixtures/oauth/clients.yml index f2aaf446d7..841b858da8 100644 --- a/test/fixtures/oauth/clients.yml +++ b/test/fixtures/oauth/clients.yml @@ -32,3 +32,13 @@ confidential_client: token_endpoint_auth_method: client_secret_post dynamically_registered: true trusted: false + +personal_access: + client_id: fizzy-personal-access + name: Personal Access Tokens + redirect_uris: [] + scopes: + - read + - write + dynamically_registered: false + trusted: true diff --git a/test/integration/personal_access_token_forms_test.rb b/test/integration/personal_access_token_forms_test.rb new file mode 100644 index 0000000000..a32b540b6d --- /dev/null +++ b/test/integration/personal_access_token_forms_test.rb @@ -0,0 +1,246 @@ +require "test_helper" + +# A personal access token takes one of two forms: issued by the personal access +# client, as every token made from now on is, or with no client at all, as every +# token made before that client existed is, and as production code that predates +# it still makes them. Each rule that sets personal tokens apart from OAuth +# grants must hold for both alike, so every test here runs once per form. +class PersonalAccessTokenFormsTest < ActionDispatch::IntegrationTest + include OauthAvailabilityTestHelper + + ENDPOINT = "https://fcm.googleapis.com/fcm/send/cli-device" + + { "client-less" => nil, "client-backed" => :personal_access }.each do |form, issuer| + test "a #{form} personal access token is personal, and no app's" do + token = personal_token(issuer) + + assert token.personal? + assert_not token.oauth? + assert_nil token.connected_app + assert_includes Identity::AccessToken.personal, token + assert_not_includes Identity::AccessToken.oauth, token + end + + test "a #{form} personal access token has the personal prefix, its plaintext, no expiry and no refresh token" do + token = personal_token(issuer) + + assert token.raw_token.start_with?("fizzy_pat_") + assert_equal token.raw_token, token.reload.token + assert_nil token.expires_at + assert_nil token.refresh_token_digest + assert_nil token.raw_refresh_token + end + + test "a #{form} personal access token is honored while OAuth is dark" do + token = personal_token(issuer) + + with_oauth_availability acceptance: false do + get user_path(users(:david)), env: bearer(token), as: :json + end + + assert_response :success + end + + # Shipped clients (fizzy-cli, the native apps) have always had a 401 for a + # read-only personal token's write. Only an app's grant gets the 403. + test "a #{form} read-only personal access token's write is a 401, as shipped clients expect" do + token = personal_token(issuer) + + post boards_path(format: :json), params: { board: { name: "Not made" } }, env: bearer(token) + + assert_response :unauthorized + assert_match 'error="insufficient_scope"', response.headers["WWW-Authenticate"] + assert_match 'scope="write"', response.headers["WWW-Authenticate"] + end + + test "a #{form} personal access token never lapses, however long it sits idle" do + token = personal_token(issuer) + + travel Identity::AccessToken::REFRESH_IDLE_LIMIT + 1.day + assert_not token.lapsed? + assert_not_includes Identity::AccessToken.lapsed, token + + Identity::AccessToken.cleanup + assert Identity::AccessToken.exists?(token.id) + + get user_path(users(:david)), env: bearer(token), as: :json + assert_response :success + end + + test "a #{form} personal access token is revoked by whoever presents it" do + [ {}, { client_id: oauth_clients(:mcp_client).client_id }, { client_id: Oauth::Client::PERSONAL_ACCESS_CLIENT_ID } ].each do |params| + token = personal_token(issuer) + + untenanted { post oauth_revocation_path, params: { token: token.raw_token, **params } } + + assert_response :success + assert_not Identity::AccessToken.exists?(token.id), "expected revocation with #{params}" + end + end + + test "a #{form} personal access token gets no refresh at the token endpoint" do + token = personal_token(issuer) + + untenanted do + post oauth_token_path, params: { grant_type: "refresh_token", refresh_token: token.raw_token, client_id: Oauth::Client::PERSONAL_ACCESS_CLIENT_ID } + end + + assert_response :bad_request + assert_equal "unauthorized_client", response.parsed_body["error"] + + untenanted do + post oauth_token_path, params: { grant_type: "refresh_token", refresh_token: token.raw_token, client_id: oauth_clients(:mcp_client).client_id } + end + + assert_response :bad_request + assert_equal "invalid_grant", response.parsed_body["error"] + assert_equal token.token_digest, token.reload.token_digest + end + + test "a #{form} personal access token manages personal access tokens" do + token = personal_token(issuer, permission: :write) + + untenanted do + post my_access_tokens_path, params: { access_token: { description: "Fizzy CLI", permission: "read" } }, env: bearer(token), as: :json + assert_response :created + + get my_access_tokens_path, env: bearer(token), as: :json + assert_response :success + assert_includes response.parsed_body.pluck("id"), token.id + end + end + + test "a #{form} personal access token is listed with personal access tokens, and not under Connected Apps" do + token = personal_token(issuer, description: "Shell script") + sign_in_as :david + + untenanted { get my_access_tokens_path } + assert_in_body "Shell script" + + get my_connected_apps_path + assert_response :success + assert_not_in_body "Shell script" + assert_not_in_body "Personal Access Tokens" + + delete my_connected_app_path(oauth_clients(:personal_access)) + assert_response :not_found + assert Identity::AccessToken.exists?(token.id) + end + + test "a #{form} personal access token is revoked from the personal access tokens page" do + token = personal_token(issuer) + sign_in_as :david + + untenanted { delete my_access_token_path(token), as: :json } + + assert_response :no_content + assert_not Identity::AccessToken.exists?(token.id) + end + + test "a webhook a #{form} personal access token sets up is attributed to the person alone" do + users(:kevin).update!(role: :admin) + token = personal_token(issuer, identity: :kevin, permission: :write) + + post board_webhooks_path(boards(:writebook)), params: { webhook: { name: "Feed", url: "https://app.example.com/hooks" } }, env: bearer(token), as: :json + + assert_response :created + webhook = Webhook.find(response.parsed_body["id"]) + assert_equal users(:kevin), webhook.creator + assert_nil webhook.created_via + end + + test "a push subscription a #{form} personal access token registers is no app's, and survives the token" do + stub_web_push_dns_resolution + token = personal_token(issuer, identity: :kevin, permission: :write) + + post user_push_subscriptions_path(users(:kevin)), params: { push_subscription: { endpoint: ENDPOINT, p256dh_key: "p256dh", auth_key: "auth" } }, env: bearer(token), as: :json + + assert_response :created + subscription = Push::Subscription.find_by!(endpoint: ENDPOINT) + assert_nil subscription.oauth_client + assert_includes Push::Subscription.deliverable_to(users(:kevin)), subscription + with_oauth_availability acceptance: false do + assert_includes Push::Subscription.deliverable_to(users(:kevin)), subscription + end + + token.destroy + assert Push::Subscription.exists?(subscription.id) + end + + test "a #{form} personal access token reaches an account only while its user is active there" do + token = personal_token(issuer) + + get boards_path, env: bearer(token), as: :json + assert_response :success + + users(:david).deactivate + + get boards_path, env: bearer(token), as: :json + assert_response :forbidden + end + + test "a #{form} personal access token without a digest is found by its plaintext and digested" do + token = personal_token(issuer) + token.update_columns token_digest: nil + + assert_equal token, Identity::AccessToken.find_honored(token.raw_token) + assert_equal TokenDigester.digest(token.raw_token), token.reload.token_digest + end + + test "a #{form} personal access token without a digest authenticates outside the primary datacenter, undigested" do + token = personal_token(issuer) + token.update_columns token_digest: nil + Deployment::DatabaseResolver.stubs(:in_primary_datacenter?).returns(false) + + assert_equal token, Identity::AccessToken.find_honored(token.raw_token) + assert_nil token.reload.token_digest + end + + test "issuing or revoking a #{form} personal access token leaves the issuer's retention clock alone" do + assert_no_changes -> { oauth_clients(:personal_access).reload.updated_at } do + personal_token(issuer).destroy + end + end + end + + test "a personal access token made from the page is issued by the personal access client" do + sign_in_as :david + + untenanted { post my_access_tokens_path, params: { access_token: { description: "Fizzy CLI", permission: "write" } }, as: :json } + + assert_response :created + assert_equal %w[ created_at description id permission token ], response.parsed_body.keys.sort + assert_equal oauth_clients(:personal_access), Identity::AccessToken.find(response.parsed_body["id"]).oauth_client + end + + test "the authorization endpoint refuses the personal access client" do + sign_in_as :david + + untenanted do + get new_oauth_authorization_path, params: { client_id: Oauth::Client::PERSONAL_ACCESS_CLIENT_ID, response_type: "code", + redirect_uri: "http://127.0.0.1:8888/callback", code_challenge: "challenge", code_challenge_method: "S256", state: "state" } + end + + assert_response :bad_request + assert_equal "unauthorized_client", response.parsed_body["error"] + end + + test "the token endpoint refuses the personal access client the code grant" do + untenanted do + post oauth_token_path, params: { grant_type: "authorization_code", code: "code", code_verifier: "verifier", + redirect_uri: "http://127.0.0.1:8888/callback", client_id: Oauth::Client::PERSONAL_ACCESS_CLIENT_ID } + end + + assert_response :bad_request + assert_equal "unauthorized_client", response.parsed_body["error"] + end + + private + def personal_token(issuer, identity: :david, **attributes) + identities(identity).access_tokens.create!(oauth_client: issuer && oauth_clients(issuer), permission: :read, **attributes) + end + + def bearer(token) + { "HTTP_AUTHORIZATION" => "Bearer #{token.raw_token}" } + end +end diff --git a/test/migrations/personal_access_client_test.rb b/test/migrations/personal_access_client_test.rb new file mode 100644 index 0000000000..1ed0765344 --- /dev/null +++ b/test/migrations/personal_access_client_test.rb @@ -0,0 +1,114 @@ +require "test_helper" +require Rails.root.join("db/migrate/20261015120100_backfill_token_digests") +require Rails.root.join("db/migrate/20261016120000_create_personal_access_client") +require Rails.root.join("db/migrate/20261016120100_backfill_personal_access_client") + +class PersonalAccessClientMigrationTest < ActiveSupport::TestCase + setup do + Oauth::Client.personal_access.delete_all + + @personal = identities(:david).access_tokens.create!(description: "Before the client", permission: :write) + @personal.update_columns updated_at: 200.days.ago.round + @grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) + end + + test "provisions the issuer once" do + assert_difference("Oauth::Client.count", +1) { migrate CreatePersonalAccessClient, :up } + assert_no_difference("Oauth::Client.count") { migrate CreatePersonalAccessClient, :up } + + issuer = Oauth::Client.personal_access.sole + assert_equal "Personal Access Tokens", issuer.name + assert issuer.trusted? + assert_not issuer.dynamically_registered? + assert_equal "none", issuer.token_endpoint_auth_method + assert_empty issuer.grant_types + assert issuer.valid? + end + + test "the backfill gives client-less tokens the issuer, keeping their string, digest, reach, non-expiry and updated_at" do + kept = %w[ token token_digest refresh_token_digest permission expires_at description updated_at ] + before = @personal.reload.attributes.slice(*kept) + migrate CreatePersonalAccessClient, :up + + assert_no_changes -> { @grant.reload.attributes } do + migrate BackfillPersonalAccessClient, :up + end + + @personal.reload + assert_equal Oauth::Client.personal_access.sole, @personal.oauth_client + assert_equal before, @personal.attributes.slice(*kept) + assert_nil @personal.expires_at + assert @personal.personal? + + assert_equal @personal, Identity::AccessToken.find_honored(before["token"]) + assert @personal.allows?("POST") + end + + test "the backfill reruns over tokens written client-less since" do + migrate CreatePersonalAccessClient, :up + migrate BackfillPersonalAccessClient, :up + + later = identities(:kevin).access_tokens.create!(description: "From older code") + assert_nil later.oauth_client_id + + migrate BackfillPersonalAccessClient, :up + assert_equal Oauth::Client.personal_access.sole, later.reload.oauth_client + end + + test "the backfill refuses to run before the issuer exists" do + assert_raises(RuntimeError) { migrate BackfillPersonalAccessClient, :up } + assert_nil @personal.reload.oauth_client_id + end + + test "down hands the tokens back to the client-less form and removes the issuer" do + migrate CreatePersonalAccessClient, :up + migrate BackfillPersonalAccessClient, :up + + assert_no_changes -> { @personal.reload.updated_at } do + migrate BackfillPersonalAccessClient, :down + migrate CreatePersonalAccessClient, :down + end + + assert_nil @personal.reload.oauth_client_id + assert @personal.personal? + assert_equal oauth_clients(:mcp_client), @grant.reload.oauth_client + assert_not Oauth::Client.personal_access.exists? + end + + # BackfillTokenDigests is rerun to digest tokens production writes in + # plaintext only. Once those have the issuer, the rerun must still treat them + # as personal and keep the plaintext production reads. + test "a down that stops partway finds the issuer again and finishes" do + migrate CreatePersonalAccessClient, :up + migrate BackfillPersonalAccessClient, :up + second = identities(:kevin).access_tokens.create!(oauth_client: Oauth::Client.personal_access.sole) + ActiveRecord::Relation.any_instance.stubs(:update_all).raises(ActiveRecord::StatementInvalid, "interrupted") + assert_raises(ActiveRecord::StatementInvalid) { migrate CreatePersonalAccessClient, :down } + ActiveRecord::Relation.any_instance.unstub(:update_all) + + assert Oauth::Client.personal_access.exists? + + migrate CreatePersonalAccessClient, :down + + assert_not Oauth::Client.personal_access.exists? + assert_nil @personal.reload.oauth_client_id + assert_nil second.reload.oauth_client_id + end + + test "rerunning the digest backfill keeps a client-backed personal token's plaintext" do + migrate CreatePersonalAccessClient, :up + migrate BackfillPersonalAccessClient, :up + @personal.update_columns token: "legacyPersonalToken0123456789", token_digest: nil + + migrate BackfillTokenDigests, :up + + @personal.reload + assert_equal "legacyPersonalToken0123456789", @personal.token + assert_equal TokenDigester.digest("legacyPersonalToken0123456789"), @personal.token_digest + end + + private + def migrate(migration, direction) + ActiveRecord::Migration.suppress_messages { migration.new.migrate(direction) } + end +end diff --git a/test/models/oauth/client_test.rb b/test/models/oauth/client_test.rb index fdbae59c0d..6c177e537c 100644 --- a/test/models/oauth/client_test.rb +++ b/test/models/oauth/client_test.rb @@ -445,6 +445,91 @@ class Oauth::ClientTest < ActiveSupport::TestCase assert_not Identity::AccessToken.exists?(grant.id) end + test "the personal access token issuer is the provisioned client" do + assert_equal oauth_clients(:personal_access), Oauth::Client.personal_access_issuer + end + + # A fresh install loads the schema instead of migrating. + test "the personal access token issuer is provisioned on first use where it's missing" do + Oauth::Client.personal_access.delete_all + + issuer = assert_difference("Oauth::Client.count", +1) { Oauth::Client.personal_access_issuer } + + assert issuer.personal_access? + assert issuer.trusted? + assert_not issuer.dynamically_registered? + assert_not issuer.confidential? + assert_empty issuer.redirect_uris + assert_equal issuer, Oauth::Client.personal_access_issuer + end + + # Another first use committed the issuer after this one looked and before its + # uniqueness validation ran. + test "the personal access token issuer is found when a concurrent first use provisioned it" do + issuer = oauth_clients(:personal_access) + Oauth::Client.stubs(:personal_access).returns(Oauth::Client.none).then.returns(Oauth::Client.where(id: issuer.id)) + + assert_no_difference("Oauth::Client.count") do + assert_equal issuer, Oauth::Client.personal_access_issuer + end + end + + test "the personal access token issuer is found when a concurrent first use won the unique index" do + issuer = oauth_clients(:personal_access) + Oauth::Client.stubs(:personal_access).returns(Oauth::Client.none) + Oauth::Client.any_instance.stubs(:valid?).returns(true) + + assert_no_difference("Oauth::Client.count") do + assert_equal issuer, Oauth::Client.personal_access_issuer + end + end + + test "the personal access token issuer may obtain no grant, and every other client the code and refresh grants" do + assert_empty oauth_clients(:personal_access).grant_types + assert_not oauth_clients(:personal_access).allows_grant_type?("authorization_code") + assert_not oauth_clients(:personal_access).allows_grant_type?("refresh_token") + + assert oauth_clients(:mcp_client).allows_grant_type?("authorization_code") + assert oauth_clients(:mcp_client).allows_grant_type?("refresh_token") + assert_not oauth_clients(:mcp_client).allows_grant_type?("client_credentials") + end + + test "the personal access token issuer has no redirect to send a code to" do + assert_not oauth_clients(:personal_access).allows_redirect?("http://127.0.0.1:8888/callback") + end + + test "only the personal access token issuer goes without redirect URIs" do + client = Oauth::Client.new(name: "Unregistered", client_id: "fizzy-personal-access-impostor", redirect_uris: []) + + assert_not client.valid? + assert client.errors[:redirect_uris].any? + end + + test "the personal access token issuer can't be destroyed, so its tokens can't cascade away with it" do + token = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:personal_access)) + + assert_not oauth_clients(:personal_access).destroy + assert Oauth::Client.exists?(oauth_clients(:personal_access).id) + assert Identity::AccessToken.exists?(token.id) + end + + test "the personal access token issuer can't be destroyed with unsaved changes either" do + issuer = oauth_clients(:personal_access) + token = identities(:david).access_tokens.create!(oauth_client: issuer) + issuer.assign_attributes client_id: "renamed", name: "Renamed" + + assert_not issuer.destroy + assert Oauth::Client.exists?(issuer.id) + assert Identity::AccessToken.exists?(token.id) + end + + test "cleanup never sweeps the personal access token issuer, however long it goes unused" do + oauth_clients(:personal_access).update_columns updated_at: 1.year.ago + + Oauth::Client.cleanup + + assert Oauth::Client.exists?(oauth_clients(:personal_access).id) + end test "destroying a client with unsaved changes still destroys it and its grants" do client = Oauth::Client.create!(name: "Dirty", redirect_uris: %w[ http://127.0.0.1:8888/callback ])