{"action":"created","issue":{"url":"https://api.github.com/repos/pac4j/vertx-pac4j/issues/20","labels_url":"https://api.github.com/repos/pac4j/vertx-pac4j/issues/20/labels{/name}","comments_url":"https://api.github.com/repos/pac4j/vertx-pac4j/issues/20/comments","events_url":"https://api.github.com/repos/pac4j/vertx-pac4j/issues/20/events","html_url":"https://github.com/pac4j/vertx-pac4j/issues/20","id":104745325,"number":20,"title":"Investigate possible use of ProfileManager in Pac4j ","user":{"login":"millross","id":1318862,"avatar_url":"https://avatars.githubusercontent.com/u/1318862?v=3","gravatar_id":"","url":"https://api.github.com/users/millross","html_url":"https://github.com/millross","followers_url":"https://api.github.com/users/millross/followers","following_url":"https://api.github.com/users/millross/following{/other_user}","gists_url":"https://api.github.com/users/millross/gists{/gist_id}","starred_url":"https://api.github.com/users/millross/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/millross/subscriptions","organizations_url":"https://api.github.com/users/millross/orgs","repos_url":"https://api.github.com/users/millross/repos","events_url":"https://api.github.com/users/millross/events{/privacy}","received_events_url":"https://api.github.com/users/millross/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":9,"created_at":"2015-09-03T17:31:13Z","updated_at":"2015-09-10T11:11:35Z","closed_at":null,"body":"At present the vert.x auth providers must be separate because they encapsulate dealing with the web session.\r\n\r\nInvestigate using the ProfileManager for this, and if possible, switch to its use. This might mean that we can have a single Pac4jAuthProvider for vert.x which simply rejects so that the underlying vertx-pac4j subsystem can take care of authentication fully. Pac4j offers the abstractions we want to use, so it makes \r\nsense to make use of those."},"comment":{"url":"https://api.github.com/repos/pac4j/vertx-pac4j/issues/comments/139205723","html_url":"https://github.com/pac4j/vertx-pac4j/issues/20#issuecomment-139205723","issue_url":"https://api.github.com/repos/pac4j/vertx-pac4j/issues/20","id":139205723,"user":{"login":"millross","id":1318862,"avatar_url":"https://avatars.githubusercontent.com/u/1318862?v=3","gravatar_id":"","url":"https://api.github.com/users/millross","html_url":"https://github.com/millross","followers_url":"https://api.github.com/users/millross/followers","following_url":"https://api.github.com/users/millross/following{/other_user}","gists_url":"https://api.github.com/users/millross/gists{/gist_id}","starred_url":"https://api.github.com/users/millross/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/millross/subscriptions","organizations_url":"https://api.github.com/users/millross/orgs","repos_url":"https://api.github.com/users/millross/repos","events_url":"https://api.github.com/users/millross/events{/privacy}","received_events_url":"https://api.github.com/users/millross/received_events","type":"User","site_admin":false},"created_at":"2015-09-10T11:11:35Z","updated_at":"2015-09-10T11:11:35Z","body":"I actually find code easier to read and understand where the Direct vs Indirect behaviour is captured in two different handler subclasses rather than shared in one, but maybe I’m in the minority. \n\nHowever, if it would ever make sense to use a direct client with a stateful behaviour or an indirect client with a stateless behaviour then if clauses based on type in a common handler class are required anyway (though that code could be put into some kind of plugin determined based on the client type when we set up the handler). \n\nIf, however, there is no situation where you would use a direct client in a redirect-based authentication flow, or an indirect one without a redirect-based authentication flow then to me the codebase is cleaner and more legible by having a different handler subclass (from a parent RequiresAuthenticationHandler) for each of those, with those pieces of code where we currently use a client type check isolated down in those subclasses. When I read a conditional based on type, I find it harder, rather than easier, to read the code, especially if they appear in more than one place. \n\nHow about for now, while I’m making the changes to pac4j-vertx, I do it with separate subclasses and then you can review the PR and determine whether you still think it’s harder to read. I’ll do it in such a way that I can easily turn things back into conditionals based on type if we decide that’s preferable :)\n\nCheers,\n\nJez\n\n> On 10 Sep 2015, at 09:56, LELEU Jérôme <notifications@github.com> wrote:\n> \n> You raise a good point. I'm not sure it would make things easier to read and understand, which is my main objective. I have the feeling that the behaviour implemented in the RequiresAuthenticationFilter exists outside of the client itself, but I'm open to any suggestion.\n> Give it a try on the j2e-pac4j library with a \"fake\" PR if you want...\n> \n> —\n> Reply to this email directly or view it on GitHub <https://github.com/pac4j/vertx-pac4j/issues/20#issuecomment-139174251>.\n> \n\n"}}