java-oauth2-server prośba o wnioski

0

Witam, bardzo proszę osoby które znajdą troche czasu o code review :
(https://github.com/CienZZZ/java-oauth2-server/tree/users)

wszelkie uwagi, sugestie, obelgi , itp. mile widziane :)

szczególnie proszę o informacje czy taki model, podziały, metody jest dobry itp.

testów nie napisałem jeszcze, potem będą

korzystałem z różnych stron, poradników itp. i coś czuje że przedobrzyłem :)

1
  1. Nie ma README, wiec nie wiadomo co oceniamy
  2. Bez testów się nie liczy
  3. Już widzę podział pakietowy na warstwy (łeb, serwajsy i dało), czyli słabo
1

Już widzę podział pakietowy na warstwy (łeb, serwajsy i dało), czyli słabo

@Charles_Ray i co w nim niby złego? Jak dla mnie taki podział w mikroserwisie jest zupełnie poprawny. Jak chcesz tam coś dzielić "domenowo" skoro masz mikroserwis i jedną "domenę"? Taki podział jaki sugerujesz to sie nadaje do monolitu. Widać że nawet nie otwarłeś tego kodu. Co ty chcesz tam dzielic jak jest jeden kontroler, jedno dto, jedno repozytorium xD

2

To zależy jak duży jest mikroserwis, ale zgadzam się, że zależy to od konkretnego przypadku.

1

@Shalom: ale własnie mając jedną domene mała, ten podział też niewiele wnosi :) Masz dzięki temu pełno pakietów po jednej-dwie klasy. Imo to tylko przeszkadza

2

@danek: co w zamian, wrzucic wszystko do src/main/java? ;)

Jesli ktoś bardzo chce dzielić to ja osobiście polecam dwupoziomowe rozwiązanie, które sam stosuje od dłuższego czasu:

  1. Podział serwisu na moduły mavenowe/gradlowe, przynajmniej te, ale często więcej w miarę potrzeby (np. repozytoria też jako osobne moduły, metryki, testy integracyjne, api/interfejsy)
  • część aplikacyjno-serwerowa -> zwykle coś związanego z frameworkiem -> configuracja ioc, kontrolery etc.
  • client -> np. ładnie opakowany http client + DTOsy do używania tego serwisu z innych aplikacji (tak zeby nie trzeba było tego pisać w każdym serwisie od nowa, hardkodować ścieżek itd)
  • część domenowa -> klasy domenowe bez jakichkolwiek zależności od frameworków, taka logika którą można by wyciągnąć "w całości" i użyc w innym miejscu
  1. Każdy taki moduł jest podzielony na pakiety względem encji/domeny, o ile ma to sens (jak mam 2 kontrolery to nie będę dla nich robił osobnych pakietów).

Ten podział na moduły na taki plus, że oczywiscie można je wziąć sobie jako zależności -> możesz sobie w innym serwisie wrzucić zależność na clienta i wygodnie używać tego serwisu. Możesz też np. wziąć całe domain i zbudować nową aplikacje opartą o tą samą logikę (np. stojącą na jakimś hipsterskim frameworku zamiast springa). Warto o takich rzeczach też myśleć jak robisz np. jakieś interfejsy. Często ludzie robią interfejs i obok niego wrzucają implementacje i nasuwa sie pytanie jaki to ma sens. Taki interfejs powinien być w jakimś module api gdzie są same interfejsy i dtosy z nimi związane...

0

Dziękuje za rozjaśnienie jak powinna wyglądać struktura projektu. Docelowo na pewno będzie tam więcej niż tylko User. Jakieś produkty, adresy itp.
Dorzuciłem dzisiaj kilka testów do klasy Usera. Jakby ktoś znalazł chwile to może rzucić okiem na kod z Security? Czy to Oauth2 dobrze zaimplementowałem ?
Czy macie jeszcze jakieś inne cenne rady ?

2
  1. Nadaj testom strukturę given/when/then oraz nazwy zgodne z konwencją should... - będzie czytelniej
  2. W testach podnosisz kontekst webowy, zupełnie niepotrzebnie
  3. Nie widzę żadnych testów na "core" Twojej domeny czyli uwierzytelnianie i autoryzację. W którym miejscu "implementujesz OAuth2"?
  4. Nie rozumiem, dlaczego po każdym teście zapisujesz do bazy admina. On jest wymagany zawsze czy tylko w konkretnym przypadku. Nie jest to jasne.
  5. Uwagi stylistyczne:
  • zakomentowany kod nie powienien być w repo
  • klasy nie powinny mieć sufiksu Impl, ponieważ nic on nie wnosi
  • używaj importów, widzę jakieś java.util.List
