diff --git a/app/assets/stylesheets/oauth.css b/app/assets/stylesheets/oauth.css new file mode 100644 index 0000000000..e3a06d4f12 --- /dev/null +++ b/app/assets/stylesheets/oauth.css @@ -0,0 +1,125 @@ +/* Authorizing an app (oauth/authorizations) and the apps you've connected (my/connected_apps) */ + +@layer components { + .oauth-consent { + display: flex; + flex-direction: column; + gap: var(--block-space); + } + + .oauth-client__name { + overflow-wrap: anywhere; + } + + .oauth-client__self-name { + overflow-wrap: anywhere; + } + + /* A long email still leaves room for "Not you?": the row wraps, and the address breaks */ + .oauth-consent__identity { + flex-wrap: wrap; + + > span { + min-inline-size: 0; + overflow-wrap: anywhere; + } + } + + .oauth-client__warning { + background-color: var(--color-highlight); + border-radius: 0.5em; + padding: var(--block-space-half) var(--inline-space); + } + + .oauth-access { + border: 0; + margin: 0; + padding: 0; + } + + .oauth-access__legend { + margin-block-end: var(--block-space-half); + padding: 0; + } + + .oauth-access__choice { + align-items: start; + + /* A long account name breaks inside its card rather than widening it */ + > span { + min-inline-size: 0; + overflow-wrap: anywhere; + } + + border: var(--border); + border-radius: 0.5em; + cursor: pointer; + display: flex; + gap: var(--inline-space); + padding: var(--block-space-half) var(--inline-space); + + &:has(:checked) { + border-color: var(--color-ink); + } + + &:has(:focus-visible) { + outline: 2px solid var(--color-ink); + outline-offset: 2px; + } + } + + .oauth-access__radio { + accent-color: var(--color-ink); + flex-shrink: 0; + margin-block-start: 0.2em; + } + + .oauth-consent__actions { + flex-wrap: wrap; + } + + .connected-apps { + display: flex; + flex-direction: column; + gap: var(--block-space); + } + + .connected-apps__group { + display: flex; + flex-direction: column; + } + + .connected-apps__empty { + border: 2px dashed var(--color-ink-lighter); + border-radius: 0.5em; + padding: var(--block-space) var(--inline-space-double); + } + + .connected-app { + align-items: start; + border-block-end: var(--border); + display: flex; + gap: var(--inline-space); + justify-content: space-between; + padding-block: var(--block-space-half); + + &:last-child { + border-block-end: 0; + } + + /* Narrow screens: the disconnect button drops under the details */ + @media (max-width: 480px) { + flex-direction: column; + } + } + + .connected-app__disconnect { + flex-shrink: 0; + } + + .connected-app__details { + gap: 0.15em; + min-inline-size: 0; + overflow-wrap: anywhere; + } +} diff --git a/app/controllers/my/connected_apps_controller.rb b/app/controllers/my/connected_apps_controller.rb index fb427f5043..ad7e0ccf75 100644 --- a/app/controllers/my/connected_apps_controller.rb +++ b/app/controllers/my/connected_apps_controller.rb @@ -29,7 +29,7 @@ def destroy private def set_connected_apps - tokens = oauth_tokens.unlapsed.includes(:oauth_client).order(:created_at) + tokens = oauth_tokens.unlapsed.includes(:oauth_client, :account).order(:created_at) @connected_apps = tokens.group_by(&:oauth_client).sort_by { |client, _| client.name.downcase } end diff --git a/app/controllers/oauth/authorizations/switches_controller.rb b/app/controllers/oauth/authorizations/switches_controller.rb new file mode 100644 index 0000000000..8cbec24fd0 --- /dev/null +++ b/app/controllers/oauth/authorizations/switches_controller.rb @@ -0,0 +1,50 @@ +# "Not you?" on the consent screen. Signs the person out, then brings whoever +# signs in next back to the same authorization request. The request is rebuilt +# from its parameters once they have passed every check the consent screen's +# did, never taken as a URL, so the only place this can return anyone to is +# consent for that request. The account choice isn't carried: it was the +# signed-out identity's, so the account check that guards granting doesn't +# apply here. The resource is carried, so an audienced request comes back as +# one, and its consent asks the new identity to pick an account. +class Oauth::Authorizations::SwitchesController < Oauth::AuthorizationsController + AUTHORIZATION_PARAMS = %i[ client_id redirect_uri response_type scope state code_challenge code_challenge_method resource ] + + # Account eligibility belongs to the identity being signed out, so neither + # account check applies; the request-level audience check still does. + skip_before_action :set_account, :set_accounts + before_action :require_browser_session + + + # A request too long to keep in the session can't be resumed, so the person + # stays signed in rather than signing out to nowhere. + def create + return_to = new_oauth_authorization_url(script_name: nil, **authorization_params) + + if return_to.bytesize <= RETURN_URL_MAXIMUM_LENGTH + terminate_session + reset_session + session[:return_to_after_authenticating] = return_to + + redirect_to_login_url + else + render_error "invalid_request", "This request is too long to pick up again after switching. Sign out, sign in as the right person, then connect from the app again." + end + end + + private + # Switching signs a browser out. A request authenticated by a bearer token + # has no session to end, so it is refused rather than half-handled. + def require_browser_session + head :forbidden unless Current.session + end + + # Switching identity is a POST but authorizes nothing, so a metadata + # document client is only read here, never registered. + def persist_metadata_document? + false + end + + def authorization_params + params.permit(*AUTHORIZATION_PARAMS).to_h.symbolize_keys + end +end diff --git a/app/controllers/oauth/authorizations_controller.rb b/app/controllers/oauth/authorizations_controller.rb index 088419ed72..803ad476f5 100644 --- a/app/controllers/oauth/authorizations_controller.rb +++ b/app/controllers/oauth/authorizations_controller.rb @@ -71,7 +71,7 @@ def create Oauth::Audit.record "code.issued", client: @client, identity_id: Current.identity.id, actor: "user" redirect_to success_redirect_uri(code), allow_other_host: true else - oauth_error "invalid_request", "Unknown client" + render_error "invalid_request", "Unknown client" end end @@ -118,8 +118,9 @@ def set_client end unless performed? + # There's no client to send the browser back to, so the person is told here. if @client.nil? - oauth_error "invalid_request", "Unknown client" + render_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 @@ -134,11 +135,15 @@ def metadata_document_client if denial? metadata_document_client_for_denial elsif Oauth::Availability.metadata_documents_enabled?(params[:client_id]) - resolution = Oauth::ClientMetadata::Resolver.new(params[:client_id], requester: Current.identity).resolve(persist: request.post?) + resolution = Oauth::ClientMetadata::Resolver.new(params[:client_id], requester: Current.identity).resolve(persist: persist_metadata_document?) resolution.client || refuse_metadata_document(resolution) end end + def persist_metadata_document? + request.post? + end + # A denial never registers a client, and stays reachable while metadata # documents are off: the stored row answers when it allows the redirect. # Otherwise (no row, or a stale one the consent screen saw past) the @@ -267,10 +272,17 @@ def redirect_with_error(error, description) end end + # Shown here rather than sent back to the app, and counted like any OAuth + # error the app would have been sent. def render_error(error, description) + instrument_oauth_error error @error = error @error_description = description - render :error, status: :bad_request + + respond_to do |format| + format.html { render :error, status: :bad_request } + format.json { render_oauth_error error, description, status: :bad_request } + end end def success_redirect_uri(code) diff --git a/app/controllers/oauth/base_controller.rb b/app/controllers/oauth/base_controller.rb index 9534561fb9..11e7568559 100644 --- a/app/controllers/oauth/base_controller.rb +++ b/app/controllers/oauth/base_controller.rb @@ -168,10 +168,13 @@ def prevent_referrer # them in Yabeda). The client is named by its type alone: a self-registered # client's id is anyone's to mint, so it can't be a metric label. def oauth_error(error, description = nil, status: :bad_request) + instrument_oauth_error error + render_oauth_error error, description, status: status + end + + def instrument_oauth_error(error) ActiveSupport::Notifications.instrument "error.oauth", endpoint: controller_name, error_code: error, client_type: oauth_error_client&.client_type || "unknown" - - render_oauth_error error, description, status: status end # A throttled request is counted as a rate-limit hit and not as an error, diff --git a/app/helpers/oauth_helper.rb b/app/helpers/oauth_helper.rb new file mode 100644 index 0000000000..1ada31e0cb --- /dev/null +++ b/app/helpers/oauth_helper.rb @@ -0,0 +1,118 @@ +module OauthHelper + # Bidirectional controls can make a name read as something else, or push the + # host beside it out of place. A self-registered name is the app's own claim, + # so they go before it is shown anywhere. + BIDI_CONTROLS = /[؜‎‏‪-‮⁦-⁩]/ + + # Grouped by what the app is (Oauth::Client#connection_kind), in this order; + # a group with nothing in it isn't shown. + CONNECTED_APP_GROUPS = { + web: "Apps and assistants on the web", + local: "Apps on your devices", + fizzy: "Fizzy apps" + } + + ACCESS_LEVELS = { + "read" => { + label: "Read only", + description: "See your boards, cards, comments and the people in your %{accounts}. It can't change anything." + }, + "read write" => { + label: "Read and write", + description: "See everything you can, and act as you: create, edit, move, close and delete cards, comments and boards." + } + } + + # A name of nothing but direction controls is no name, so it reads as one. + def oauth_client_self_name(client) + client.name.to_s.gsub(BIDI_CONTROLS, "").presence || "Unnamed app" + end + + # Who the person is dealing with, as far as we can vouch for it. A trusted or + # operator-registered app is known by its name. A self-registered app's name + # is only its claim, so it goes by where its access goes: the callback's + # host, port included when it isn't the default, or this device. + def oauth_client_display_identity(client, redirect_uri) + if client.dynamically_registered? + oauth_redirect_host(redirect_uri) || "an app on your device" + else + oauth_client_self_name(client) + end + end + + # Lowercase, as a browser reads it. nil for a callback that stays on this + # device: loopback, or an app's own URL scheme. + def oauth_redirect_host(redirect_uri) + uri = URI.parse(redirect_uri.to_s) + + if uri.host.present? && !Oauth.loopback_host?(uri.host) && uri.scheme.to_s.downcase.in?(%w[ http https ]) + uri.port == uri.default_port ? uri.host.downcase : "#{uri.host.downcase}:#{uri.port}" + end + rescue URI::InvalidURIError + nil + end + + # Every place outside this device a client's access can go, as consent would + # name each one. A client may register several callbacks, and its grants may + # have come through any of them, so none speaks for the rest. + def oauth_client_remote_hosts(client) + client.redirect_uris.filter_map { |uri| oauth_redirect_host(uri) }.uniq + end + + # The access levels a client may be granted, weakest first, each with what + # it lets the app do in plain words. A grant reaches every account the + # person belongs to, unless it's bound to the one picked at consent. + def oauth_access_levels(client, one_account: false) + levels = client.scopes.include?("write") ? ACCESS_LEVELS.keys : %w[ read ] + + levels.map do |scope| + level = ACCESS_LEVELS.fetch(scope) + [ scope, level.merge(description: format(level[:description], accounts: one_account ? "account" : "accounts")) ] + end + end + + def oauth_strongest_permission(tokens) + if tokens.any?(&:write?) + "write" + elsif tokens.any? + "read" + end + end + + def oauth_access_label(permission) + ACCESS_LEVELS.fetch(Oauth.canonical_scope(permission))[:label] + end + + # What an app's grants together let it do, as [ permission, accounts ] + # pairs, accounts nil for every account the person belongs to. An unbound + # grant reaches every account; a bound one adds only its own, and only + # where it grants more, so no pair claims more than some grant gives. + def oauth_connected_app_reach(tokens) + # A grant bound to an account since deleted, cancelled, or left reaches + # nothing: refreshing it would be refused, so it isn't shown as reach. + unbound, bound = tokens.partition { |token| !token.account_bound? } + bound = bound.select { |token| token.account && oauth_active_account_ids.include?(token.account_id) } + everywhere = oauth_strongest_permission(unbound) + + reach = bound.group_by(&:account).filter_map do |account, account_tokens| + permission = oauth_strongest_permission(account_tokens) + [ permission, account ] if everywhere.nil? || (permission == "write" && everywhere == "read") + end + + [ (everywhere && [ everywhere, nil ]), *reach.group_by(&:first).map { |permission, pairs| [ permission, pairs.map(&:last) ] } ].compact + end + + # The accounts the signed-in person is still active in, read once per render + # however many apps the page lists. + def oauth_active_account_ids + @oauth_active_account_ids ||= Current.identity.users_with_active_accounts.pluck(:account_id).to_set + end + + def connected_app_groups(connected_apps) + by_kind = connected_apps.group_by { |client, _| client.connection_kind } + + CONNECTED_APP_GROUPS.filter_map do |kind, heading| + [ kind, heading, by_kind[kind] ] if by_kind[kind].present? + end + end +end diff --git a/app/models/oauth/client.rb b/app/models/oauth/client.rb index e38b47a1da..617119110b 100644 --- a/app/models/oauth/client.rb +++ b/app/models/oauth/client.rb @@ -203,6 +203,21 @@ def self_registered? dynamically_registered? || metadata_document? end + # What kind of app this is, as this server sees it rather than as it names + # itself: Connected Apps groups by it. Our own apps are the trusted ones. An + # app whose every callback stays on this device, a loopback address or an + # app's own URL scheme, is a tool running here, such as a CLI or a desktop + # assistant. Anything else receives its access somewhere on the web. + def connection_kind + if trusted? + :fizzy + elsif redirect_uris.all? { |uri| on_device_uri?(uri) } + :local + else + :web + end + end + def confidential? token_endpoint_auth_method.in?(SECRET_AUTH_METHODS) end @@ -304,6 +319,16 @@ def validate_redirect_uri(uri) errors.add :redirect_uris, "includes an invalid URI" end + # As consent tells it (OauthHelper): a loopback host on http or https, or an + # app's own scheme. Redirect matching keeps its http-only loopback_uri?. + def on_device_uri?(uri) + parsed = URI.parse(uri) + web = parsed.scheme.to_s.downcase.in?(%w[ http https ]) + !web || Oauth.loopback_host?(parsed.host) + rescue URI::InvalidURIError + false + end + # http only: an app scheme naming a loopback host (cursor://localhost) is # dispatched to an app, not served on loopback (RFC 8252 §7.3). def loopback_uri?(uri) diff --git a/app/views/my/connected_apps/_connected_app.html.erb b/app/views/my/connected_apps/_connected_app.html.erb index 3c8b2a2f35..22641c09d1 100644 --- a/app/views/my/connected_apps/_connected_app.html.erb +++ b/app/views/my/connected_apps/_connected_app.html.erb @@ -1,44 +1,74 @@ - - <%= client.name %> - <%= tokens.map { |t| t.permission.humanize }.uniq.sort.join(", ") %> - <%= local_datetime_tag tokens.map(&:created_at).min, style: :datetime %> - -
- - - -

