Fattibilità di consentire a un client con User Api Key di registrare un auth_redirect valido

Sviluppatore
1

Sto valutando se sia fattibile consentire ai client user api key di registrare un auth_redirect valido ai fini della creazione della chiave.

Funzionalità Attuale

Attualmente, i reindirizzamenti delle user api key devono essere registrati dal sito host nell’impostazione del sito allowed_user_api_auth_redirects. Qualsiasi auth_redirect inviato con una richiesta di creazione chiave viene verificato rispetto a quell’elenco di impostazioni del sito (vedi qui). Questa è una protezione sensata per evitare un reindirizzamento aperto (che dà origine a vari rischi di sicurezza).

Causa Efficiente

La causa efficiente di questa considerazione è la funzionalità di autorizzazione utente del plugin ActivityPub. Ciò consente a un utente di dimostrare la proprietà di un attore ActivityPub in modo che le attività di tale attore possano essere associate al proprio account su una data istanza. Questo è ciò che appare:

Attualmente funziona con Mastodon poiché ogni server Mastodon è un provider OAuth che consente la creazione programmatica di client OAuth. Il flusso funziona in questo modo:

  1. L’utente identifica il dominio Mastodon su cui si trova il suo account.
  2. Se è la prima volta che viene autorizzato su quel dominio, il Plugin crea un’App OAuth su quel server Mastodon, registrando i suoi reindirizzamenti e ottenendo le credenziali del client per un Flusso OAuth.
  3. Il Plugin esegue il flusso OAuth con l’utente utilizzando l’App OAuth creata (o precedentemente creata) al punto 2.

L’obiettivo è avere una funzionalità effettivamente identica con Discourse utilizzando le User Api Keys. Questo è già implementato in questo PR:

Questo flusso richiede che il sito host abbia già aggiunto il dominio o il reindirizzamento del client a allowed_user_api_auth_redirects.

Possibile Modifica

Quello a cui sto pensando attualmente è creare un PR per discourse/discourse che farebbe quanto segue:

Creare una tabella user_api_key_client

Colonne:

  • client_id e application_name migrati da user_api_key
  • redirect_uri (opzionale)
  • public_key (opzionale)

Tutta la funzionalità esistente che utilizza client_id e application_name funzionerebbe come prima, tramite user_api_key_client.

Aggiungere una route di registrazione client

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

Parametri richiesti:

  • application_name
  • client_id
  • public_key
  • auth_redirect

Azione:

  • Validare e salvare i parametri in user_api_key_client
  • Restituire un codice di successo se l’operazione ha avuto successo

Consentire l’uso di client registrati nelle azioni di creazione

Modificare l’azione create di UserApiKeyController come segue:

  • Se client_id esiste in user_api_key_client e ha SIA auth_redirect che public_key:
    • Validare l’uso di auth_redirect con il auth_redirect memorizzato
    • Utilizzare la public_key memorizzata per crittografare il payload restituito
  • Altrimenti, seguire la funzionalità esistente.

cc @pmusaraj

2 Mi Piace
2

Nel complesso, sono favorevole a questo perché consentirebbe il flusso di lavoro di autorizzazione dell’utente nel plugin ActivityPub e oltre senza troppi intoppi per gli amministratori.

Tuttavia, alcune domande:

Non sono un grande fan della migrazione qui, preferirei lasciare le chiavi API utente esistenti così come sono perché l’ID e il nome delle singole chiavi utente sono indipendenti dall’ID e dal nome dei client registrati. Forse invece possiamo aggiungere una colonna a user_api_key per user_api_key_client_id. Le chiavi con client associati potranno quindi utilizzare il reindirizzamento configurato dal client (e le chiavi esistenti, ovviamente, manterranno il comportamento attuale).

Inoltre, gli amministratori dovrebbero essere in grado di rivedere da qualche parte l’elenco dei client registrati?

1 Mi Piace
4

Questo è il motivo per cui penso che il client_id dovrebbe essere migrato :slight_smile:

Attualmente la tabella user_api_keys è una conflazione di quelli che forse dovrebbero essere due modelli di dati separati: chiavi API utente e client di chiavi API utente. Puoi vedere i limiti dell’approccio attuale nel metodo di creazione della chiave (qui):

# distruggi tutte le vecchie chiavi che avevamo
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) },
  )

Per creare una nuova chiave devi distruggere tutte le chiavi con lo stesso client_id per mantenere il client_id univoco. In altre parole, il modello di dati attuale richiede una relazione 1:1 tra client e chiave. Quella relazione 1:1 è utile per un sottoinsieme di casi d’uso, ma ne limita altri.

Infatti, questa limitazione 1:1 è il motivo per cui l’approccio che stai suggerendo probabilmente non funzionerebbe

Affinché un client possa registrare un reindirizzamento per più chiavi, l’univocità del client_id deve essere indipendente dalle chiavi stesse. Altrimenti la registrazione non funzionerà per più chiavi poiché il client_id nell’azione di registrazione è ciò che viene utilizzato per identificare il reindirizzamento.

Se sposti il client_id e application_name nella tabella dedicata user_api_key_clients, mantieni i casi 1:1, ma consenti anche casi 1:molti e le funzionalità che ne derivano, come la registrazione di redirect_uri ai fini di ActivityPub.

1 Mi Piace
6

Mi dispiace per il ritardo, Angus.

Capisco cosa intendi, ma migrando l’client_id esistente, manterremmo parte di questa conflazione nella nuova tabella perché gli ID/nomi migrati sarebbero per chiavi che sono client 1-a-1. Soprattutto dalla prospettiva di un amministratore in futuro, è bene avere un elenco dei client multi-a-uno registrati.

Ma forse affrontare questo aspetto non è appropriato in questo momento.

Andiamo avanti con il piano che proponi, attendo con ansia la PR. Grazie!

1 Mi Piace
7

Ho pubblicato una bozza di PR che implementa questo

Nota che:

  • Tutta la funzionalità esistente funziona come prima.
  • La migrazione è reversibile.

Da fare:

  • Correggere i test falliti.
  • Attualmente qualsiasi utente può registrare un client. Chiedere se questo debba essere in qualche modo limitato (?)
1 Mi Piace
8

Ho ribasato questo PR poiché sono tornato a concentrarmi su ActivityPub, e questo è un potenziale framework per una delle sue funzionalità, come discusso nell’OP.

Mentre ribasavo, ho notato che separare le chiavi dai client, come fa questo PR, risolverebbe anche problemi come quello affrontato di recente da @nat

Vale a dire, la necessità di apportare questa modifica, per distruggere tutte le vecchie chiavi associate a un client, indipendentemente dall’utente, nasce dal fatto che chiavi e client si trovano nella stessa tabella. Separarli significa che puoi semplicemente registrare una nuova chiave per l’utente alternativo del client.

1 Mi Piace
9

Questo è stato finalmente unito, grazie mille @angus, spero che questo ti sblocchi ora.

1 Mi Piace
10

Molte grazie! Mi ha (sbloccato) :slight_smile: Ci sarà presto un’implementazione in una PR di autenticazione Discourse-to-Discourse ActivityPub.

1 Mi Piace