dodany REST login

[SebastianCelejewski]

W jaki sposób mógłbym przetestować ten kod? Czy wymagane jest utworzenie bazy danych?

[lukaszfranczuk]

1. Brakuje metody odbierającej dane z aplikacji mobilnej

2. System.out.println("PublicKey:"+clPublicKey+" LOGIN"+login+" PASSWORD:"+password); - Należy użyć Logger-a, system.out.println wypisze dane tylko na konsole
3. List users = userRepository.findByEmailAndPassword(login,password); - metoda powinna zwracać jednego użytkownika i szukać go po loginie (login powinien być unique)
4. Należy sprawdzać czy hash hasła z bazy jest równy hashowi hashła przesłanego przez użytkownika
5. if( users==null && users.isEmpty() ) - warunek nigdy nie zostanie spełniony lub zostanie wyrzucony NullPointerException
6. updateUserToken - powinna to być metoda należąca do modułu krypto (chcemy używać gotowych bibliotek)
7. Dobrze w kodzie używać samo-komentujących się statycznych i finalnych zmiennych, a nie stringów wprost wpisanych w metody (ogólna dobra praktyka)

[SebastianCelejewski]

Zastanawiam się czy warto teraz implementować autentykację, czy lepiej skupić się na funkcjonalności, czyli przesyłaniu danych na serwer.

[mrozim78]

Ma to sens gdy mamy powiązanie loginu czy id urządzenia z protokołem przesłanym. Nie uwzględniliśmy tego wogóle w schemacie bazy.

[waldek18]

hash hasła z bazy jest równy hashowi hashła przesłanego przez użytkownika - hasło będzie zakodowane ? czyli metoda compareTo .

[ProgramerAndroid]

Tak Arrays.equals pytanie czy na początek w ogóle kodujemy hasła, czy robimy wyłącznie transfer danych ?

W dniu 13 listopada 2015 23:14 użytkownik waldek18 <notifications@github.com

napisał:

hash hasła z bazy jest równy hashowi hashła przesłanego przez użytkownika

• hasło będzie zakodowane ? czyli metoda compareTo .

— Reply to this email directly or view it on GitHub https://github.com/openpkw/openpkw-weryfikator/pull/11#issuecomment-156574023 .

[RafalRegula]

Ja uwazam ze powinnismy kodowac wraz z haslami. To praktycznie zadne obciazenie a i tak odlozylismy prace nad crypto. Nie ma co odwlekacz wszystkiego na pozniej.

[pogos]

Mała uwaga do kodu. Zamiast zwracać token z zawartością "ERROR" w przypadku błędu, lepiej użyć bardziej rest'owego sposobu. Zadeklarować typ zwracany z metody jako:

ResponseEntity<Token>

A następnie zwracać w przypadku sukcesu:

return new ResponseEntity<>(token, HttpStatus.OK);

Zaś w przypadku błędu np. FORBIDDEN:

return new ResponseEntity<>(HttpStatus.FORBIDDEN);

Dobrze by było dodać też jakieś testy.

[ProgramerAndroid]

Ok zaimplementowałem to rozwiązanie do apki mobilnej przetestowałem i wszystko działa

W dniu 14 listopada 2015 08:35 użytkownik Sebastian Pogorzelski < notifications@github.com> napisał:

Mała uwaga do kodu. Zamiast zwracać token z zawartością "ERROR" w przypadku błędu, lepiej użyć bardziej rest'owego sposobu. Zadeklarować typ zwracany z metody jako:

ResponseEntity

A następnie zwracać w przypadku sukcesu:

return new ResponseEntity<>(token, HttpStatus.OK);

Zaś w przypadku błędu np. FORBIDDEN:

return new ResponseEntity<>(HttpStatus.FORBIDDEN);

— Reply to this email directly or view it on GitHub https://github.com/openpkw/openpkw-weryfikator/pull/11#issuecomment-156662677 .

[mrozim78]

Tak mi przyszla jeszcze inna rzecz do glowy. Musimy sobie ustalic jakas metodologie budowania ogolnie restow (w senie urle i parametry). Czyli jak przekazujemy id. Jak interpretujemy co jest POST co jest PUT. By kazdy trzymal sie przy nowych restach tych ustalen. Bo tutaj widze ze wiekszosc idzie w @QueryParam ale rownie dobrze uzywac @PathVariable czy wrzucac w POST. Mysle ze to czas ustalic jaka metodologie uzyjemy lub oprzeć się na znanych wzorcach w tym temacie.

[SebastianCelejewski]

