Enregistrer et restaurer automatiquement les données du formulaire (avec chiffrement) #58
Conversation
tar-gezed
changed the title
|
@tar-gezed Je pense qu'il y a besoin d'un rebase car il y a des commits en retard par rapport à l'upstream, notamment une feature d'heure automatique qui a été ajoutée entre temps (a2566e8) qui pourrait potentiellement rentrer en conflit avec cette PR. Peut-être qu'il faudra adapter cette partie après rebase. (Si tu veux, tu peux m'ajouter en contributeur sur ton fork comme ça je pourrais t'aider à maintenir ta branche directement ;)) |
|
@theblackhole Je me suis rebase sur la branche main qui semble être la nouvelle branche par défaut, mais bon c'est un peu flou j'avoue. Sinon oui pas de soucis je t'ajoute en tant que contributeur sur la branche ;) EDIT: J'avais pas vu y'as en effet des changes en plus, j'ai pas le temps de regarder ça ce soir, je t'ai donné les droits sur mon repo en principe si tu veux t'amuser ce soir ^^ |
tar-gezed
force-pushed
the
feature/store-and-autocomplete-data
branch
from
88b3622 to
b2f2438
Compare
|
Prêt à être mergé, c'est rebase sur la dernière version, j'ai fix quelques bugs et j'ai supprimé le code qui n'était plus nécessaire 😉 |
|
Pardonnez moi, mais quel est l'intérêt de chiffrer les données que l'on stocke dans LocalStorage... Si une personne malveillante à accès au localStorage, il aura alors accès à la clé, et donc aura la capacité de tout déchiffrer. |
Après review, tout me semble bon en effet. 👍 J'ai donc mis à jour mon instance de demo avec le travail que tu as fait @tar-gezed : https://autocovid19.oa.r.appspot.com/
@nyroDev En faisant le choix de cette lib, je n'ai pas cherché très loin : je me suis simplement dit que c'était un peu mieux que de stocker les données en clair sur l'appareil de l'utilisateur. La clé de secure-ls est, certes, accessible, mais n'est pas stockée en clair : il faut aussi savoir comment la décoder. L'intéret n'est donc pas d'avoir un truc ultra-sécurisé (pour ça, il faudrait probablement une communication avec un serveur backend), juste de compliquer la vie d'éventuelles personnes malveillantes... Je vois ça un peu comme un grillage : ça n'empêche pas un intrus de rentrer mais ça lui complique un peu la vie car il faudra qu'il passe par dessus |
| En application du décret n°2020-1310 du 29 octobre 2020 prescrivant les mesures générales | ||
| nécessaires pour faire face à l'épidémie de Covid19 dans le cadre de l'état d'urgence sanitaire | ||
| En application du décret n°2020-1310 du 29 octobre 2020 prescrivant les mesures générales | ||
| nécessaires pour faire face à l'épidémie de Covid19 dans le cadre de l'état d'urgence sanitaire |
There was a problem hiding this comment.
| nécessaires pour faire face à l'épidémie de Covid19 dans le cadre de l'état d'urgence sanitaire | |
| nécessaires pour faire face à l’épidémie de Covid19 dans le cadre de l’état d'urgence sanitaire |
There was a problem hiding this comment.
Je pense que cette suggestion de modification de caractère mérite une PR séparée car il n'y a pas de lien avec la fonctionnalité apportée par l'actuelle.
| value="storedata"> | ||
| <label class="form-check-label" | ||
| for="field-storedata"> | ||
| Stocker mes données sur l'appareil afin de remplir automatiquement mes futures attestations |
There was a problem hiding this comment.
| Stocker mes données sur l'appareil afin de remplir automatiquement mes futures attestations | |
| Stocker mes données sur l’appareil afin de remplir automatiquement mes futures attestations |
|
Merge obligatoire de cette feature !!! |
|
Je suis d'accord avec @nyroDev : cela donne un faux sentiment de sécurité, ce qui à mon avis est pire que de ne pas avoir de sécurité et d'en être conscient. Il suffirait à un attaquant d’exécuter Ceci dit, le site n'utilise aucun JS externe. A priori, le risque viendrait donc de l'ordinateur infecté lui-même, et la protection des données de ce site est une goutte d'eau dans l'océan des conséquences. Je ne pense pas qu'on puisse y faire quelque chose. Bref, si l'utilisateur.rice a le choix de stocker ou non les infos, ça me semble suffisant comme sécurité. |
|
En plus d'être inutile, le mécanisme de chiffrement ajoute une dépendance supplémentaire ce qui est un peu dommage. |
- raw copy from LAB-MI#58 - simplify text
- raw copy from LAB-MI#58 - simplify text
- raw copy from LAB-MI#58 - simplify text
👍 Moi, je prefer un solution plus simple (sans chiffrement.... comme #43) car chiffrement dans En géneral, si il y a un attaque XSS, c'est extrêment facile à wrapper les invocations avec secure-ls pour obtenir les données. |
- raw copy from LAB-MI#58 - simplify text - fix genrated time to out time
|
Je plussoie le fait de ne pas chiffrer les données dans le local storage, c'est contraire au principe KISS et ça n'apporte pas de bénéfice de sécurité comme l'ont dit d'autres plus haut. Par contre si vraiment tu veux chiffrer les données @tar-gezed , pas besoin d'une lib externe. Il existe des API de web crypto qui fonctionnent bien: https://developer.mozilla.org/en-US/docs/Web/API/Crypto/subtle |
Remove add current time devSwoop code added before (not relevant with this new version)
Co-authored-by: Julien Waechter <[email protected]>
Co-authored-by: Julien Waechter <[email protected]>
theblackhole
force-pushed
the
feature/store-and-autocomplete-data
branch
from
97bf71a to
eba728a
Compare
|
ℹ️ Rebase effectué suite aux changements de la master (et github n'a pas trop l'air d'aimer ça pour tout ce qui est review hahaha) |
|
Top! Pour la prochaine fois, pour éviter de rebase (qui effectivement supprime l'historique des commits donc de certaines reviews) tu peux merger |
|
Personnellement j'ai un peu horreur des "merge remote tracking branch", quand il faut visualiser l'arbre, ça rend l'historique difficile à lire (J'ai été traumatisé quand il a fallu debugger un gros projet et chercher le commit qui a introduit le bug était comme chercher une éguille dans une botte de foin. Heuseusement que le git bisect m'avait aidé). Je préfère m'en tenir au rebase pour mettre à jour par rapport à l'upstream (et garder un historique linéaire) et au merge pour l'ajout de fonctionnalités :) |
|
Bon j'ai l'impression qu'ils ont backport la feature eux même au final, cette PR peut être fermée je pense ^^ |
|
Je crois aussi, c'est dommage après tout votre travail, c'est vraiment une bonne PR... 😥 |
|
L'intégration a été faite brutalement 😅 Mais bon, l'essentielle c'est que la fonctionnalité est enfin dispo, c'est top 👍 🎉 |
|
Est-ce que LAB-MI prend vraiment en compte les contribution externes? Ca n'a pas trop l'air de les intéresser? |
|
Pour les petites corrections, au moins. Le reste, je ne sais pas (j'espère !) |
|
Et bien après avoir proposé une PR (en attente) + demo sur l'attestation couvre-feu puis celle-ci (et toutes les autres qui ont été proposés comme #43 par @0xCAFEADD1C7 , #69 par @jak78 , #74 par @kairos666 , #90 par @tubededentifrice , #113 par @gjgd , #121 par @Punkte ), quel retournement de situation ! Ils ont vraiment tout repris dans 78b122d , même la petite refacto de la snackbar ! Je ne sais pas si c'est un manque de connaissances (ils ne savent pas qu'ils peuvent directement apporter des modifications à la PR avant de merge ? que s'ils y a trop de commits on peut les squash pour n'en faire qu'un seul si besoin ? ça pourrait aussi expliquer aussi la création de tous ces repos au lieu de créer des branches ou des forks sur le générateur d'attestation original) ou si c'est volontaire (pas envie de tracer l'historique jusqu'à cette PR car on y discute de la possibilité de détourner la sécurité apportée par secure-ls ?). Dans tous les cas, le principal est bien que cette fonctionnalité soit enfin disponible à tous afin d'améliorer le quotidien des utilisateurs 😃 |
T'inquiète ils ont bien repris le travail effectué, c'est juste qu'ils ont fait un copier coller au lieu d'un merge haha |
|
Très déçu par leur manière de faire les choses... 😕 |
|
Ça peut aussi être de l'incompétence, on ne sait pas |
|
Formellement c'est un plagiat. Dommage pour un ministère. |
C'est dégueu de faire ça, mais je ne pense pas que ce soit un plagiat... puisque c'est open source. |
|
Oui et le projet est sous licence MIT qui permet a chacun de faire ce qu'il veut. Ils ont tout a fait le droit de faire ça contrairement à d'autres licences Open Source plus restrictives
|
Bon on dirait que c'est bien de l'incompétence. Regardez moi ce magnifique commit dont les changements n'ont rien à voir avec le message 😂 bf7a6d0 |
|
Oui la fonctionnalité est disponible pour tout le monde, c'est ce qui compte. Merci aux contributeur.rice.s. Pour cellui qui veut perdre son temps dans des considérations philosophiques, je prétend que c'est du plagiat. Ce projet n'a pas de Bon on est bien d'accord qu'on ne parle que de quelques lignes. Mon vrai problème, c'est qu'un ministère, et en fait à travers lui l'État, fait une N-ième fois preuve d'un mépris du peuple, avec une absence totale de dialogue. C'est dommage, parce que j'ai été agréablement surpris de voir que le code était sur GitHub. Woua l'administration se modernise et propose un projet de co-construction participative dans un contexte de crise multifactorielle et je passe pleins d'autres buzzwords ! En fait non, c'est que du vent. Pas un signe de leur part, pas un merci. |
|
Je suis d'accord, mais selon mes souvenirs la license MIT n'oblige pas à citer les auteurs. De toute manière, je ne sais pas si ça inclut les PR. Enfin bon -- la fonctionnalité est maintenant disponible. Ils apprendront peut être de leurs erreurs. |
Encore faudrait-il que ces commentaires soient lus |
Oui la MIT est la plus permissive de toutes elle dit simplement "faites ce que vous voulez avec le code tant que la licence est incluse mais dans tous les cas je ne donne aucune garantie et vous ne pouvez pas m'attaquer en justice s'il y a des problèmes" Mais on met le cadre juridique de côté, que ce soit fait exprès ou non, oui c'est clairement du plagiat et du mépris des contributeurs pour le temps passé gratuitement à améliorer le projet. D'ailleurs , @betalabmi , à qui je dois envoyer la facture des 0.26€ pour l'hébergement de mon instance de demo ? 😄 |
Donc si, la notice de copyright doit être incluse. Et de toutes façons en France le droit au nom est inaliénable. |
Bref, dans tous les cas la PR n'est pas sous license, le repo l'est. |
Un code, quel qu'il soit, doit être fourni sous une licence, quelle qu'elle soit, sinon on ne peut pas l'intégrer, le droit d'auteur s'applique (sauf si les modifications sont triviales). L'usage est effectivement qu'on considère que la PR est soumise sous la même licence que le fichier modifié, mais ça n'est pas complètement fortuit. |
Reprise du travail fait par @theblackhole (LAB-MI/attestation-couvre-feu-covid-19#4)