marykristina4 / Tsoha-kotityosovellus

0 stars 0 forks source link

Vertaisarviointi #2

Open jussmaki opened 3 years ago

jussmaki commented 3 years ago

Vertaisarviointi

Tutustuin sovellukseen herokussa ja kloonamalla repositorion omalle koneelle. Kaikki sovelluksen keskeiset toiminnot on toteutettu. Hyvälle näyttää!

Toiminnallisuuksista

Sovelluksen readmessä kirjoitit

Ajattelin, että parasta olisi, ettei dataa tule liikaa tauluihin, että kun maksan kotityöt, deletoin kotityöt joista palkka maksetaan. Toki maksun voisi merkitä myös jollakin tavalla johonkin tauluun kuitatuksi.

Mitä jos maksuihin liittyvään tauluun lisäisi boolean sarakkeen paid, ja maksun päivänmäärälle timestampin? Maksettuja maksuja voisi sitten säilyttää tietokannassa ainakin jonkin aikaa ja tarvittaessa katsoa sovelluksesta.

En tiedä onko readmessä mainitsemasi kotitöiden hakutoiminnallisuus tarpeellinen sovelluksen käyttämisen kannalta, jos ei ole todella monta jäsentä perheessä. Mutta siitä voisi saada sovellukseen lisää toiminnallisuutta ja SQL-kyselyjä.

Koodista

Sovelluksen koodi kannattaisi ehdottomasti jakaa useampaan tiedostoon. Koodi olisi näin laadukkampaa, eli helpommin luettavaa ja sitä myötä ylläpidettävämpää. Samalla voisi hieman refaktoroida koodia. Single responsibility principle periaatteen mukaisesti, jokaisella luokalla, moduulilla ja funktiolla tulisi olla oma selkeä tehtävänsä. Kun sovelluksen kehitystyössä noudattaa tätä periaattetetta koodin rakenteesta tulee laadukkaampaa.

App.py:stä voisi mielestäni siirtää omaan tiedostoonsa tietokantayhteyden luomisen ja reitit (funktiot joissa käytetään @route dekoraattoria) omaansa. Reittien funktiot voisi lisäksi refaktoroida niin että reittifunktoista SQL-kyselyt ja mahdollisesti sovelluksen toimintaan liittyvää logiikkaa, siirtäisi omaan tiedostoonsa.

Löysin sovelluksesta myös seuraavanlaista toisteista koodia:

    kukapa = session["username"]
    sql = "SELECT id FROM users WHERE username=:username"
    result = db.session.execute(sql, {"username":kukapa})
    kukapaid = result.fetchone()[0]
    sql2 = "SELECT role FROM users WHERE username=:username"
    result = db.session.execute(sql2, {"username":kukapa})
    role = result.fetchone()[0]

Myös tälläiset toistuvat koodinpätät olisi hyvä eriyttää omiksi funktioikseen (tässä tapauksessa vaikkapa funktiot def kukapaid(username: str) ja def role(username: str)), joita sitten käyttäisi näiden reittien funktion alussa. Kun teet näin niin jos joskus päätät muuttaa ko. koodin toimintaa samaa muutosta ei tarvitse tehdä moneen kertaa.

Tietoturva ja lomakkeet

Ensin en meinannut löytää reiteistä mitään tarkistuksia siihen onko käyttäjälle oikeus nähdä sivua vai ei, mutta löysinkin toiminnallisuuden kirjoittettuna sivujen templateihin.

<title>Uusi kotityö</title>
<h1>Uusi tekemätön kotityö</h1>
{% if session.username %}
<p>Olet kirjautunut nimellä {{ session.username }}</p>

{% else %}
<form action="/login" method="POST">
<p>Tunnus:<br>
<input type="text" name="username"></p>
<p>Salasana:<br>
<input type="password" name="password"></p>
<input type="submit" value="Kirjaudu">
</form>
{% endif %}