Przerzuciłem kod z tego brancha do repozytorium openpkw/openpkw-weryfikator do brancha 'waldek', żeby łatwiej było nad nim pracować.

[SebastianCelejewski]

Hmm, nie jestem przekonany, że kod w tej wersji jest dobrym kandydatem do zmergowania do mastera.

Zastanawia mnie przykładowo ten fragment:

public String register(@RequestBody UserRegister userRegister) {
    User user = userRepository.findByEmailAddress("fdsaf");
    System.out.println("L:" + userRegister + ", user:" + user);
    if (userRegister == null || userRegister.isEmpty()) {
        return "ERROR";
    }

(...) }

[mrozim78]

Patrzylem na ta czesc kodu co trzeba by poprawic:

1. Wyszukiwanie na sztywno "fdsaf"
2. Zwracanie sensownych kodow HTTP
3. Zwracanie jednak jsona w wyniku wykonania rejestracji a nie string z bledami

[SebastianCelejewski]

Wziąłem to na warsztat i zaraz wrzucę wersję, która ma parę rzeczy poprawionych. Np. zwracanie błędów przy pomocy kodów http itd.

[SebastianCelejewski]

No i od razu parę testów akceptacyjnych wrzucę.

[SebastianCelejewski]

Tutaj będę wrzucać moje zmiany: https://github.com/openpkw/openpkw-weryfikator/tree/waldek

[SebastianCelejewski]

A testy tutaj:

https://github.com/openpkw/openpkw-weryfikator/blob/waldek/openpkw-test/src/test/java/org/openpkw/weryfikator/rest/login/When_registering_new_user.java

[SebastianCelejewski]

A testy jednostkowe tutaj: https://github.com/openpkw/openpkw-weryfikator/tree/waldek/openpkw-rest/src/test/java/org/openpkw/web/validation

[SebastianCelejewski]

Jest jeszcze jedna rzecz, której nie rozumiem. Piszę testy dla metody login i nie bardzo wiem jak ona ma działać. Przesyłane są następujące dane:

• e-mail
• password
• client public key
• device id

Które pola są wymagane? Na podstawie których ma być wykonywana autentykacja? Email i hasło? Co to jest client public key?

[SebastianCelejewski]

Sorry za zalew pytań, ale mam jeszcze jedno. Z kodu wynika, że kiedy się woła metodę /login i adres e-mail użytkownika nie został znaleziony w bazie danych, to użytkownik zostanie utworzony automatycznie. Czy taki był zamysł? Jeśli tak, to do czego służy metoda /register?

[ProgramerAndroid]

Z tego co rozmawiałem z Waldkiem to autentykacja miała się odbywać na podstawie adres email i hasło, ponieważ loginu nie ma w tabeli User. Clint public key to klucz publiczny klienta RSA, który ma być wykorzystany do zaszyfrowania tokenu- klucza sesji. Metoda register miała służyć do rejestracji nowych użytkowników wyłącznie.

W dniu 1 grudnia 2015 09:01 użytkownik Sebastian Celejewski < notifications@github.com> napisał:

Sorry za zalew pytań, ale mam jeszcze jedno. Z kodu wynika, że kiedy się woła metodę /login i adres e-mail użytkownika nie został znaleziony w bazie danych, to użytkownik zostanie utworzony automatycznie. Czy taki był zamysł? Jeśli tak, to do czego służy metoda /register?

— Reply to this email directly or view it on GitHub https://github.com/openpkw/openpkw-weryfikator/pull/11#issuecomment-160888394 .

[mrozim78]

No a ta obsługa błędów w postaci statusów http ? No i czemu zwracany jest string z metody ? Nie lepiej było to opakować w jakiegoś jsona ?

[ProgramerAndroid]

To pytanie do Waldka obydwie metody miały zwracać jsona. Moim zdaniem w metodzie login w przypadku niepoprawnego adresu email lub hasła należy wykorzystać bardziej RESTowe rozwiązanie i zwrócić kod błędu np. 401 lub 403.

W dniu 1 grudnia 2015 10:21 użytkownik Mrozi notifications@github.com napisał:

No a ta obsługa błędów w postaci statusów http ? No i czemu zwracany jest string z metody ? Nie lepiej było to opakować w jakiegoś jsona ?

— Reply to this email directly or view it on GitHub https://github.com/openpkw/openpkw-weryfikator/pull/11#issuecomment-160905122 .

[SebastianCelejewski]

W implementacji, nad którą pracuję, obie metody zwracają kody HTTP jako kody błędu, a także zwracają JSONa zawierającego szczegółowy kod błędu i message. Metoda login zwraca token równiez w JSONie.