0
Charles_Ray napisał(a):
  1. Nadaj testom strukturę given/when/then oraz nazwy zgodne z konwencją should... - będzie czytelniej
  2. W testach podnosisz kontekst webowy, zupełnie niepotrzebnie
  3. Nie widzę żadnych testów na "core" Twojej domeny czyli uwierzytelnianie i autoryzację. W którym miejscu "implementujesz OAuth2"?
  4. Nie rozumiem, dlaczego po każdym teście zapisujesz do bazy admina. On jest wymagany zawsze czy tylko w konkretnym przypadku. Nie jest to jasne.
  5. Uwagi stylistyczne:
  • zakomentowany kod nie powienien być w repo
  • klasy nie powinny mieć sufiksu Impl, ponieważ nic on nie wnosi
  • używaj importów, widzę jakieś java.util.List

ad.1 ok
ad.2 musze poczytać o tym, nie do końca rozumiem o co Ci chodzi
ad.3 testów na "core" jeszcze nie zrobiłem. Wszystko jest w paczce "config"
ad.4 tylko raz tworze na koniec testów klasy Usera, przynajmniej tylko raz powinno być to wykonane według kodu
ad.5 ok, co do 'java.util.List' bo wcześniej już użyłem w tej klasie Listy z 'vavr' :)

1

Package by feature - jak Ci dojdzie więcej rzeczy to będzie Ci łatwiej się odnaleźć, zamiast przyrównywać usera do nulla w UserServiceImpl możesz użyć ifPresentOrElse(), możesz też pomyśleć nad @ControllerAdvice, spring.jpa.hibernate.ddl-auto powinno być na validate a nie update - chcesz sprawdzić czy te encje wygenerowane przez kod javy zgadzają się z tym co masz w plikach .sql, podepnij sobie jakąś h2 do testów lub embedded postgresa

0

@Charles_Ray: tabele Oauth2 tworzy w bazie danych skrypt z folderu "migration" z Resources, a całe Security jest w paczce "config" tak jak wcześniej pisałem.
Lepiej jest robić tak jak teraz przez ten skrypt tabele, czy zrobić Encje w kodzie i nie korzystać z skryptu ?

0

Ok, ustalmy jedną rzecz. Co ma robić ten projekt? Co tu jest Twojego, a co jest opazdzierzane gotową konfiguracją?

0

Ogólnie ma to umożliwiać komunikacje danych z bazy danych dla frontu i aplikacji Androidowej. Program ma działać na zasadzie magazynu, czyli użytkownicy, produkty, lokalizacje itd. Na froncie jakieś rzeczy do raportowania. Na początek chciałem zrobić obsługę użytkowników, logowanie, autoryzację. I gdy to będzie gotowe, mogę dalej dodawać kolejne części.
Ogólnie jeśli chodzi o kod to korzystałem z różnych stron, kursów, tutoriali. Wszystko co w paczce "config" ma za zadanie dawanie bądź nie dostępu, większość tego kodu nie jest moja, tylko niewielkie części tam zmieniłem. Typu tokeny są przechowywane w bazie danych (nie wiem czy to dobre rozwiązanie, ale mi odpowiada), ClientDetails jest inMemory (bo po co trzymać to w bazie ?). W ResourceServerConfig zezwalam na uruchomienie Swaggera i użytkownikom którzy się zalogują, reszta ma nie mieć dostępu.

0
  1. Jakie tokeny są trzymane w bazie? O_o
  2. Ja bym raczej zostawił security na później, a zaczął od funkcji przechodzących przez wszystkie domeny. W momencie kiedy odpracujesz „moduł użytkownika” tak naprawdę aplikacja ma dla użytkownika końcowego zerową wartość - może się zalogować i tyle, nie po to powstaje cała apka. Ale to Ci przyjdzie z doświadczeniem.
  3. UserService i UserServiceImpl. Myślmy. Po co Ci ten interfejs? Co Ci daje sufiks „Impl”?
0
Charles_Ray napisał(a):
  1. Jakie tokeny są trzymane w bazie? O_o
  2. Ja bym raczej zostawił security na później, a zaczął od funkcji przechodzących przez wszystkie domeny. W momencie kiedy odpracujesz „moduł użytkownika” tak naprawdę aplikacja ma dla użytkownika końcowego zerową wartość - może się zalogować i tyle, nie po to powstaje cała apka. Ale to Ci przyjdzie z doświadczeniem.
  3. UserService i UserServiceImpl. Myślmy. Po co Ci ten interfejs? Co Ci daje sufiks „Impl”?

ad.1
Access i Refresh tokeny
ad.2
chciałem właśnie zrobić to od tej drugiej strony, czyli potrenować z security i logowaniem :)
ad.3
czyli najlepiej wywalić ten interfejs, zostawić klase UserService i metody publiczne lub jeśli będę mógł package-private ? (albo tak jak @Skoq wspomniał fasade zrobić do tego ?)