Disconnect <%= client.name %>?

-

This will revoke all of its access.

- - <%= form_with url: my_connected_app_path(client), method: :delete, class: "flex flex-column gap" do |form| %> - <% if webhooks.any? %> -
-

It set up <%= webhooks.one? ? "this webhook" : "these webhooks" %>, which will keep running unless you remove <%= webhooks.one? ? "it" : "them" %>:

-
    - <% webhooks.each do |webhook| %> -
  • - <%= webhook.name %> on <%= webhook.board.name %><%= " in #{webhook.account.name}" unless webhook.account == Current.account %> - <%= hidden_field_tag "webhook_ids[]", webhook.id, id: nil %> -
  • - <% end %> -
- - -
- <% end %> - -
- - <%= form.button "Disconnect", class: "btn txt-negative" %> +<% remote_hosts = oauth_client_remote_hosts(client) %> + +
+
+ <%# A self-registered app goes by the name it gave itself, since that's the + name people know it by, but never without where its access goes %> +

<%= oauth_client_self_name(client) %>

+ + <% if client.dynamically_registered? %> +

+ <% if remote_hosts.any? %> + Name set by the app at <%= remote_hosts.to_sentence(two_words_connector: " or ", last_word_connector: " or ") %> + <% else %> + Name set by an app on your device + <% end %> +

