Repository navigation
Leer los certificados solo al listar: la vista previa y la firma resuelven el elegido sin abrir almacenes - #2102
Conversation
Implements #2093: Un listado del DNIe abre una sola sesión con la tarjeta - list lee titulares e intermedia en una sesión y guarda lo leído para el every_certificate siguiente - lo guardado se consume una vez: la tarjeta no se retiene y el puerto Token no cambia
fix(identity): un listado del DNIe abre una sola sesión con la tarjeta
…ado, sin abrir almacenes Implements #2092: Resolver el certificado elegido contra el último listado, sin abrir almacenes - chosen lee de LastListing y solo lista si no hay ningún listado - los listados guardan cada certificado con sus emisores - quitar un .p12 lo saca del último listado
perf(identity): resolver el certificado elegido contra el último listado, sin abrir almacenes
perf(identity): resolver el certificado elegido contra el último listado, sin abrir almacenes
perf(identity): al recuperar el foco, listar solo los almacenes que no son de tarjeta
sgomez
left a comment
There was a problem hiding this comment.
NEEDS_FIXES
Alcance: completo (no había revisión previa), contra origin/main, head 667e85ea. Revisado contra la spec #2091 y sus tres sub-issues (#2092, #2093, #2094). Checks: seguían en marcha en 667e85ea (Cadena TypeScript, Cadena Rust, Imagen nativa, Plataformas / macOS, Plataformas / Windows y los Analyze de CodeQL), así que no hay veredicto de CI todavía. No he corrido la suite en local.
Cada sub-issue funciona por separado. Fallan dos combinaciones sobre mecanismos que comparten. Las he confirmado con pruebas desechables que ya he borrado.
[blocking]
- La lista al recuperar el foco (#2094) y la lista en caliente (#2092 y ADR-0048) se pisan en
LastListing, y el certificado de la tarjeta deja de resolverse (readers.rs,relisted_other_than_cards). La lista del foco lee la parte de tarjeta del último listado antes de listar los otros almacenes y guarda después. Si la lista en caliente guarda una tarjeta recién metida en ese intervalo, la del foco la sobrescribe con la parte vieja. La ventana enseña el certificado de la tarjeta, porque el anuncio de los lectores invalida la respuesta del foco, perochosenya no lo encuentra: la vista previa y la firma fallan conCertificateNotFound. Además no se arregla al volver a la ventana, porque cada foco vuelve a tomar del último listado una parte de tarjeta en la que ya no está. Prueba desechable: unCountingque se queda bloqueado al listar el NSS, conrelisted_other_than_cards()en otro hilo; entretanto,relisted()con la tarjeta ya puesta; luego,chosen_from_the_last_listingcon el asa que anunció →Err(CertificateNotFound). - El foco que llega durante un listado completo deja tirado ese listado, y si la lista del foco falla, la ventana se queda en «loading» para siempre (
useCertificateListing.ts).onFocusincrementalatestListing, así que se descarta el resultado dellookAgainque está en marcha (el del arranque, «Volver a buscar», el de después de instalar o quitar un.p12). Luego el error del foco se traga con() => {}. Lo he confirmado con dos pruebas desechables: (a) el primer listado está pendiente, llega un foco cuyolistOtherThanCardsse rechaza, y el listado se resuelve bien:listingse queda en{"kind":"loading"}; (b) foco durante «Volver a buscar»: la ventana enseña lo que devuelve el foco y se pierde el certificado nuevo de la tarjeta que había traído el listado completo, justo lo que la historia 15 deja en manos de «Volver a buscar» cuando no hay vigilante. En el arranque, ese foco sin listado previo cae además en el listado completo derelisted_other_than_cardsy lee el DNIe una segunda vez, en contra de la historia 16.
[fix]
- Ninguna prueba se pone roja si la vista previa o la firma vuelven a listar (historia 18, criterio «La vista previa del sello y la firma no listan ningún almacén» de #2092). Las pruebas nuevas atacan a
chosen_from_the_last_listing, pero no a la fachadaIdentityRoot::chosenni awhat_is_ordered, que es por donde entran la vista previa y la firma. Sichosenvolviera aself.certificates(), todo seguiría en verde. Hay que añadir una prueba sobreIdentityRoot::chosencon un token que cuente las llamadas: listado completo primero, luegochosen(asa), y comprobar que no se llama alist.
[note]
certificates.rsimportarows_marking_the_installeddereaders.rs, yreaders.rsimportalisted_rowsywith_their_chainsdecertificates.rs. Es una dependencia circular entre dos módulos hermanos. Podría ser envidia de funcionalidad (feature envy):rows_marking_the_installedes decertificates.rs.DnieToken::last_readlo consume el siguienteevery_certificatedel mismo almacén, sea del listado que sea. Si dos listados se cruzan (arranque, lista en caliente y la reserva dechosen) y entretanto se ha cambiado la tarjeta, uno puede completar la cadena con lo que leyó el otro. Es poco probable y solo afecta a la cadena, pero conviene saberlo.
| impl CardListing<'_> { | ||
| /// Al recuperar el foco: abre solo los almacenes que no son de tarjeta y toma el resto del último listado. | ||
| pub fn relisted_other_than_cards(&self) -> Result<Vec<ListedCertificate>, TokenError> { | ||
| let Some(on_the_cards) = self.last.on_a_card(self.installed_dir) else { |
There was a problem hiding this comment.
[blocking] Esto se lee antes de listar los otros almacenes, y en la línea 198 se guarda después. Si la lista en caliente guarda una tarjeta recién metida en medio, este keep la borra del último listado. La ventana la sigue enseñando, porque el anuncio de los lectores invalida esta respuesta, pero chosen falla con CertificateNotFound, y los focos siguientes no la recuperan. Lo he confirmado con una prueba desechable: Counting bloqueado al listar el NSS, relisted() con la tarjeta puesta entretanto y, al final, chosen_from_the_last_listing(asa anunciada) → Err(CertificateNotFound). Arreglo: listar primero los almacenes que no son de tarjeta y, después, con un único bloqueo de LastListing, tomar la parte de tarjeta actual y guardar. Por ejemplo, con un LastListing::replace_other_than_cards(found) que haga las dos cosas bajo el mismo lock. Añade una prueba como la desechable, con barreras.
There was a problem hiding this comment.
Arreglado en 572bc8a: LastListing::replace_other_than_cards toma la parte de tarjeta actual y guarda bajo un único lock, después de listar los otros almacenes. Prueba con barreras: a_card_put_in_while_the_focus_lists_is_still_resolved_afterwards, roja antes del arreglo con CertificateNotFound. Para no pasar del umbral de files_stay_small, las pruebas de readers se parten en tests/mod.rs, tests/chosen.rs y tests/focus.rs.
| ); | ||
|
|
||
| useEffect(() => { | ||
| const onFocus = () => { |
There was a problem hiding this comment.
[blocking] Al incrementar latestListing, el foco descarta el lookAgain en curso (el del arranque, «Volver a buscar», el de después de instalar o quitar), y luego su error se traga en la línea 59. Lo he confirmado con pruebas desechables: (a) el primer listado está pendiente y el foco falla: listing se queda en loading para siempre aunque el listado completo acabe bien; (b) foco durante «Volver a buscar»: se enseña la lista del foco y se pierde la tarjeta nueva que había traído el listado completo (historia 15). En el arranque, además, ese foco sin último listado hace en el backend un segundo listado completo y lee el DNIe dos veces (historia 16). Arreglo: no lanzar la lista del foco mientras haya un listado completo en marcha (listing.kind === "loading" o un ref de «en curso»); un listado completo ya cubre lo que la lista del foco iba a traer. Prueba para cada uno de los dos casos.
There was a problem hiding this comment.
Arreglado en 572bc8a: el foco no lanza la lista mientras haya un listado completo en marcha (un ref que cuenta los lookAgain en curso), así que ni descarta su resultado ni deja la ventana en loading, y en el arranque no se lee el DNIe dos veces. Una prueba por caso: el primer listado pendiente con un foco que falla, y el foco durante «Volver a buscar»; las dos, rojas antes del arreglo.
|
|
||
| /// El certificado del asa, listado de nuevo y comprobado antes de usarlo. | ||
| /// El certificado del asa, tomado del último listado y comprobado antes de usarlo. | ||
| pub fn chosen(&self, handle: &str) -> Result<TokenCertificate, TokenError> { |
There was a problem hiding this comment.
[fix] Esta fachada es por donde entran la vista previa y la firma (what_is_ordered), y ninguna prueba la cubre: si volviera a self.certificates(), todo seguiría en verde (historia 18 y criterio de #2092). Añade una prueba de IdentityRoot::chosen con un token que cuente list: después de un listado completo, chosen(asa) no lista.
There was a problem hiding this comment.
Arreglado en 572bc8a: after_a_full_listing_the_chosen_certificate_opens_no_store (identity/tests.rs) lista por completo y comprueba que IdentityRoot::chosen no llama a list. Comprobada con un cebo: con self.certificates() dentro de chosen, se pone roja.
La lista del foco guarda la parte de tarjeta actual bajo el mismo bloqueo, el foco no lista durante un listado completo y una prueba fija que IdentityRoot::chosen no lista.
sgomez
left a comment
There was a problem hiding this comment.
CLEAN
Alcance: incremental, desde la revisión anterior en 667e85ea hasta 572bc8a0 (un commit, fix(pr): atiende la revisión de #2102). Lo he revisado contra la spec #2091 y sus sub-issues #2092, #2093 y #2094.
Checks en 572bc8a0:
- Cadena Rust está en rojo, pero no ha llegado a compilar. Falla en
setup-runnerporque se agota el tiempo al descargar pnpm («Could not reach https://registry.npmjs.org/@pnpm/linux-x64/11.25.0: timed out after 30s»), yjust check-rustse queda en skipped. Es un rojo de infraestructura que no dice nada del código, y hay que relanzarlo: https://github.com/sgomez/rfirma/actions/runs/39735283185/job/119679464949. Para no quedarme sin veredicto de Rust, he corrido en localjust test-one-rust identity::: 366 en verde, entre ellos las pruebas nuevas. - Siguen en marcha: Cadena TypeScript, Imagen nativa, Plataformas / Windows, Plataformas / macOS y Analyze (rust). En local he corrido
useCertificateListing.test.tsy está en verde (17).
Hallazgos anteriores
- [blocking] Lista del foco contra lista en caliente en
LastListing: arreglado.replace_other_than_cardstoma la parte de tarjeta actual y guarda con un sololock, después de listar los otros almacenes.a_card_put_in_while_the_focus_lists_is_still_resolved_afterwardsreproduce el cruce con barreras. - [blocking] El foco descartaba el listado completo en curso: arreglado.
fullListingsUnderWayimpide lanzar la lista del foco mientras haya unlookAgainen marcha, y lo baja en unfinally. Las dos pruebas nuevas se pondrían rojas sin la guarda: la primera se quedaría enloadingy la segunda enseñaría la lista del foco. - [fix] Faltaba una prueba de la fachada
IdentityRoot::chosen: arreglado.after_a_full_listing_the_chosen_certificate_opens_no_storecuenta loslistdel token después de un listado completo.
Combinaciones sobre los mecanismos que ha tocado el arreglo
Las he confirmado con pruebas desechables, ya borradas, y ninguna falla:
- Gancho, foco durante la instalación de un
.p12: el foco llega entre el diálogo deinstally sulookAgain, y la respuesta del foco llega tarde. Se queda lo que trae el listado completo. - Gancho, anuncio de los lectores durante un listado completo: gana el anuncio, y el foco siguiente vuelve a listar.
- Gancho, listado completo que falla con el foco ignorado: queda
failed, y el foco siguiente lo recupera. - Gancho, dos
lookAgainsolapados: un foco que llega después de que acabe el primero sigue ignorado, y gana el último. - Backend, el cruce al revés: la lista en caliente se bloquea leyendo la tarjeta mientras el foco guarda un certificado nuevo de NSS. Cada fila que anuncia el lector se sigue resolviendo con
chosen. - Backend, foco, tarjeta dentro, foco, tarjeta fuera y foco: el asa de la tarjeta se conserva mientras está dentro y desaparece al sacarla.
[note]
relisted_other_than_cardshaceself.last.found().is_none(), que clona el último listado entero solo para saber si existe. Bastaría con unLastListing::is_empty()o algo parecido. No cambia el comportamiento.- En el cruce al revés, la lista en caliente guarda con
keepla parte que no es de tarjeta que leyó antes. Si en ese intervalo apareció un certificado nuevo en NSS, desaparece del último listado y de la ventana hasta el siguiente foco. La ventana ychosensiguen coincidiendo, así que no es un fallo. Es una consecuencia de que el foco es el único que refresca esos almacenes.
|
Arreglados los checks en rojo en 32cfe5c y d32be93: clippy ( |
sgomez
left a comment
There was a problem hiding this comment.
CLEAN
Alcance: incremental, desde la revisión anterior en 572bc8a0 hasta d32be936. Son dos commits: 32cfe5c9, que mete los tres argumentos del último listado de remove_installed en un RemovalListing para que clippy no rechace la función por too_many_arguments, y d32be936, que adapta a ese cambio la prueba de grada B de installed_p12_removal.rs. Lo he revisado contra la spec #2091 y sus sub-issues #2092, #2093 y #2094.
Checks en d32be936: siguen en marcha Cadena Rust, Cadena TypeScript, Imagen nativa, Plataformas / Windows, Plataformas / macOS y los Analyze de CodeQL (actions, java-kotlin, javascript-typescript, python y rust). Alcance y Alcance de Rust están en verde, y Landing y Cadena Java se han saltado. No he corrido la suite en local: el veredicto de clippy y de las pruebas de grada B le toca al CI.
Hallazgos anteriores
Los tres (los dos [blocking] y el [fix]) quedaron arreglados en código en 572bc8a0, y así lo comprobó la revisión anterior. Estos dos commits no tocan readers.rs, useCertificateListing.ts ni IdentityRoot::chosen, así que esos arreglos siguen intactos.
El cambio
Es una refactorización mecánica que no cambia el comportamiento. remove_installed busca primero en installed_copies, si no encuentra nada busca en listed y luego llama a forget sobre el último listado, igual que antes. Están actualizadas todas las llamadas: el adaptador de Tauri, las pruebas de removal.rs, copies.rs y system_install.rs, y la de grada B. grep no encuentra ninguna otra llamada.
Combinaciones
El arreglo no cambia el comportamiento de ningún mecanismo compartido. LastListing se sigue olvidando con la misma llamada y en el mismo momento, así que ninguna combinación revisada antes puede haber cambiado, y no hacía falta ninguna prueba desechable nueva.
Sin hallazgos nuevos.
|
Arreglados los checks en rojo en 19e6cd7: |
sgomez
left a comment
There was a problem hiding this comment.
CLEAN
Alcance: incremental, desde la revisión anterior en d32be936 hasta 19e6cd7f. Es un solo commit, que sube de 66 a 67 la cuenta de the_list_of_commands_is_closed_and_this_is_how_long_it_is (crossing/guards/tests.rs). Lo he revisado contra la spec #2091 y sus sub-issues #2092, #2093 y #2094.
Checks en 19e6cd7f: siguen en marcha Cadena Rust, Cadena TypeScript, Imagen nativa, Plataformas / macOS y CodeQL con sus Analyze (actions, java-kotlin, javascript-typescript, python y rust). Alcance y Alcance de Rust están en verde. Landing, Cadena Java y Plataformas / Windows se han saltado. No he corrido la suite en local, así que el veredicto de las pruebas le toca al CI.
Hallazgos anteriores
Los dos [blocking] y el [fix] de la primera revisión se arreglaron en código en 572bc8a0, y así lo comprobó la segunda. Este commit no toca readers.rs, useCertificateListing.ts ni IdentityRoot::chosen, así que esos arreglos siguen intactos. No queda ningún hilo sin responder.
El cambio
La cuenta es correcta. En origin/main la prueba sigue en 66, y la PR añade exactamente un #[tauri::command(async)], el de list_certificates_other_than_cards en identity/adapters/tauri.rs. Ningún otro fichero del diff contra main añade o quita una orden.
Combinaciones
El commit solo cambia una constante de una prueba. No toca ningún mecanismo compartido, así que no hacía falta ninguna prueba desechable nueva.
Sin hallazgos nuevos.
Delivers spec #2091.
Closes #2091
Closes #2092
Closes #2093
Closes #2094