1

Ad. 1) ok, aczkolwiek teraz modne są JWT
Ad. 2) ok ;)
Ad. 3) niech na razie ten UserService będzie taką "fasadą". Jak się rozrośnie to zrefaktorujesz sobie do pomniejszych serwisów domenowych. Zacznij od package-private, jeśli będzie trzeba to może tutaj być public (bo jest to fasada). Testy powinny dotykać tylko UserService, możliwie żadnego z wydzielonych komponentów.

1

Ogólnie podejście z package-private i wystawienie takiej np. Fasady to krok w przyszłość. Potem nie będziesz miał problemu żeby z moduliku w projekcie zrobić mikroserwis. Dzięki takiemu podejściu chowasz sobie całą logike u siebie, a wystawiasz na zewnątrz tylko jedną klase, która udostępnia Ci już gotowe zachowania i nie widzi tego co się dzieje w środku, a jedynie gotowe rozwiązanie.

0
@Transactional
public LocationDTO editLocation(long locationId, String newCode) {
        final Optional<Location> location = this.locationRepository.findById(locationId);
        return location.map( l->{
            if (!newCode.matches("\\w{3}-\\w{3}-\\w{2}-\\w{2}")){
                throw new NotValidPattern("\\w{3}-\\w{3}-\\w{2}-\\w{2}");
            } else {
                l.setCode(newCode);
                return l.toLocationDTO();
            }
        }).orElseThrow(
                ()-> new NoLocationException(locationId)
        );
    }

Proszę o wytłumaczenie, ta metoda aby działała z @Transactional musi być publiczna. Czyli jak mam taka klase z takimi metodami, to wszystkie musza być publiczne i nie moge dać na nich package-private. Co w takim przypadku z podejściem package-private ?

0

a czy mogę prosić o jakiś dobry przykład na czasie testów jednostkowych i integracyjnych ? bo co spojrzę na jakieś githuby itp to każdy inaczej ;)
może znacie jakiś dobry github czy coś innego ?

1

@Transactional powinno być używane tam gdzie konieczne. W powyższym przykładzie nie ma konieczności użycia tej adnotacji.Tutaj leci update tylko na 1 obiekcie, którego nowa wartość nie jest uzależniona od stanu innego obiektu. Jak poleci rollback to trudno.
Co do architektury heksagonalnej - czy często? Nie wiem. W projekcie mamy w jednym mikroserwisie gdzie logika jest dość rozbudowana i sprawdza się tam bardzo dobrze. Jak nie masz dużo logiki to zamiast strzelać z armaty do wróbla lepiej użyć zwykłą wielowarstwową controller-service-repo :)

0

mam jeszcze jedno pytanie, te testy co tam napisałem, dać je sobie na baze H2 in memory czy robić to z MockUser ?
myśle żeby testy jednostkowe serwisu zrobić właśnie na bazie H2 in memory, a integracyjne testy kontrolera przez Mocki

2

Moment, pytasz o całkiem różne rzeczy :P Po pierwsze te testy co widzę są całkowicie do wywalenia. Tam chyba próbujesz jednostkowo coś przetestować i nie wiem dlaczego cały kontekst podnosisz. Jak nauczysz się pisać testy integracyjne to zaciągnij sobie dependency H2 i sprawdzaj czy faktycznie odpowiednie obiekty są zapisywane. Chodziło Ci o adnotację @WithMockUser? Ona Ci mockuje usera żeby security przy integracyjnych Ci nie robiło problemu.

myśle żeby testy jednostkowe serwisu zrobić właśnie na bazie H2 in memory, a integracyjne testy kontrolera przez Mocki

Chyba na odwrót ;p

2

Nie mockuj, testując integracyjnie chcesz operować na Twoich prawdziwych obiektach (chociaż spring i tak tam swoje proxy wepcha ale nvm). Z racji ze masz tak naprawdę tylko prostą logikę crudową zrezygnowałbym z testowania jednostkowo a skupił się na napisaniu dobrych testow integracyjnych. Stwórz sobie jakas metode pomocniczą, ktora bedzie populować Ci bazkę np. przed uderzaniem pod endpoint do wyciagania wszystkich userow a pozniej sprawdz czy zwracany json to faktycznie userzy, ktorych wsadziles do bazy ;) I analogicznie reszta endpointow.

0

czyli takie testy nie ?

@Test
    void accessHaveOnlyAdmin() throws Exception {
        final String accessToken = obtainAccessToken("admin", "admin");

        mockMvc.perform(post("/users/all")
                .header("Authorization", "Bearer" + accessToken))
                .andExpect(status().isOk());

        final String accessTokenUser = obtainAccessToken("Krzys", "1234");

        mockMvc.perform(post("/users/all")
                .header("Authorization", "Bearer" + accessTokenUser))
                .andExpect(status().isForbidden());
    }