Tälläinen toiminnallisuus kannattaa ehdottamasti toteuttaa myös omana funktiokutsunaan reitin alussa. Näin sitäkään ei tarvitse kirjoittaa moneen kertaan, jokaiseen käytössä olevaan templateen. Funktio voisi palauttaa totuusarvon ja jos totuusarvo on False, käyttäjä ohjattaisiin kirjautumissivulle.


@app.route("/jotain")
def jotain():
    if user_is_authenticated():
        return redirect("/login")

Myös ihan vain viesti forbidden ja vastaava HTTP-statuskoodi (403) olisi minusta ok return "forbidden", 403

CSFR-haavoittuvuus

En löytänyt sovelluksesta SQL-injektion mahdollisuutta tai XSS-haavoittuvuuksia, mutta huomasin että sovelluksen lomakkeita ei ole suojattu Cross Site Request Forgery hyökkäystä vastaan. En nyt tiedä onko tämä todellinen uhka tälläisessä perheen sisäiseen käyttöön tarkoitetussa sovelluksessa, mutta arvosteluperusteissa mainittiin että sovellus ei saisi sisältää CSFR-haavoittuvuutta. Jotta sovellus ei olisi altis CSFR-hyökkäykselle, kannattaa lisätä lomakkeisiin kurssimateriaalissa opetetulla tavalla CSFR-tokeni.

Samalla kun muuttaa lomakkeita, voisi käyttäjän antamia syötteitä validoita lisää. Sovellusta testatessani onnistuin lisäämän kotityön, johon aikaa meni negatiivinen määrä. Voisi myös lisätä tarkistuksia eri toiminnallisuuksien vaatimille käyttäjärooleille.

Muita huomita

Sovellukseen olisin mielelläni tutustunut enemmänkin ja antanut lisää palautetta, mutta vertaisarvioinnin deadline lähestyy. Mielenkiintoinen projekti oikeaan tarpeeseen. Nyt vain ulkoasua viimeistelemään ja toiminnallisuuksia hiomaan. Suosittelen materiaalissa mainittua bootsrapia ja yhtenäisen layoutin tekemistä blockeilla. Tsemppiä loppukurssille ja hyvää kevään jatkoa!

marykristina4 commented 3 years ago

Moi, Ja kiitos todella paljon huolellisesta vertaisarvioinnista!

Kuten huomasitkin, en ole kauheasti keskittynyt tietoturva-asioihin, koska sovellus on perheensisäiseen käyttöön, ja olen nimenomaan ajatellut niin myös, että jos joku vauva syöttää hassuja numeroita, saan ne kuitenkin korjattua ennen palkanmaksua. Tein kuitenkin sellaisen korjauksen jo, että kestoksi ei voi syöttää kirjaimia ja että mikään valtaisa luku ei mene läpi - tähän on hyvä lisätä tuo että luku ei saa olla negatiivinen! Tarkistan kuitenkin tuon CSFR-token -asian, kiitos kun toit sen esiin!

Olen tuosta app.py:sta kysynyt ohjaajilta labtoolissa nimenomaan siitä miten sitä tulisi jakaa mutta en ole saanut vastausta - teen sen näin ollen varmaankin ehdottamallasi tavalla. En ole ymmärtänyt materiaalista oikein miten importit menee kun tekee tuon jaon ja sen takia olen aristellut sitä - yritän vielä uudelleen.

Kurssilla on tuntunut tosi hankalalta se kun tekee ekaa kertaa tämmöisiä ihan itselle uusia juttuja ja koko jakson aikana ohjaajilta ei saa palautetta ja epäselvien asioiden kysyminen chättäilemällä on tosi hankalaa. Arvostan siis näin ollen todella paljon että näit vaivaa paneutua sovellukseeni ja arvokkaita kommenttejasi. En enää varmaankaan kaikkea ehdottamaasi ehdi tekemään, harmi ettei tämmöistä palautetta ole saanut jo aiemmin. Kiitos sinulle kuitenkin suuresti tästä vaivannäöstäsi.