Viabilidade de permitir que um cliente com Chave API de Usuário registre um auth_redirect válido

Dev
1

Estou a considerar se é viável permitir que clientes de chaves de API de utilizador registem um auth_redirect válido para fins de criação de chaves.

Funcionalidade Atual

Atualmente, os redirecionamentos de chaves de API de utilizador têm de ser registados pelo site anfitrião na configuração do site allowed_user_api_auth_redirects. Qualquer auth_redirect enviado com um pedido de criação de chave é verificado em relação a essa lista de configurações do site (ver aqui). Esta é uma proteção sensata para evitar um redirecionamento aberto (que dá origem a vários riscos de segurança).

Causa Eficiente

A causa eficiente para esta consideração é a funcionalidade de autorização de utilizador do plugin ActivityPub. Isto permite que um utilizador prove a posse do seu ator ActivityPub para que as atividades desse ator possam ser associadas à sua conta numa determinada instância. É assim que isso funciona:

Isto funciona atualmente com o Mastodon, pois cada servidor Mastodon é um fornecedor OAuth que permite a criação programática de clientes OAuth. O fluxo funciona da seguinte forma:

  1. O utilizador identifica o domínio Mastodon em que a sua conta se encontra.
  2. Se for a primeira vez que é autorizado nesse domínio, o Plugin cria uma Aplicação OAuth nesse servidor Mastodon, registando os seus redirecionamentos e obtendo credenciais de cliente para um Fluxo OAuth.
  3. O Plugin executa o fluxo OAuth com o utilizador utilizando a Aplicação OAuth que criou (ou criou anteriormente) em 2.

O objetivo é ter essencialmente a mesma funcionalidade com o Discourse utilizando Chaves de API de Utilizador. Isto já está implementado neste PR:

Este fluxo requer que o site anfitrião já tenha adicionado o domínio ou redirecionamento do cliente a allowed_user_api_auth_redirects.

Mudança Possível

O que estou a pensar atualmente é fazer um PR para discourse/discourse que faria o seguinte:

Criar uma tabela user_api_key_client

Colunas:

  • client_id e application_name migrados de user_api_key
  • redirect_uri (opcional)
  • public_key (opcional)

Toda a funcionalidade existente que utiliza client_id e application_name funcionaria como antes, através de user_api_key_client.

Adicionar uma rota de registo de cliente

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

Parâmetros necessários:

  • application_name
  • client_id
  • public_key
  • auth_redirect

Ação:

  • Validar e guardar os parâmetros em user_api_key_client
  • Retornar um código de sucesso se for bem-sucedido

Permitir o uso de clientes registados em ações de criação

Alterar a ação create do UserApiKeyController da seguinte forma:

  • Se client_id existir em user_api_key_client e tiver AMBOS auth_redirect e public_key:
    • Validar o uso de auth_redirect com o auth_redirect armazenado
    • Usar a public_key armazenada para encriptar o payload de retorno
  • Caso contrário, seguir a funcionalidade existente.

cc @pmusaraj

2 curtidas
2

No geral, sou a favor disso porque permitiria o fluxo de autorização do usuário no plugin ActivityPub e além, sem muito atrito para os administradores.

Algumas perguntas, no entanto:

Não sou um grande fã da migração aqui, preferiria deixar as chaves de API de usuário existentes como estão porque o id e o nome das chaves de usuário individuais são independentes do id e do nome dos clientes registrados. Talvez, em vez disso, possamos adicionar uma coluna a user_api_key para user_api_key_client_id. As chaves com clientes associados podem então usar o redirecionamento configurado pelo cliente (e as chaves existentes, é claro, mantêm o comportamento atual).

Além disso, os administradores deveriam poder revisar em algum lugar a lista de clientes registrados?

1 curtida
4

É por isso que acho que o client_id deveria ser migrado :slight_smile:

Atualmente, a tabela user_api_keys é uma mistura do que talvez devesse ser dois modelos de dados separados: chaves de API de usuário e clientes de chaves de API de usuário. Você pode ver as limitações da abordagem atual no método de criação de chave (aqui):

# destrói quaisquer chaves antigas que tínhamos
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) },
  )

Para criar uma nova chave, você tem que destruir todas as chaves com o mesmo client_id para manter o client_id único. Em outras palavras, o modelo de dados atual exige um relacionamento 1:1 entre cliente e chave. Esse relacionamento 1:1 é útil para um subconjunto de casos de uso, mas limita outros.

De fato, essa limitação 1:1 é o motivo pelo qual a abordagem que você está sugerindo provavelmente não funcionaria

Para que um cliente registre um redirecionamento para várias chaves, a exclusividade do client_id deve ser independente das próprias chaves. Caso contrário, o registro não funcionará para várias chaves, pois o client_id na ação de registro é o que é usado para identificar o redirecionamento.

Se você mover o client_id e o application_name para a tabela dedicada user_api_key_clients, você mantém os casos 1:1, mas também permite casos 1:muitos e os recursos que eles abrem, como o registro de redirect_uri para fins de ActivityPub.

1 curtida
6

Desculpe pelo atraso, Angus.

Entendo o que você está dizendo, mas ao migrar o client_id existente, manteríamos parte dessa conglomeração na nova tabela porque os IDs/nomes migrados seriam para chaves que são clientes 1 para 1. Especialmente da perspectiva de um administrador no futuro, é bom ter uma lista dos clientes multi-para-um registrados.

Mas talvez abordar isso não seja apropriado neste momento.

Vamos seguir com o plano que você está propondo, aguardando o PR. Obrigado!

1 curtida
7

Publiquei um rascunho de PR implementando isso

Note que:

  • Toda a funcionalidade existente funciona como antes.
  • A migração é reversível.

A fazer:

  • Corrigir testes falhando.
  • Atualmente, qualquer usuário pode registrar um cliente. Consultar se isso deve ser restrito de alguma forma (?)
1 curtida
8

Rebasei este PR, pois estou de volta ao foco no ActivityPub, e esta é uma estrutura potencial para um de seus recursos, conforme discutido no OP.

Enquanto fazia o rebase, notei que separar chaves de clientes, como este PR faz, também resolveria problemas como o abordado recentemente por @nat

Ou seja, a necessidade de fazer essa alteração, para destruir todas as chaves antigas associadas a um cliente, independentemente do usuário, surge porque chaves e clientes estão na mesma tabela. Separados, você pode simplesmente registrar uma nova chave para o usuário alternativo do cliente.

1 curtida
9

Isso finalmente foi mesclado, muito obrigado @angus, espero que isso te desbloqueie agora.

1 curtida
10

Muito obrigado! Isso me (desbloqueou) :slight_smile: Em breve haverá uma implementação em um PR de autenticação Discourse-para-Discourse do ActivityPub.

1 curtida