random42 / passport-spid

Passport strategy for SPID (italian citizens) authentication
MIT License
12 stars 5 forks source link

getIDPEntityIdFromRequest allowed to return null #1

Closed dariofurlan closed 4 months ago

dariofurlan commented 1 year ago

If the getIDPEntityIdFromRequest() method will be called even for metadata generation it should be allowed to return a null (or an undefined) also in the types.

export interface SpidConfig extends StrategyOptions {
  saml: SpidSamlConfig;
  spid: {
    serviceProvider: ServiceProvider;
    IDPRegistryMetadata: string;
    // getIDPEntityIdFromRequest: (req: Request) => string | Promise<string>;
    getIDPEntityIdFromRequest: (req: Request) => (string | null) | Promise<string | null>;
    authnContext: SpidLevel;
  };
  cache: Cache;
}

Looking into the MultiSamlStrategy when generating SP metadata it passes an {} as Request so it could be better improved detecting this call in some way. I'll try something...

Ottimo lavoro, ottima libreria. Bravo!

random42 commented 1 year ago

Ciao Dario,

Non è corretto che la funzione getIDPEntityIdFromRequest possa tornare undefined, perché è lo sviluppatore che deve fornire un IDP di default nel caso non sia presente nella richiesta, o lanciare un errore se volesse. Una possibile feature che mi hai fatto venire in mente sarebbe passare come secondo parametro la lista di IDP, per facilitarne l'implementazione. Purtroppo per come funziona la generazione del metadata nella classe MultiSamlStrategy si passa comunque da _getSpidSamlOptions.

La soluzione per evitare di chiamare getIDPEntityIdFromRequest durante la generazione del metadata (così da lasciare la possibiltà di lanciare un errore) è passare una request null ed evitare di chiamare la funzione in quel caso.

Direi che posso aggiungere entrambe le cose nella versione 2.1. Vedi qualche problema? Grazie del feedback!