rombert commented on issue #14: First commit for oidc handler. URL: https://github.com/apache/sling-whiteboard/pull/14#issuecomment-414334579 Thanks for the updates @hasinidilanka . I'll re-post my feedback here into a single message, to make it easier to review. I'll separate it into the two parts: 1. What is needed to merge the initial submission 1. What is needed to make this a 'proper' Sling module and move it to its own repository ## Initial merge 1. You have a reactor pom at `/oidc-hander/pom.xml` and a module pom at `/oidc-handler/core/pom.xml`. We only need one, so please move everything under `/oidc-hander`. This will make it easier to see the history of further changes. 1. The `errorEndpoint` and `loginEndpoint` class names must start with an uppercase letter 1. All occurences of `e.printStackTrace` must be replaced with either a log call or with rethrowing the exception ## Feature completion There are three directions of code review here: - Making code better suited to working in Sling - no more static methods/fields, use `@Component` and `@Reference` - various naming/design issues - complete the implementation of the `OIDCAuthenticationHandler` Take the time and read through them, make sure you understand them and ask for clarification when needed and feel free to approach them in any order and submit PRs whenever you have something fixed. 1. You are using the `OIDCConfigServlet` only to store configuration, so it's not really a servlet :-) My suggestion would be to remove this class and expose the configuration to the `OIDCAuthenticationHandler`. Then, if you need to pass it to other classes, you can do so from the there. Note that you can also reference the component, e.g. ```java @Reference private OIDCAuthenticationHandler authHandler. ``` 2. In the `OIDCConfiguration` I suggest you remove the 'boolean'/'string'/'password' suffixes. They're not really needed. 3. The `AuthenticationError` class is not an error, just has utility method to redirect to an error page. Maybe rename or move it somewhere else? 4. The `Handler` class could be named better. What exactly does it do? 5. The `Handler` class has some hardcoded fields - `stateValue` and `nonceValue`. What do those do? And should they not be secret or configurable? 6. The `IdTokenHandler` class only has a `verify` method, maybe find a more precise name than that? 7. There are unused imports in `IdTokenHandler`. 8. The `OIDCAuthenticationHandler` class is incomplete. Credential extraction is done in the `extractCredentials`, and that is good. But we're also missing a proper implementation of `requestCredentials`, which - if I got this correctly - should replace whatever the `loginEndpoint` is doing. You should also implement `dropCredentials` 9. The `UserManagerImpl` class should not use `FrameworkUtil` and the `BundleContext` to get a `SlingRepository`. It should instead be an OSGi component and obtain the repository using annotations, e.g. ```java @Component public class UserManagerImpl { @Reference private ResourceResolverFactory factory; public boolean createUser() { ResourceResolver resolver = factory.loginAdministrative(null); Session session = resolver.adaptTo(Session.class); } } ``` 10. In the `UserManagerImpl`, don't cache sessions and make sure to close them after you're done with them. 11. The field values from `OPConstant` are only used in the `OIDCConfiguration` class. Any reason not to move them there? You can always use something like ```java tring oidc_callback_url_string() default "http://localhost:8080/";; ```
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