+ <% elsif client.metadata_document? %> +

Name set by the app at <%= URI.parse(client.client_id).host %>

+ <% elsif client.trusted? %> +

Made by 37signals

+ <% end %> + + <% oauth_connected_app_reach(tokens).each do |permission, accounts| %> +

+ <%= oauth_access_label(permission) %>, <% if accounts %>in <%= to_sentence(accounts.map { |account| tag.bdi(account.name) }) %><% else %>in every account you belong to<% end %> +

+ <% end %> + + <%# updated_at moves each time the app renews its access %> +

+ Connected <%= local_datetime_tag tokens.map(&:created_at).min, style: :date %> + · access last renewed <%= local_datetime_tag tokens.map(&:updated_at).max, style: :daysago %> + <% if tokens.many? %>· <%= pluralize(tokens.size, "connection") %><% end %> +

+
+ +
+ + + +

Disconnect <%= oauth_client_self_name(client) %>?

+

It loses its access to Fizzy right away. To use it again, you'd connect it again.

+ + <%= form_with url: my_connected_app_path(client), method: :delete, class: "flex flex-column gap" do |form| %> + <% if webhooks.any? %> +
+

It set up <%= webhooks.one? ? "this webhook" : "these webhooks" %>, which will keep running unless you remove <%= webhooks.one? ? "it" : "them" %>:

