search

rinfo / lagrummet.se

www.lagrummet.se
6 stars 0 forks source link

En redaktör kan inte ändra sitt lösenord. #259

Open lalindqv opened 9 years ago

lalindqv commented 9 years ago

Det är inte möjligt för en redaktör att ändra sitt eget lösenord, vilket det borde vara.

kamidev commented 9 years ago

Borde det verkligen det? Om vi har en distinktion mellan admin och redaktör är det inte självklart. Kanske bör vi diskutera det här kravet.

lalindqv commented 9 years ago

Om två eller fler personer känner till lösenordet för samma konto går ju autenticeringen förlorad. Jag skulle själv inte vilja att någon annan kände till mina lösenord (mer än väldigt temporärt, tills man loggat in första gången tex).

karejonssondov commented 9 years ago

Implementerat i featuregrenen 259_profileAdmin

Man får en rullgardin till vid namn "profil". I denna implementering finns två svagheter.

  1. En hackig lösning på dubbelhashning har kladdat i secuser-klassen.
  2. Använder inte lösenordsfält utan synliga fält.

Rörande punkt 2: Så är det på andra ställen så jag ändrar inte det nu.

jeldeklint commented 9 years ago

Till att börja med, använd #<ärendenummer>-notation för att länka commits till ärenden, (alternativt, om fel ärende / glömt och redan pushat (annars använd git commit --amend)) eller länka commits till inlägg genom att klippa in commitens hash, t.ex. 1cb3889.

Att lyfta användar id från params när man bara ska kunna ändra sin egen data är inte okej. Finns inte något direkt som hindrar att man ändrar eller lägger till det fältet.

Förövrigt, vi kan par-programmera det här och refaktorera endel grejer sen, när det finns tid. Kan vara värt att läsa lite om hur groovy skiljer sig från Java, http://groovy.codehaus.org/Differences+from+Java == är samma som equals i groovy, dvs, det går att använda på komplexa typer / strängar i groovy tillskillnad mot java som jämför objektreferens (även om det kan råka bli rätt p.g.a string pooling..)

Vill man använda compareTo så skrivs det <=> istället.

karejonssondov commented 9 years ago

Betyder det att

def userInstance = (params.id) ? User.get(params.id) : User.get(springSecurityService.principal.id)

skall vara

def userInstance = User.get(springSecurityService.principal.id)

?

jeldeklint commented 9 years ago

Jepp! springSecurityService.principal.id ger dig id:t på den inloggade användaren. Hinner användaren logga / time:a ut blir principal null tror jag. Risken är väl rätt liten att man ska göra det "samtidigt"..

karejonssondov commented 9 years ago

Fixade det och lite annat. Kan vi överväga att ta in? @kamidev ?

jeldeklint commented 9 years ago

Först, läs https://help.github.com/articles/writing-on-github/#references

Kodmässigt, några saker som syns direkt - CSRF tokens används i formulären men valideras aldrig i backend, felmeddelanden innehåller referenser till params.id som inte verkar existera längre. Det finns ingen koll om save(...) lyckades eller ej (knappast vanligt förekommande problem, men det går att framkalla problem genom att ända samma användare samtidigt)

Jag för högvis med meddelanden som inte är mappade i default messages.properties. Har de grejerna någonsin fungerat?

Finns det någon orsak att inte använda lösenordsfält? (se t.ex. Arons rapport..) Att under Visa visa vilken lösenordshash en användare har känns... onödigt. Den borde aldrig visas, varken här eller på annat håll.

Sen rent stilmässigt, tomma if statements? finns det någon bra orsak, speciellt när det är om fälten inte är samma vi vill testa?

jeldeklint commented 9 years ago

En helt annan grej, enl. Arons rapport bör vi byta hashmetod och använda salt (en självklarhet egentligen). Lämpligt vore att använda bcrypt enl. http://grails-plugins.github.io/grails-spring-security-core/guide/passwords.html

bcrypt i springsecurity hanterar såvitt jag vet saltet internt så vi slipper bry oss om det..