W przykładzie który podałeś ten serwis jest jakaś prosta przelotka, on po prostu zleca zapisanie obiektów do repozytoriów (przynajmniej tak wynika z przypadków testowych). Zobacz natomiast, że sam serwis tworzony jest przez new i podobnie możesz zrobić z repozytoriami - tzn podstawić własne implementacje testowe, np. in-memory. Wtedy nie potrzebujesz stawiać kontekstu Springa. - Charles_Ray

mogę prosić o jeden przykład takiego poprawnego testu ?

@Skoq też mogę prosić o jeden przykład testu ?

1

Zamiast czegoś takiego możesz po prostu nad metodą testującą dodać coś w stylu:

@WithMockUser(username = LOGIN, roles = RoleUtils.ADMIN_ROLE)

Ja Ci podałem po prostu tak ogólnie co powinieneś testować, jak masz jakiś endpoint zabezpieczony to wiadomo, że sprawdzasz. Co do kodu może uda mi się znaleźć coś w junit5 ale nie obiecuję ;)

0

Witam ponownie, staram się dochodzić do wprawy z testami. Wzorując sie na tym co mi podpowiedzieliście, książką i innymi https://phauer.com/2019/modern-best-practices-testing-java/
https://phauer.com/2019/focus-integration-tests-mock-based-tests/
tworzę testy integracyjne dla UserControllera.
Czy taki test jest akceptowalny :

@Test
    void newUserCreated() throws Exception {
        mockMvc.perform(post("/users/new")
                .header(AUTHORIZATION, BEARER + obtainAccessToken(USER_ADMIN,ADMIN_PASSWORD))
                .contentType(CONTENT_TYPE)
                .content(NEW_USER_JSON)
                .accept(CONTENT_TYPE))
                .andExpect(status().isCreated())
                .andExpect(content().string(containsString(USER_NEWUSER)));
    }

czy może tak:

@Test
    void newUserCreated() throws Exception {
        MvcResult result = mockMvc.perform(post("/users/new")
                .header(AUTHORIZATION, BEARER + obtainAccessToken(USER_ADMIN,ADMIN_PASSWORD))
                .contentType(CONTENT_TYPE)
                .content(NEW_USER_JSON)
                .accept(CONTENT_TYPE))
                .andExpect(status().isCreated())
                .andReturn();
       String responseJson = result.getResponse().getContentAsString();
       assertThat(responseJson).contains("Krzys");
    }

lub pełen test z wykorzystaniem serwisu:

@Test
    void newUserCreated() throws Exception {
        mockMvc.perform(post("/users/new")
                .header(AUTHORIZATION, BEARER + obtainAccessToken(USER_ADMIN,ADMIN_PASSWORD))
                .contentType(CONTENT_TYPE)
                .content(NEW_USER_JSON)
                .accept(CONTENT_TYPE))
                .andExpect(status().isCreated())
                .andExpect(content().string(containsString(userService.getUserByName(USER_NEWUSER).getName())));
}

albo w jakiś inny sposób taki test powinien być napisany ?
Który z tych testów jest najlepszy do tego ?

i jeszce jeden test, czy dobrze go napisałem?

  @Test
    void returnGoodValueOfUsers() throws Exception {
        MvcResult result = mockMvc.perform(post("/users/all")
                .header(AUTHORIZATION, BEARER + obtainAccessToken(USER_ADMIN, ADMIN_PASSWORD)))
                .andExpect(status().isOk())
                .andReturn();

        ObjectMapper mapper = new ObjectMapper();
        mapper.registerModule(new JavaTimeModule());
        List<User> userList = mapper.readValue(result.getResponse().getContentAsString(), new TypeReference<List<User>>() {});
        assertThat(userList.size()).isEqualTo(2);
    }
1

Ja preferuje strzelanie TestRestTemplatem w odpalony serwer, ale na MockMvc też może być chociaż ciężej nadać strukturę given/when/then.

Przeszkadza mi zaciemnienie testu ustawianiem nagłówków itp - ekstraktuj do metod pomocniczych co się da. Generalnie sporo tekstu, mało treści. Zwróć uwagę na nazwy testów „good values” nic nie mówi. Tworzenie ObjectMappera w teście jest dziwne, jeśli musisz to zrób go wcześniej poza testem. Url endpointu nie jest Restowy, masz tego świadomość?

Czyli popracuj nad nazewnictwem i czytelnością.

Zobacz jeszcze taki wynalazek: https://www.baeldung.com/spring-security-integration-tests

1 użytkowników online, w tym zalogowanych: 0, gości: 1