diff --git a/app/controllers/oauth/authorizations_controller.rb b/app/controllers/oauth/authorizations_controller.rb index a7bb3c6c3..803ad476f 100644 --- a/app/controllers/oauth/authorizations_controller.rb +++ b/app/controllers/oauth/authorizations_controller.rb @@ -23,6 +23,8 @@ class Oauth::AuthorizationsController < Oauth::BaseController before_action :require_authentication disallow_oauth_grants + before_action :require_admitted_identity, unless: :denial? + before_action :set_client before_action :validate_redirect_uri before_action :validate_single_valued_parameters @@ -82,6 +84,17 @@ def denial? request.post? && params[:error] == "access_denied" end + # While OAuth is staff only, nobody else is asked to consent. They're told + # why, rather than bounced back to an app that can't say. + def require_admitted_identity + unless Oauth::Availability.admits?(Current.identity) + respond_to do |format| + format.html { render :staff_only, status: :forbidden } + format.json { render_oauth_error "access_denied", "OAuth apps are limited to 37signals staff for now", status: :forbidden } + end + end + end + # Runs before authentication, so a person already signed in is told apart # by their session cookie. The URL goes into the cookie-stored session, # which overflows (a 500) past 4KB once encrypted, so a request too long to diff --git a/app/controllers/oauth/tokens_controller.rb b/app/controllers/oauth/tokens_controller.rb index e90c85bc6..b56cc836b 100644 --- a/app/controllers/oauth/tokens_controller.rb +++ b/app/controllers/oauth/tokens_controller.rb @@ -38,11 +38,16 @@ class Oauth::TokensController < Oauth::BaseController with_options if: :authorization_code_grant? do before_action :validate_pkce before_action :validate_redirect_uri + before_action :answer_code_replay before_action :set_identity + before_action :require_admitted_identity before_action :validate_code_resource before_action :validate_bound_account end + # A replay that can't be a retry earns its revocation whoever owns the grant, + # so it isn't stopped here: it issues nothing either way. + before_action :require_admitted_grant_owner, unless: -> { authorization_code_grant? || unretryable_replay? } before_action :validate_refresh_resource, unless: -> { authorization_code_grant? || unretryable_replay? } before_action :validate_refresh_bound_account, unless: -> { authorization_code_grant? || unretryable_replay? } @@ -155,6 +160,30 @@ def set_identity end end + # While OAuth is staff only, a code or refresh token mints nothing for + # someone who isn't staff, even one issued while they were. + # A code presented again, by the client it was issued to with its verifier, + # is answered as a replay before anything about the person or account is + # weighed: the revocation it earns can't hinge on what has changed since. + def answer_code_replay + if Identity::AccessToken.code_redeemed?(@auth_code) + Identity::AccessToken.answer_code_replay(@auth_code, client: @client) + oauth_error "invalid_grant", "Authorization code already used" + end + end + + def require_admitted_identity + unless Oauth::Availability.admits?(@identity) + oauth_error "invalid_grant", "OAuth apps are limited to 37signals staff for now" + end + end + + def require_admitted_grant_owner + unless @access_token.personal? || Oauth::Availability.admits?(@access_token.identity) + oauth_error "invalid_grant", "OAuth apps are limited to 37signals staff for now" + end + end + # RFC 8707 ยง2.2: a resource on redemption must name what the code was # issued for. A code for the hosted MCP server answers to that resource # alone; any other code, to Fizzy's own API. Neither is widened or moved diff --git a/app/models/identity/access_token.rb b/app/models/identity/access_token.rb index 89acbbeb3..9fa313858 100644 --- a/app/models/identity/access_token.rb +++ b/app/models/identity/access_token.rb @@ -206,13 +206,28 @@ def redeem(authorization_code, **attributes) create! authorization_code_jti: authorization_code.jti, **attributes end rescue ActiveRecord::RecordNotUnique - replayed = where(authorization_code_jti: authorization_code.jti).each { |grant| grant.revoke(reason: "code_replayed", actor: "system") } - # The code names its identity even when no grant is left to. - Oauth::Audit.record "code.replayed", grant: replayed.first, client: attributes[:oauth_client], identity_id: authorization_code.identity_id, actor: "client" - remember_redeemed authorization_code + answer_code_replay authorization_code, client: attributes[:oauth_client] nil end + # Whether a code has been redeemed before: recorded, or, for one redeemed + # before redemptions were recorded, stamped on the grant it minted. + def code_redeemed?(authorization_code) + Oauth::RedeemedAuthorizationCode.exists?(jti: authorization_code.jti) || unscoped.exists?(authorization_code_jti: authorization_code.jti) + end + + # A replayed code revokes the grant its first redemption issued, whoever + # presents it and whatever has changed since, and mints nothing. + # The revocations report as they commit, after the replay that caused them. + def answer_code_replay(authorization_code, client:) + transaction do + replayed = unscoped.where(authorization_code_jti: authorization_code.jti).each { |grant| grant.revoke(reason: "code_replayed", actor: "system") } + # The code names its identity even when no grant is left to. + Oauth::Audit.record "code.replayed", grant: replayed.first, client: client, identity_id: authorization_code.identity_id, actor: "client" + remember_redeemed authorization_code + end + end + private # A code redeemed before redemptions were recorded has a grant but no # record, and the failed transaction above rolled back the one it tried @@ -268,7 +283,7 @@ def find_and_digest_undigested(raw) # write once that grant can't: a refresh that narrows the grant to read # narrows the tokens delegated from it too. def honored? - honored_client? && (!delegated? || honored_delegation?) + honored_client? && honored_identity? && (!delegated? || honored_delegation?) end def personal? @@ -390,6 +405,14 @@ def honored_delegation? grant.present? && grant.honored_client? && (read? || grant.write?) end + # While OAuth is staff only, an OAuth token works only for staff, so a + # grant stops working once its owner isn't. A personal access token isn't + # OAuth and always works. The switch is checked first, so an install that + # isn't staff only doesn't load each token's client to tell. + def honored_identity? + !Oauth::Availability.staff_only? || personal? || identity&.staff? == true + end + def honored_client? oauth_client_id.nil? || Oauth::Availability.acceptance_enabled? || personal? || Oauth::Availability.acceptance_enabled?(oauth_client&.client_id) end diff --git a/app/models/oauth/availability.rb b/app/models/oauth/availability.rb index e1129d7b7..a642d6dc3 100644 --- a/app/models/oauth/availability.rb +++ b/app/models/oauth/availability.rb @@ -35,6 +35,15 @@ # # An incident pause must therefore empty pilot_client_ids as well as throwing # the switch, or listed clients keep minting through it. +# +# == Staff only +# +# The switches above choose clients. staff_only chooses people: while it's on, +# only 37signals staff (Identity#staff?) can connect an app, redeem a code, +# refresh, or use an OAuth token, whichever client asks. Someone who stops +# being staff loses the access they granted on their next request. Personal +# access tokens are not OAuth credentials, so it doesn't touch them, and +# management stays open to everyone, so anyone can still shed a grant. module Oauth::Availability extend self @@ -53,6 +62,15 @@ def metadata_documents_enabled?(client_id = nil) issuance_enabled?(client_id) && config.cimd_enabled == true end + def staff_only? + config.staff_only == true + end + + # Whether OAuth is open to this person: everyone, unless staff_only. + def admits?(identity) + !staff_only? || identity&.staff? == true + end + # The clients whose tokens are honored, as a relation, for code that weighs # many grants at once rather than one presented token. def accepted_clients diff --git a/app/models/push/subscription.rb b/app/models/push/subscription.rb index b368cf316..a10109aba 100644 --- a/app/models/push/subscription.rb +++ b/app/models/push/subscription.rb @@ -23,16 +23,18 @@ class Push::Subscription < ApplicationRecord # moving to another identity, a claim racing the last grant's end), where # the cleanup on grant destroy only finds what the identity still reaches. # The grant must also be one Fizzy honors (Oauth::Availability): while - # OAuth is dark, an app whose tokens are refused gets no notifications - # either. + # OAuth is dark, or staff only and the user isn't staff, an app whose tokens + # are refused gets no notifications either. # # An account-bound grant reaches only its own account, so only it, or a grant # bound to no account, keeps a subscription in that account delivering: once # the app's grant for one account ends, a grant it holds for another account # doesn't carry that account's notifications. scope :deliverable_to, ->(user) do + grants = Identity::AccessToken.grants.unlapsed.where(identity_id: user.identity_id, account_id: [ nil, user.account_id ], oauth_client: Oauth::Availability.accepted_clients) + where(oauth_client_id: nil).or \ - where(oauth_client_id: Identity::AccessToken.grants.unlapsed.where(identity_id: user.identity_id, account_id: [ nil, user.account_id ], oauth_client: Oauth::Availability.accepted_clients).select(:oauth_client_id)) + where(oauth_client_id: (!Oauth::Availability.staff_only? || Oauth::Availability.admits?(user.identity) ? grants : grants.none).select(:oauth_client_id)) end validates :endpoint, presence: true diff --git a/app/views/oauth/authorizations/staff_only.html.erb b/app/views/oauth/authorizations/staff_only.html.erb new file mode 100644 index 000000000..064904956 --- /dev/null +++ b/app/views/oauth/authorizations/staff_only.html.erb @@ -0,0 +1,11 @@ +
+
+

