Reduzierung von Validierungsumfang in DTOs

[Flips01]

Die Nutzung der Validierungsoption in den DTOs sollte auf simple Checks beschränkt bleiben (Regex, Feld A == Feld B etc). Werden Datenbankzugriffe ausgeführt, ist diese Schranke überschritten.

Grund: Werden Stateful-Checks (wie z.B. welche, die DB Zugriffe inkludieren) im DTO-Validator ausgeführt, kann es zu Race-Conditions kommen. Beispielhaft zu sehen beim CreateUserDto. Es besteht keine Transaktion zwischen Check, ob Nutzer bereits existiert und dem tatsächlichen Anlegen des Nutzers in der CreateUser Methode im Controller -> Nur weil die Validierung im DTO ergeben hat, dass der Nutzer nicht existiert, ist dies beim tatsächlichen Anlegen nicht mehr garantiert.

Dies mag zwar im konkreten Fall keine wirkliche Sicherheitslücke darstellen, ist jedoch dennoch ein Design Flaw.

[juliuszint]

Finde diesen Vorschlag sehr sinnvoll, zumal wir auch bei der Entwicklung schon gemerkt haben das es in einer Validierungsannotation nicht ganz so einfach ist an Daten aus der Datenbank zu kommen. Wobei ich Raceconditions nicht tatsächlich als problem sehe da z.B. das der Username Uniqe ist zusätzlich natürlich noch in der Datenbank forciert wird. Sollte somit der fall eintreffen das tatsächlich zwei User den selben Namen verwenden zur gleichen Zeit wird einer davon eine Fehlermeldung angezeigt bekommen. Was ein weiterer Punkt wäre den wir beachten müssten. Fehlermeldungen mit Informationen über interne Abläufe sollten nicht beim User angelangen.

[eloquenza]

Finde diesen Vorschlag sehr sinnvoll, zumal wir auch bei der Entwicklung schon gemerkt haben das es in einer Validierungsannotation nicht ganz so einfach ist an Daten aus der Datenbank zu kommen.

Huh? Es hat doch funktioniert, und so schwer wars nun auch wieder nicht. Beide Varianten, die ich gezeigt habe, hatten Vor- und Nachteile. Aber nicht so einfach? Du hast btw immernoch nichts zur anderen Variante gesagt, @juliuszint

Dies mag zwar im konkreten Fall keine wirkliche Sicherheitslücke darstellen, ist jedoch dennoch ein Design Flaw.

Es ist nen Designflaw, weil wir unsere Applikation nicht wirklich als verteiltes System betrachten. Im schlimmstem Fall ist das zurzeit ein Error, der eigentlich von unserer DB gefangen werden sollte - spaetestens diese sollte sich beschweren. Wie Julius es eben schon gesagt hat. Hier muessen wir einfach nur nen besseres Error Handling einbauen - was wir aber die ganze Applikation eh machen muessen. Ich finde es okay, wenn ein User ein Error deswegen angezeigt bekommt. Zu verhindern ist das naemlich nicht. Vielleicht sollten wir doch als allererstes naechste Woche ueber unsere Architektur und all diese offenen Punkte bzgl Error-Handling, Logging, DB-Transaktionen etc reden.

[juliuszint]

Mit Fehlermeldung meinte ich an dieser Stelle mehr das möglicherweise der Exception Stacktrace angezeigt wird. Ich bin mir dabei aber tatsächlich nicht ganz sicher wie das von Play gehandelt wird wenn in einem Controller eine Exception fliegt und die Anwendung im Release Mode läuft. Das sollte man noch weiter untersuchen.

Dein zweiter Vorschlag ist eine typisierte Version die direkt den Userfinder mit reinbekommt dafür aber in einer eigenen Klasse landen muss. Somit ist meiner Meinung nach auch nicht so viel gewonnen da man halt mehr Code rumfliegen hat (anstatt dem cast). Finde auf jeden fall den Vorschlag hier vom @Flips01 am besten das wir Validierungen die nicht einen simplen Check zugrunde haben sondern Datenbank Zugriff benötigen seperat abhandeln.

[eloquenza]

Seperat heisst fuer euch beiden wo? Illustrieren wir das mal am folgendem Beispiel: Der Admin will einen neuen Nutzer anlegen, allerdings gibt den Username, den er ausgewaehlt hat, bereits in der Datenbank.

Wer soll bei eurem Vorschlag fuer folgendes sorgen:

• Pruefung, ob der Username bereits in der DB existiert,
• Weiterleitung des Fehlers an den User,
• Fehlermeldungs-String,

[eloquenza]

Mit Fehlermeldung meinte ich an dieser Stelle mehr das möglicherweise der Exception Stacktrace angezeigt wird. Ich bin mir dabei aber tatsächlich nicht ganz sicher wie das von Play gehandelt wird wenn in einem Controller eine Exception fliegt und die Anwendung im Release Mode läuft. Das sollte man noch weiter untersuchen.

Exception Stacktrace wird nicht angezeigt, habs gerade getestet. Schoener ist das hier allerdings auch nicht:

[Flips01]

Toben und ich haben es mal implementiert. Im Controller wird Nutzer/Gruppe innerhalb einer Transaktion angelegt. Wird innerhalb der Transaktion festgestellt, dass das Objekt bereits existiert, wird der Request abgebrochen und eine Fehlermeldung an der Entsprechenden Input-Box angezeigt.