Issue 116

[witjem]

Додана JWT (JSON Web Token) авторизація

Security пакет перенесений в service шар JWT криптується за допомогою AES, тому потрібно вказати 2 ключа aes.init.vector.hex та aes.key.hex Трохи перероблений swagger під JWT авторизацію connected to #116

[vaa25]

1. Почистити імпорти,
2. почистити невикористовувані дані в скопіпастених класах
3. дописати тест в Jwtokenservice
4. Виправити SecurityTests

[vaa25]

1. Дописать тест на AESCryptTokenService

[vaa25]

валится org.itevents.service.security.AESCryptTokenServiceTest#shouldEncryptDecrypt http://pastebin.com/7aauPF2e

ключи в lp(local.properties) указаны, в tlp нет, так как их нет в tlp.TEMPLATE

[witjem]

В мене тест проходить, треба розбиратись

[IgorMaksymov]

Тест должен раскодировать закодированный токен или что? из названия не понятно

[witjem]

Він повинен закодувати і розкодувати токен.

[vaa25]

@roma-ilnitsky вообще-то тогда это уже не юнит тест

[vaa25]

А зачем objectMapper вынес в контекст? Но благодаря этому стало очевидно почему у меня валился тест. Теперь у меня валится весь контекст. No qualifying bean of type [org.codehaus.jackson.map.ObjectMapper] found for dependency: expected at least 1 bean which qualifies as autowire candidate for this dependency.

[witjem]

objectMapper щоб в тестах можна було його замокати та контролювати. Напевно в градл забув додати jackson хоча дивно що його немає, оскільки спрінг якось в json серіалізує об'єкти

[vaa25]

Так ты в тестах и без контекста его замокал. В градле он есть...

[vaa25]

Почему используешь не SpringJUnit4ClassRunner ?

[witjem]

Бо в ньому немає потреби

[vaa25]

shouldCreateToken(). Создаешь юзера, но при этом используешь левые логин и пароль

Почему нет потребности? Не пойму ход мысли. С одной стороны кидаешь объект в контекст чтобы использовать в тестах его моки, с другой стороны исключаешь спринг из тестов.

[witjem]

А навіщо, я використовую мокіто для тесту, мені не потрібен спрінг для даного тесту

[vaa25]

Есть такой метод assertNull называется...

[vaa25]

В AESCryptTokenServiceTest весь спринг заменил константами и рефлексией, а потом говоришь что спринг тебе не нужен

[vaa25]

NOT PASSED

[witjem]

За assertNull дякую, буду знати ) З приводу AESCryptTokenServiceTest не згоден, для тестування мені повністю вистачає Mockito, якщо можна ще якось реалізувати тест, здорово, але моя реалізація така, чим вона погана не зрозуміло. Наведи якісь аргументи.

[vaa25]

Если постараться, то и ни Mokito ни Junit могут быть не нужны. Все можно сделать на чистой джаве. Однако эти фреймворки, как и Spring, придумали для сокращения кода и облегчения жизни программисту. В данном случае Spring может сократить код наполовину, а также избавить от нарушения принципа DRY, так как некоторые константы лежат в пропертях. К тому же тесты из этого класса все равно падают. http://pastebin.com/nvGvnhf4

[vaa25]

strToken -> jsonToken str -> jsonToken

[vaa25]

проблему падения теста решил путем обрезания aes.key.hex до 128 бит

[witjem]

@IgorMaksymov це інтиграційний тест, він зразу перевіряє на правельність закодування і розкодування.

@vaa25

зачем появился spring-config-mocks.xml? треба перейменувати в spring-security-mocks.xml

Він виник із за того що в secyrity залежить від userService been а в тестах потрібно мокати деякі біни щоб не піднімати усі біни проекту для тестування однієї фічі

[vaa25]

це інтиграційний тест

интеграционные у нас в отдельной папке лежат, хотя я сомневаюсь что это он.

Він виник із за того що в secyrity залежить від userService been а в тестах потрібно мокати деякі біни щоб не піднімати усі біни проекту для тестування однієї фічі

Ну и в каком месте этот мок используется?

[witjem]

@vaa25

1. У нас половину тестів інтеграційних, справжній юніт тест, це коли ти тестиш один клас, причому не піднімаєш 100500 бінів для його тестування.
2. Коли я піднімаю біни для security, виникає потреба userService біна в JwtTokenService

[vaa25]

@roma-ilnitsky

1. Если у нас по-началу что-то было неправильно, то это не значит что к неправильному нужно стремиться.
2. Я имел в виду конкретное место в твоем пр.

[witjem]

@vaa25 використовується в CryptTokenServiceTest

[vaa25]

Не вижу замоканого UserService. А если его нет, то это мертвый код

[witjem]

@vaa25 Як ти сказав, краще використовувати SpringJUnit4ClassRunner.class в CryptTokenServiceTest щоб зменшити код. Якщо я використовую SpringJUnit4ClassRunner.class в тесті, мені потрібно вказати ContextConfiguration, тому я вказав spring-security.xml, але в мене один бін із spring-security не піднімається із за залежності до userService, таким чином я створив spring-security-mocks.xml в якому обявив userService бін.

Звісно можна обійтись без SpringJUnit4ClassRunner.class але тоді прийдеться гратися із рефлексіїю, тоді коду буде трохи більше.