+
    + <% webhooks.each do |webhook| %> +
  • + <%= webhook.name %> on <%= webhook.board.name %><%= " in #{webhook.account.name}" unless webhook.account == Current.account %> + <%= hidden_field_tag "webhook_ids[]", webhook.id, id: nil %> +
  • + <% end %> +
+ +
<% end %> -
-
- - + +
+ + <%= form.button "Disconnect", class: "btn txt-negative" %> +
+ <% end %> +
+
+ diff --git a/app/views/my/connected_apps/index.html.erb b/app/views/my/connected_apps/index.html.erb index 49cb77e80b..83eca19f0a 100644 --- a/app/views/my/connected_apps/index.html.erb +++ b/app/views/my/connected_apps/index.html.erb @@ -12,25 +12,23 @@

<%= @page_title %>

<% end %> -
+
<% if @connected_apps.any? %> -

Apps you've authorized to access Fizzy on your behalf.

- - - - - - - - - - - <% @connected_apps.each do |client, tokens| %> +

Apps you've authorized to use Fizzy as you. Each one can reach what you can, with the access shown. Don't recognize one? Disconnect it, and its access ends right away.

+ + <% connected_app_groups(@connected_apps).each do |kind, heading, apps| %> +
"> +

" class="divider txt-medium margin-none"><%= heading %>

