wpg app code review

[zigewb]

1) AccessTokenServlet: it would make sense to distinguish error conditions like invalid code and unauthenticated client and return appropriate error codes to the user. Right now all exceptions are handled by a generic catch

2) AuthorizationServlet: in case of an error it returns a JSON message. This is wrong: per the RFC it should return a 302 redirect with error parameters properly encoded in the URL

Recommendation: walk through the RFC and make sure that the code covers all possible error conditions listed there

3) AuthorizationServlet: should validate the client before issuing the authz code. E.g. the user must explicitly grant the consent to the client or the client must be a WPG client

4) AuthorizationServlet: the current code organization doesn't allow to distinguish different error conditions like: invalid client, invalid scope, etc

5) AuthenticationService.getAuthorizationCode: authorization code expiration should not exceed 10 minutes (access token expiration can be indefinite)

6) It makes sense to have a special function that will generate a suitable random token/code. Also right now a standard Java random UUID generator is used (UUID type 4). In case of cluster deployment it can result in collisions. I'd recommend to use UUID type 1 (https://mvnrepository.com/artifact/com.eaio.uuid/uuid/3.2). To ensure some additional cryptographic protection of the generated tokens I'd also recommend to add MD5 on top of that and use the same hexadecimal representation as you use now for UUID (it will also be a 16 byte sequence)

7) AuthenticationService.getAuthorizationCode: the operation consists of several DB queries. It might have sense to take advantage of DB transactions and execute all those operations in a transaction context. This will require refactoring part of that code and moving it into UserDao

8) AuthorizationServlet: a redirect URL is built by taking the template AUTHORIZE_PATH and substituting some entries with external parameters. To ensure code security I'd recommend to (a) validate incoming values (b) URL-encode these parameters before substitution

9) LoginServlet: highly insecure way of storing user password is used: (a) do not use MD5 as the crypto hash function, use SHA-1 or SHA-256 instead (b) use random and (more or less) unique salt for every user

10) LoginServlet: session attribute "authenticated" is written as a boolean value, but in AuthorizationServlet it is read as a string value

11) RegisterServlet: should be removed

12) ThirdLoginServlet, WPGServlet: should be moved to src/test/java

13) SendEmailUtil: it reads configuration file from the working directory. There will be no custom files there. Consider reading files from something like "/cfg/var/runtime/wpg". The distributed configuration directory is mounted at "/cfg" inside the app framework container

14) AuthenticationResource: AnyAuthenticatedUser authorization scheme is used on all methods which is wrong. Please refer to https://github.com/wpgsh/cloud4water/wiki/Authentication-Application for the suggested authorization schemes

15) AuthenticationResource.updateUserProfile: I'd recommend to pass the password in as a plain text and calculate hash and salt inside the method

16) AuthenticatoinResource: most POST and PUT methods take input as application/x-www-form-urlencoded (@FormParam). This is an inconsistent way to design REST API. My recommendation is to use JSON as input format

17) AuthorizationFilter (in *.rest bundle): the purpose of AuthorizationFilter is unclear. Typically authorization can be handled with the suggested TW extension (see net.wapwag.authn.rest.authz)

[asd1245dss]

I have fixed most of the issues and write unit tests as much as possible.I used the remote karaf container not the embedded jetty server for servlet test in order to saving some time.

[sdwbvios]

@alexlukichev question 14, I'v no idea how to achieve the code.E.g Method getPrivateUserProfile just need Authorization Only {userId} and method createNewUser need Authorization Part of sign up process.

[alexlukichev]

Well, it may have sense not to expose createNewUser as REST API.

[alexlukichev]

On the use of authorization utils check here https://github.com/wpgsh/cloud4water/wiki/TW-JAX-RS-Authorization-Utils and in the javadoc attached to the maven artifact com.thingswise.appframework:jaxrs-utils:1.1.rc-SNAPSHOT

[asd1245dss]

Authorization Only {userId} is the default operation for the rest request or we need to create other annotation like @AnyAuthenticatedUser

[alexlukichev]

There is no default authorization operation, so yes, you'll need to create another AuthorizationScheme.