Возможность регистрации валидного auth_redirect клиентом User Api Key

Разработка
1

Я размышляю о том, целесообразно ли разрешить клиентам user api key регистрировать валидный auth_redirect для целей создания ключей.

Текущая функциональность

В настоящее время редиректы для пользовательских API-ключей должны регистрироваться хост-сайтом в настройке сайта allowed_user_api_auth_redirects. Любой auth_redirect, отправленный с запросом на создание ключа, проверяется по этому списку настроек сайта (см. здесь). Это разумная мера защиты от открытых редиректов, которые создают различные риски для безопасности.

Эффективная причина

Эффективной причиной для такого рассмотрения является функция авторизации пользователей плагина ActivityPub. Она позволяет пользователю подтвердить владение актером ActivityPub, чтобы действия этого актера могли быть связаны с его учетной записью на данном инстансе. Вот как это выглядит:

В настоящее время это работает с Mastodon, поскольку каждый сервер Mastodon является провайдером OAuth, который позволяет программно создавать OAuth-клиенты. Процесс выглядит следующим образом:

  1. Пользователь указывает домен Mastodon, на котором находится его учетная запись.
  2. Если это первый раз авторизации на этом домене, плагин создает OAuth-приложение на этом сервере Mastodon, регистрируя свои редиректы и получая учетные данные клиента для OAuth-потока.
  3. Плагин выполняет OAuth-поток с пользователем, используя созданное (или ранее созданное) в пункте 2 OAuth-приложение.

Цель — реализовать аналогичную функциональность в Discourse с помощью пользовательских API-ключей. Это уже реализовано в данном PR:

Этот процесс требует, чтобы хост-сайт заранее добавил домен или редирект клиента в allowed_user_api_auth_redirects.

Возможное изменение

Сейчас я думаю о создании PR для discourse/discourse, который бы выполнял следующее:

Создание таблицы user_api_key_client

Колонки:

  • client_id и application_name, перенесенные из user_api_key
  • redirect_uri (необязательно)
  • public_key (необязательно)

Вся существующая функциональность, использующая client_id и application_name, будет работать как прежде, через user_api_key_client.

Добавление маршрута для регистрации клиента

post "/user-api-key/register" => "user_api_keys#register"

Обязательные параметры:

  • application_name
  • client_id
  • public_key
  • auth_redirect

Действие:

  • Валидация и сохранение параметров в user_api_key_client
  • Возврат кода успеха в случае успеха

Разрешение использования зарегистрированных клиентов в действиях создания

Изменить действие create в контроллере UserApiKey следующим образом:

  • Если client_id существует в user_api_key_client и имеет И auth_redirect, И public_key:
    • Проверить использование auth_redirect с сохраненным auth_redirect
    • Использовать сохраненный public_key для шифрования возвращаемой полезной нагрузки
  • В противном случае следовать существующей функциональности.

cc @pmusaraj

2

В целом, я поддерживаю это, так как это позволит реализовать рабочий процесс авторизации пользователей в плагине ActivityPub и за его пределами без лишних сложностей для администраторов.

Однако есть несколько вопросов:

Мне не очень нравится идея миграции здесь; я предпочёл бы оставить существующие API-ключи пользователей без изменений, поскольку идентификатор и имя отдельных ключей пользователей независимы от идентификатора и имени зарегистрированных клиентов. Возможно, вместо этого мы могли бы добавить столбец user_api_key_client_id в таблицу user_api_key. Тогда ключи, связанные с клиентами, смогут использовать перенаправление, настроенное клиентом (а существующие ключи, разумеется, сохранят текущее поведение).

Кроме того, должны ли администраторы иметь возможность просматривать где-либо список зарегистрированных клиентов?

4

Вот почему я считаю, что client_id следует мигрировать :slight_smile:

В настоящее время таблица user_api_keys представляет собой смешение того, что, возможно, должно быть двумя отдельными моделями данных: ключи API пользователей и клиенты ключей API пользователей. Ограничения текущего подхода видны в методе создания ключа (здесь):

# уничтожить все старые ключи, которые у нас были
UserApiKey.where(user_id: current_user.id, client_id: params[:client_id]).destroy_all

key =
  UserApiKey.create!(
    application_name: @application_name,
    client_id: params[:client_id],
    user_id: current_user.id,
    push_url: params[:push_url],
    scopes: scopes.map { |name| UserApiKeyScope.new(name: name) },
  )

Чтобы создать новый ключ, необходимо уничтожить все ключи с тем же client_id, чтобы сохранить уникальность client_id. Иными словами, текущая модель данных требует соотношения 1:1 между клиентом и ключом. Такое соотношение 1:1 полезно для подмножества случаев использования, но ограничивает другие.

Действительно, именно это ограничение 1:1 является причиной того, что предлагаемый вами подход, вероятно, не сработает.

Чтобы клиент мог зарегистрировать перенаправление для нескольких ключей, уникальность client_id должна быть независима от самих ключей. В противном случае регистрация не будет работать для нескольких ключей, так как client_id в действии регистрации используется для идентификации перенаправления.

Если вы перенесете client_id и application_name в выделенную таблицу user_api_key_clients, вы сохраните случаи 1:1, но также получите возможность случаев 1:множество и функций, которые это открывает, таких как регистрация redirect_uri для целей ActivityPub.

6

Извините за задержку, Ангус.

Я понимаю, что вы имеете в виду, но при миграции существующих client_id мы сохраним часть этого смешения в новой таблице, поскольку мигрированные идентификаторы/имена будут относиться к ключам, которые являются клиентами в соотношении 1-к-1. Особенно с точки зрения администратора в будущем, полезно иметь список зарегистрированных клиентов в соотношении многие-к-одному.

Но, возможно, решение этого вопроса неуместно в данный момент.

Давайте продолжим с планом, который вы предлагаете, с нетерпением ждём ваш PR. Спасибо!

7

Я опубликовал черновик PR с реализацией этого

Обратите внимание на следующее:

  • Вся существующая функциональность работает как прежде.
  • Миграция обратима.

Сделать:

  • Исправить падающие тесты.
  • В настоящее время любой пользователь может зарегистрировать клиент. Нужно уточнить, следует ли как-то ограничивать это (?).
8

Я перебил эту PR, так как вернулся к работе над ActivityPub, и это потенциальная основа для одной из её функций, как обсуждалось в первом посте.

При перебивании я заметил, что разделение ключей и клиентов, как это сделано в данной PR, также решит проблемы, подобные той, которую недавно исправил @nat.

А именно, необходимость внести это изменение — уничтожить все старые ключи, связанные с клиентом, независимо от пользователя, — возникает из-за того, что ключи и клиенты находятся в одной таблице. Их разделение позволит просто зарегистрировать новый ключ для альтернативного пользователя этого клиента.

9

Наконец-то это объединено, огромное спасибо @angus, надеюсь, это разблокирует вас сейчас.

10

Огромное спасибо! Это разблокировало меня :slight_smile: Скоро будет реализация в PR аутентификации ActivityPub Discourse-to-Discourse.