+ + <% apps.each do |client, tokens| %> <%= render "my/connected_apps/connected_app", client: client, tokens: tokens, webhooks: @webhooks_by_client.fetch(client.id, []) %> <% end %> -
-
AppPermissionsConnected
+
+ <% end %> <% else %> -

No apps are connected to your account. When you authorize an app to access Fizzy, it will appear here.

+
+

Nothing connected yet.

+

AI assistants and other apps you connect to Fizzy show up here, so you can see what each one can do and disconnect it whenever you like.

+
<% end %>
diff --git a/app/views/oauth/authorizations/_access.html.erb b/app/views/oauth/authorizations/_access.html.erb new file mode 100644 index 0000000000..9173a6d0f2 --- /dev/null +++ b/app/views/oauth/authorizations/_access.html.erb @@ -0,0 +1,24 @@ +<%# Fizzy's two access levels, each in plain words. Write includes read, so + the choice is one or the other; a client registered for read alone gets + the single option, shown so the person still sees what they're granting. %> +
+ Access + + <% oauth_access_levels(client, one_account: local_assigns[:one_account]).each do |value, level| %> + + <% end %> + +

+ <% if local_assigns[:one_account] %> + It works as you, so it can only reach what you can, in the account above. + <% else %> + It works as you, so it can only reach what you can, in every account you belong to. + <% end %> +