[IgorMaksymov]

@roma-ilnitsky созданием spring-security-mocks.xml ты нарушил DRY. Что тебе мешает испортировать нужную зависимость ?

[witjem]

@IgorMaksymov Я не можу підняти spring-security.xml біни без userServise біна

[IgorMaksymov]

@roma-ilnitsky @ContextConfiguration({ "classpath:applicationContextTestAddon.xml", "classpath*:spring-security.xml" })

поскольку я не могу сбилдить проект из-за отсутвия проперти в темплейте, кидаю ответ как есть.

[vaa25]

@IgorMaksymov

поскольку я не могу сбилдить проект из-за отсутвия проперти в темплейте

Какой именно проперти отсутствует?

[IgorMaksymov]

что-то про AES Encrypt

[vaa25]

В темплейте они есть

[IgorMaksymov]

@vaa25 действительно, не в тот темплейт смотрел. Это уже не в первый раз, когда я так путаюсь, думаю, надо их как-то переименовать или выделить слово TEST

[IgorMaksymov]

@roma-ilnitsky "classpath:spring-security.xml", "classpath:applicationContext.xml"

[vaa25]

@IgorMaksymov "classpath:applicationContext.xml" подымает весь контекст "classpath:spring-security-mocks.xml" мокает только UserService Разница есть? @roma-ilnitsky strToken говорит о том, что это токен в виде строки созданной методом toString(), который тоже кстати нужно исправить, если не удалить str говорит о том, что это произвольная строка Рекомендую эти переменные порефакторить, например так:

strToken -> jsonToken str -> jsonToken

[IgorMaksymov]

@vaa25 есть, там мы нарушаем DRY, а в другом способе не нарушаем. но поднимаем лишнее..

[vaa25]

@IgorMaksymov lets listen teamlead about it

[witjem]

Need merge

[vaa25]

• PASSED

[alex-anakin]

to @roma-ilnitsky

1. Try make lines of code no longer than 120 symbols. Sometimes it's allowed use some extra symbols, for example, if method signature is about 125-130 symbols.
2. Interface CryptTokenService is placed in package security, but interface TokenService is placed at up level. Why?
3. We have our own service exceptions and parent for it. But AESCryptTokenException created with another principle. Why?
4. I advise you to read article. You have some problems in your solution described in the 2nd and 3d section of the article.
5. Don't use xml declaring for AESCryptTokenService. It's different from the our general approach. Use annotation @Service for class and @Value for fields.
6. You should place classes in package security more structured. I think you should add some internal packages.
7. JwtAuthenticationProvider.additionalAuthenticationChecks method is not implemented. May be you need some explanation in comment?
8. Class JwtTokenAuthenticationFilter:
   • doFilterInternal method signature has weird formatting.
   • Code inside try-block in doFilterInternal method need refactoring - it is difficult to read.
   • You don't handle exception inside catch-block in doFilterInternal method, so you should or handle it, or make as described in the article section number 10.
   • Method setAuthenticationDetailsSource is not used
   • You don't need neither default constructor nor the constructor with arguments in the class
9. Class AESCryptTokenService:
   • Move public methods upper than private.
   • Why do you need to inject ObjectMapper? It's a simple class with default constructor. If you create it by new, you could test AESCryptTokenService without mocks.
10. How you get value of field String TOKEN in test AESCryptTokenServiceTest? It seems like fraud :)
11. JwtTokenServiceTest. Using encrypt(any()) you allow errors appear inside method logic without control. You should create new Token using input params of method and encrypting it.

[alex-anakin]

@roma-ilnitsky, I have the same opinion as @romach about his comments (magic numbers, etc.)

[witjem]

@alex-anakin

9. If I dont inject ObjectMapper, I can not test this class.
10. Where can I get AES/CBC/PKCS5PADDING encrypt kay ?

[vaa25]

@roma-ilnitsky keys for test can be in travis.properties

[witjem]

@vaa25 they here

[witjem]

@AndriyBaibak 2:30

Refactor code

[alex-anakin]

@roma-ilnitsky

1. Two commits without issue number. Don't fix it, but keep this case in mind for future.
2. String BEARER = "Bearer " - variable name should explain it's purpose, don't duplicate it's value.
3. Field authenticationDetailsSource is never used.
4. You haven't any test for JwtTokenAuthenticationFilter. And don't forget about constructor because you have logic in it.
5. Try to avoid explicit bean name appointment if you use annotations. F.e. @Component("jwtAuthenticationFilter")
6. Use @Override annotation if you implement interface method
7. Place explaination of additionalAuthenticationChecks method unnessesarity implementation inside braces
8. You don't need to declare unchecked exceptions (RuntimeException and its subclasses) in the throws clause of a method. Declaration of unchecked exceptions are not required
9. Don't use /* */ -type of comments for one line of comment. It reduces code readability.
10. May be you mean generated in this statement String generateToken = "someGenerateToken"? Verb in past time?
11. In test shouldFailedDecryptToken() additionally check exception message text.
12. You have unused imports in some classes. Remove it.

[alex-anakin]

FAILED

@AndriyBaibak 1:00

[witjem]

@alex-anakin Refactore done @AndriyBaibak 3:00