Not available yet

+ +

+ Only 37signals staff can connect apps to Fizzy for now. +

+ + <%= link_to "Go back", root_path, class: "btn btn--outline center txt-medium" %> +
+
diff --git a/config/environments/production.rb b/config/environments/production.rb index dea1e20c2..e0b1f6c43 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -128,6 +128,9 @@ config.x.oauth.acceptance_enabled = ENV["OAUTH_ACCEPTANCE_ENABLED"] == "true" config.x.oauth.issuance_enabled = ENV["OAUTH_ISSUANCE_ENABLED"] != "false" config.x.oauth.pilot_client_ids = ENV.fetch("OAUTH_PILOT_CLIENT_IDS", "").split(",").map(&:strip).compact_blank + # OAUTH_STAFF_ONLY=true limits OAuth to 37signals staff, whichever client + # asks; personal access tokens are unaffected (Oauth::Availability). + config.x.oauth.staff_only = ENV["OAUTH_STAFF_ONLY"] == "true" # Client ID Metadata Documents are off unless OAUTH_CIMD_ENABLED is "true", # and then fetched only from the hosts OAUTH_CIMD_ALLOWED_HOSTS names diff --git a/saas/config/deploy.beta.yml b/saas/config/deploy.beta.yml index 2019c29e8..d0e0e46f0 100644 --- a/saas/config/deploy.beta.yml +++ b/saas/config/deploy.beta.yml @@ -51,6 +51,7 @@ env: CACHE_NAMESPACE: <%= @beta_number %> RAILS_ENV: beta RAILS_LOG_LEVEL: fatal # suppress unstructured log lines + OAUTH_STAFF_ONLY: "true" # OAuth is for 37signals staff until we open it up MYSQL_DATABASE_HOST: fizzy-mysql-primary MYSQL_DATABASE_REPLICA_HOST: fizzy-mysql-replica MYSQL_SOLID_CABLE_HOST: fizzy-mysql-primary diff --git a/saas/config/deploy.production.yml b/saas/config/deploy.production.yml index 7ad590b18..e2b5448ab 100644 --- a/saas/config/deploy.production.yml +++ b/saas/config/deploy.production.yml @@ -30,6 +30,7 @@ env: clear: RAILS_ENV: production RAILS_LOG_LEVEL: fatal # suppress unstructured log lines + OAUTH_STAFF_ONLY: "true" # OAuth is for 37signals staff until we open it up MYSQL_DATABASE_HOST: fizzy-mysql-primary MYSQL_DATABASE_REPLICA_HOST: fizzy-mysql-replica MYSQL_SOLID_CABLE_HOST: fizzy-mysql-primary diff --git a/saas/config/deploy.staging.yml b/saas/config/deploy.staging.yml index 6042ac05a..7fc517f14 100644 --- a/saas/config/deploy.staging.yml +++ b/saas/config/deploy.staging.yml @@ -30,6 +30,7 @@ env: clear: RAILS_ENV: staging RAILS_LOG_LEVEL: fatal # suppress unstructured log lines + OAUTH_STAFF_ONLY: "true" # OAuth is for 37signals staff until we open it up MYSQL_DATABASE_HOST: fizzy-staging-mysql-primary MYSQL_DATABASE_REPLICA_HOST: fizzy-staging-mysql-replica MYSQL_SOLID_CABLE_HOST: fizzy-staging-mysql-primary diff --git a/test/integration/oauth_delegate_resources_test.rb b/test/integration/oauth_delegate_resources_test.rb index e825ce189..77bb66471 100644 --- a/test/integration/oauth_delegate_resources_test.rb +++ b/test/integration/oauth_delegate_resources_test.rb @@ -129,6 +129,29 @@ class OauthDelegateResourcesTest < ActionDispatch::IntegrationTest end end + test "an app's push subscription stops delivering while OAuth is staff only and the user isn't staff, alongside the browser's" do + subscribe_push bearer(@grant) + users(:kevin).push_subscriptions.create!(push_params(BROWSER)) + + with_oauth_availability staff_only: true do + assert_equal [ BROWSER, ENDPOINT ].sort, delivered_endpoints.sort + + identities(:kevin).update!(staff: false) + assert_equal [ BROWSER ], delivered_endpoints + end + end + + test "delivering push looks up no identity while OAuth isn't staff only" do + subscribe_push bearer(@grant) + user = User.find(users(:kevin).id) + + with_oauth_availability staff_only: false do + Push::Subscription.deliverable_to(user).load + end + + assert_not user.association(:identity).loaded? + end + test "an app's push subscription stops delivering when the user leaves the account" do subscribe_push bearer(@grant) users(:kevin).push_subscriptions.create!(push_params(BROWSER)) diff --git a/test/integration/oauth_staff_only_test.rb b/test/integration/oauth_staff_only_test.rb new file mode 100644 index 000000000..d59684fa8 --- /dev/null +++ b/test/integration/oauth_staff_only_test.rb @@ -0,0 +1,223 @@ +require "test_helper" + +# While OAuth is staff only (Oauth::Availability), only 37signals staff can +# connect an app or use what they connected, whichever client asks. +class OauthStaffOnlyTest < ActionDispatch::IntegrationTest + include OauthAvailabilityTestHelper + + CODE_VERIFIER = "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk" + + test "staff see the consent screen and connect" do + with_oauth_availability staff_only: true do + sign_in_as :david + get_consent_screen + assert_response :success + assert_select "form[action$=?]", "/oauth/authorization" + + post_consent + assert_response :redirect + assert_match %r{\Ahttp://127\.0\.0\.1:8888/callback\?code=}, response.location + end + end + + test "anyone else is told OAuth is staff only, and gets no code" do + with_oauth_availability staff_only: true do + sign_in_as :jz + get_consent_screen + assert_response :forbidden + assert_match "Only 37signals staff can connect apps", response.body + + post_consent + assert_response :forbidden + assert_nil response.location + end + end + + test "anyone else can still decline" do + with_oauth_availability staff_only: true do + sign_in_as :jz + post_consent error: "access_denied" + + assert_response :redirect + assert_match "error=access_denied", response.location + end + end + + test "everyone connects when it's off" do + sign_in_as :jz + get_consent_screen + + assert_response :success + end + + test "a code issued to someone no longer staff mints nothing" do + code = authorization_code_for(identities(:david)) + identities(:david).update!(staff: false) + + with_oauth_availability staff_only: true do + redeem code + end + + assert_response :bad_request + assert_equal "invalid_grant", response.parsed_body["error"] + end + + test "a code replayed once its owner isn't staff still revokes the grant it minted" do + code = authorization_code_for(identities(:david)) + + with_oauth_availability staff_only: true do + redeem code + assert_response :success + grant = Identity::AccessToken.find_by_raw_token(response.parsed_body["access_token"]) + + identities(:david).update!(staff: false) + redeem code + + assert_response :bad_request + assert_equal "invalid_grant", response.parsed_body["error"] + assert_not Identity::AccessToken.exists?(grant.id), "a replay revokes the grant whoever owns it" + end + end + + test "anyone else asking for consent as JSON is refused as JSON" do + with_oauth_availability staff_only: true do + sign_in_as :jz + untenanted { get new_oauth_authorization_path(format: :json), params: consent_params } + + assert_response :forbidden + assert_equal "access_denied", response.parsed_body["error"] + end + end + + test "a staff code redeems" do + with_oauth_availability staff_only: true do + redeem authorization_code_for(identities(:david)) + end + + assert_response :success + end + + test "a grant stops working, and refreshing, once its owner isn't staff" do + grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) + + with_oauth_availability staff_only: true do + get user_path(users(:david)), env: { "HTTP_AUTHORIZATION" => "Bearer #{grant.raw_token}" }, as: :json + assert_response :success + + identities(:david).update!(staff: false) + + get user_path(users(:david)), env: { "HTTP_AUTHORIZATION" => "Bearer #{grant.raw_token}" }, as: :json + assert_response :unauthorized + assert_match 'error="invalid_token"', response.headers["WWW-Authenticate"] + + refresh grant + assert_response :bad_request + assert_equal "invalid_grant", response.parsed_body["error"] + end + + assert grant.reload.persisted?, "a refused refresh leaves the grant to its owner to revoke" + end + + test "a replayed refresh token still revokes the grant once its owner isn't staff" do + grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) + presented = grant.raw_refresh_token + + with_oauth_availability staff_only: true do + refresh grant + assert_response :success + + identities(:david).update!(staff: false) + travel Oauth::RetiredRefreshToken::GRACE + 1.second + + untenanted do + post oauth_token_path, params: { grant_type: "refresh_token", refresh_token: presented, client_id: oauth_clients(:mcp_client).client_id }, as: :json + end + assert_response :bad_request + end + + assert_not Identity::AccessToken.exists?(grant.id), "a replay revokes the grant whoever owns it" + end + + test "a staff grant refreshes" do + grant = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:mcp_client)) + + with_oauth_availability staff_only: true do + refresh grant + end + + assert_response :success + end + + test "personal access tokens work for anyone" do + identities(:david).update!(staff: false) + + with_oauth_availability staff_only: true do + get user_path(users(:david)), env: { "HTTP_AUTHORIZATION" => "Bearer #{identity_access_tokens(:davids_api_token).token}" }, as: :json + end + + assert_response :success + end + + test "personal access tokens issued by the first-party client work for anyone" do + token = identities(:david).access_tokens.create!(oauth_client: oauth_clients(:personal_access), permission: :read, description: "Script") + identities(:david).update!(staff: false) + + with_oauth_availability staff_only: true do + get user_path(users(:david)), env: { "HTTP_AUTHORIZATION" => "Bearer #{token.raw_token}" }, as: :json + end + + assert_response :success + end + + private + def get_consent_screen(**params) + untenanted { get new_oauth_authorization_path, params: consent_params(**params) } + end + + def post_consent(**params) + untenanted { post oauth_authorization_path, params: consent_params(**params) } + end + + def consent_params(**overrides) + { + client_id: oauth_clients(:mcp_client).client_id, + redirect_uri: "http://127.0.0.1:8888/callback", + response_type: "code", + code_challenge: code_challenge, + code_challenge_method: "S256", + scope: "read", + state: "xyz123" + }.merge(overrides) + end + + def code_challenge + Base64.urlsafe_encode64(Digest::SHA256.digest(CODE_VERIFIER), padding: false) + end + + def authorization_code_for(identity) + Oauth::AuthorizationCode.generate \ + client_id: oauth_clients(:mcp_client).client_id, + identity_id: identity.id, + code_challenge: code_challenge, + redirect_uri: "http://127.0.0.1:8888/callback", + scope: "read" + end + + def redeem(code) + untenanted do + post oauth_token_path, params: { + grant_type: "authorization_code", + client_id: oauth_clients(:mcp_client).client_id, + code: code, + redirect_uri: "http://127.0.0.1:8888/callback", + code_verifier: CODE_VERIFIER + }, as: :json + end + end + + def refresh(grant) + untenanted do + post oauth_token_path, params: { grant_type: "refresh_token", refresh_token: grant.raw_refresh_token, client_id: oauth_clients(:mcp_client).client_id }, as: :json + end + end +end diff --git a/test/integration/oauth_token_exchange_test.rb b/test/integration/oauth_token_exchange_test.rb index bf5971b5d..09d92af99 100644 --- a/test/integration/oauth_token_exchange_test.rb +++ b/test/integration/oauth_token_exchange_test.rb @@ -443,6 +443,22 @@ class OauthTokenExchangeTest < ActionDispatch::IntegrationTest assert_response :success end + test "while OAuth is staff only, a grant whose owner isn't staff is not exchanged, and its delegated tokens stop working" do + with_oauth_availability staff_only: true do + exchange + assert_response :success + delegated = response.parsed_body["access_token"] + + identities(:david).update!(staff: false) + + get user_path(users(:david)), env: { "HTTP_AUTHORIZATION" => "Bearer #{delegated}" }, as: :json + assert_response :unauthorized + + exchange + assert_invalid_grant "Subject token is not valid" + end + end + private def audienced_grant(audience: RESOURCE) identities(:david).access_tokens.create! \ diff --git a/test/models/oauth/availability_test.rb b/test/models/oauth/availability_test.rb index 33cdd931e..dc351b1a1 100644 --- a/test/models/oauth/availability_test.rb +++ b/test/models/oauth/availability_test.rb @@ -50,4 +50,21 @@ class Oauth::AvailabilityTest < ActiveSupport::TestCase assert_not Oauth::Availability.issuance_enabled?("") end end + + test "admits everyone unless staff only" do + assert Oauth::Availability.admits?(identities(:jz)) + assert Oauth::Availability.admits?(nil) + + with_oauth_availability staff_only: true do + assert Oauth::Availability.admits?(identities(:david)) + assert_not Oauth::Availability.admits?(identities(:jz)) + assert_not Oauth::Availability.admits?(nil) + end + end + + test "staff only must be exactly true" do + with_oauth_availability staff_only: "true" do + assert_not Oauth::Availability.staff_only? + end + end end diff --git a/test/test_helpers/oauth_availability_test_helper.rb b/test/test_helpers/oauth_availability_test_helper.rb index c32c95f31..e41b0c360 100644 --- a/test/test_helpers/oauth_availability_test_helper.rb +++ b/test/test_helpers/oauth_availability_test_helper.rb @@ -1,14 +1,15 @@ module OauthAvailabilityTestHelper - def with_oauth_availability(acceptance: true, issuance: true, pilot_client_ids: []) + def with_oauth_availability(acceptance: true, issuance: true, pilot_client_ids: [], staff_only: false) config = Rails.application.config.x.oauth - saved = config.to_h.slice(:acceptance_enabled, :issuance_enabled, :pilot_client_ids) + saved = config.to_h.slice(:acceptance_enabled, :issuance_enabled, :pilot_client_ids, :staff_only) config.acceptance_enabled = acceptance config.issuance_enabled = issuance config.pilot_client_ids = pilot_client_ids + config.staff_only = staff_only yield ensure - %i[ acceptance_enabled issuance_enabled pilot_client_ids ].each { |key| config[key] = saved[key] } + %i[ acceptance_enabled issuance_enabled pilot_client_ids staff_only ].each { |key| config[key] = saved[key] } end def with_dcr_allowed_https_hosts(*hosts)