+
diff --git a/app/views/oauth/authorizations/_account_picker.html.erb b/app/views/oauth/authorizations/_account_picker.html.erb index bc0b513505..f1372a0f5a 100644 --- a/app/views/oauth/authorizations/_account_picker.html.erb +++ b/app/views/oauth/authorizations/_account_picker.html.erb @@ -1,6 +1,22 @@ -<%# An audienced grant (the hosted MCP server) reaches one account: the one picked here. %> -
- <%= form.label :account_id, "Account" %> - <%= form.select :account_id, options_from_collection_for_select(accounts, :id, :name, accounts.first&.id), {}, class: "input input--select" %> -

The app can reach this account only.

-
+<%# Which account this connection reaches. The controller sets accounts (the + identity's accounts) and the preselected one; the choice posts as + account_id. With one account there is nothing to choose, so it's named, + and carried in a hidden field. %> +
+ Account + + <% if accounts.one? %> + <% account = accounts.first %> + <%= form.hidden_field :account_id, value: account.id %> +

<%= account.name %>

+ <% else %> + <% accounts.each_with_index do |account, index| %> + + <% end %> + <% end %> + +

The app reaches this account only. Connect it again to add another.

+
diff --git a/app/views/oauth/authorizations/_client.html.erb b/app/views/oauth/authorizations/_client.html.erb new file mode 100644 index 0000000000..c5d28c76ed --- /dev/null +++ b/app/views/oauth/authorizations/_client.html.erb @@ -0,0 +1,28 @@ +<%# Who is asking, as far as Fizzy can vouch for it. A self-registered app's + name is only its claim, typed by whoever registered it, so it's shown as + exactly that: what the app says it is, quoted and subdued, under a heading + that names where the access goes (bc3 reads the same). bdi keeps any + right-to-left letters in the name from moving the quotes. %> +
+

<%= client.dynamically_registered? && redirect_host.nil? ? identity.upcase_first : identity %>

+ + <% if client.dynamically_registered? %> +

+ Says it’s “<%= oauth_client_self_name(client) %>” +

+ +

+ <% if redirect_host %> + We haven’t checked this app. Anyone can run an app at any address, so only authorize it if you trust <%= redirect_host %>. + <% else %> + We haven’t checked this app. It runs on your device and will get this access, so only authorize it if you started it. + <% end %> +

+ <% elsif client.metadata_document? %> +

+ Name set by the app at <%= URI.parse(client.client_id).host %> +

+ <% elsif client.trusted? %> +

Made by 37signals

+ <% end %> +
diff --git a/app/views/oauth/authorizations/error.html.erb b/app/views/oauth/authorizations/error.html.erb index 3493d9a7c2..40b7bb0413 100644 --- a/app/views/oauth/authorizations/error.html.erb +++ b/app/views/oauth/authorizations/error.html.erb @@ -1,15 +1,15 @@ -
+<%# Shown on Fizzy, never redirected: the request couldn't be trusted enough + to send the browser back to the app (see redirect_with_error) %> +<% @page_title = "Can't authorize this app" %> + + diff --git a/app/views/oauth/authorizations/new.html.erb b/app/views/oauth/authorizations/new.html.erb index cf797dd821..80f1ffb94e 100644 --- a/app/views/oauth/authorizations/new.html.erb +++ b/app/views/oauth/authorizations/new.html.erb @@ -1,10 +1,25 @@ -<% @page_title = "Authorize #{@client.name}" %> +<% identity = oauth_client_display_identity(@client, @redirect_uri) %> +<% redirect_host = oauth_redirect_host(@redirect_uri) %> +<%# A self-registered web app is headed by where its access goes, as bc3's + consent reads: "Authorize the app at ". %> +<% @page_title = "Authorize #{@client.dynamically_registered? && redirect_host ? "the app at #{redirect_host}" : identity}" %> <% content_for :header do %>

<%= @page_title %>

<% end %> -
+