would greatly facilitate/enable composing different authentication flows much more easily (without the need for much hacking around) out of those cohesive components.
I would like to see us take an approach that can be incorporated into 4.x and future small releases one step at a time. Ultimately, changing the main flow to call this and that would be great but for all the deployments from here to CAS5, but also we should devise a strategy where these components are gradually added without breaking backwards capability and come the right time, we’ll move the invocations around into the main flow. Otherwise, we’ll come up with 4 possible ideas on how X could work, then we’ll forget about it, then revisit the topic a year from now, resume conversations, come up with more ideas…:) let’s keep the momentous ball going. From: Dmitriy Kopylenko [mailto:dkopyle...@unicon.net] Sent: Wednesday, December 3, 2014 10:07 AM To: cas-dev@lists.jasig.org Subject: Re: [cas-dev] Reducing CASImpl's complexity: ArgExtractors and more +1. Now we are getting somewhere more concrete ;-) Again, this refactoring/decoupling would require work to refactor the main default UI flow, yes, but again, would greatly facilitate/enable composing different authentication flows much more easily (without the need for much hacking around) out of those cohesive components. That’s my main thing I am after. Now a little bit of buzzwordiness fun, if you will, to make it a bit clearer: microservices - a buzzwordy architectural style that is getting much attention lately for software designs. Another word to describe this architecture is “pipes-and-filters” - a small, cohesive set of services that do one thing well and pass the data around via “pipes” to other services, thus accomplishing the high degree of reuse and mostly composability. Is this idea new? Not really. It’s been around for ages. Classical Unix design is based on this e.g. cat apple.txt | wc | mail -s "The count" nob...@december.com <mailto:nob...@december.com> This is kind of a renaissance of this architectural style making its way into mainstream software engineering as modern software systems are getting more and more complex and this way it facilitates a great degree of easier construction of such systems as well as easier maintenance and reasoning about those systems, etc. </end-of-buzzword-rant> ;-) D. On Dec 3, 2014, at 11:48 AM, Misagh Moayyed <mmoay...@unicon.net <mailto:mmoay...@unicon.net> > wrote: Yes, this is closer to what I had in mind. If I were to summarize our objectives, it would be the following: - Separate out auxiliary functions that are now buried inside CASImpl which is going to help with … - Decouple authentications from CAS-protocol functions - Define injections/abstractions for CASImpl so it can be extended and overridden if need be. Readability would simply be a byproduct of us trying to accomplish the above goals. I had a chance to review the pull proposed. Here is what I have so far: The pull really addresses the first objective (and I understand there may be other “helper” type components later on) but I am not sure this going to help at all with separating authentication events and CAS functions. Even if we ended up with an AuthenticationHelper, it is still going to be involved directly in combination with creating and/or delegating TGTs. So that’s not good, because here we are just moving code around without actually reducing any complexity. Furthermore, while it should be helpful generally we have never come across a need to override a function in a “helper” component. Same goes with CAS protocol operations. In extreme rare cases would someone want to change the way tickets are retrieved from registry or change how tickets are created in the first place. It’s the stuff in between that matters and how these all connect together. Helpers in the form of util classes don’t allow us to extend and customize that behavior. What Dima is describing is attractive 1. Separate out all authentication-related functions into a higher abstraction, like AuthenticationService or AuthenticationHelper, etc and remove all traces from CASImpl 2. We’ll need to modify the flow, and all areas that touch CASImpl functions previously having assuming this behavior, to explicitly invoke the authentication sub system rather than relying on CASImpl to do so. 3. Mark classes and methods as non-final where appropriate and expose injection points. By doing something like this, we will have: 1. Reduced the size of CASImpl which helps with readability, if you’re concerned with that. 2. Decoupled major systems in the CAS, for authentication and ticket generation, etc. 3. Introduced injections points for each subsystem that would be independently invoked From: Dmitriy Kopylenko [mailto:dkopyle...@unicon.net] Sent: Wednesday, December 3, 2014 6:20 AM To: cas-dev@lists.jasig.org <mailto:cas-dev@lists.jasig.org> Subject: Re: [cas-dev] Reducing CASImpl's complexity: ArgExtractors and more My idea would be to have distinct API for authentication (higher than say AuthenticationManager) say AuthenticationService, etc. that deals exclusively with authentication concepts e.g. Credentials, Principals, producing valid Authentication objects, etc.(using our AuthenticationManager machinery underneath, etc.) And then there is a distinct let's say SsoService that deals with tickets. Those of course are high level APIs that the UI (SWF) layer could use at appropriate times to compose complex authentication transactions in a decoupled manner. I ain't saying that we must throw everything about current CAS away immediately, but it would help that everyone sees the benefit of such design and at least start going in that direction (for CAS5 perhaps) Does that make sense? D. Sent from my iPhone On Dec 3, 2014, at 07:49, Jérôme LELEU < <mailto:lel...@gmail.com> lel...@gmail.com> wrote: Hi, I welcome any opinion. Readability is a key factor as it's generally linked to a good design. You want to decouple authentication from SSO subsystems. I think we all agree. As a SSO subsystem, we have the tickets storage. So far, tickets registries are used straightfully in the CASImpl and by trying to improve readability, I propose to extract this 'low' logic into a 'higher' component: StorageHelper (a better name could certainly be found). Take a look at this draft: <https://github.com/Jasig/cas/pull/786/files> https://github.com/Jasig/cas/pull/786/files. Does it meet your expectation? Thanks. Best regards, Jérôme LELEU Founder of CAS in the cloud: <http://www.casinthecloud.com/> www.casinthecloud.com | Twitter: @leleuj Chairman of CAS: <http://www.jasig.org/cas> www.jasig.org/cas | Creator of pac4j: <http://www.pac4j.org/> www.pac4j.org 2014-12-03 13:10 GMT+01:00 Dmitriy Kopylenko < <mailto:dkopyle...@unicon.net> dkopyle...@unicon.net>: If I may add my 2c. It's not about the readability of the code (CAS code is quite readable and not "esoteric" as it is), but about architectural API design that gets in the way quickly for implementing complex multi-staged authentication flows. The fact remains that CASImpl is "complected". It tightly couples authentication with SSO (tickets) concepts - e.g. authentication act is indistinguishable from producing TGT tickets - which lives for very dirty hacks to facilitate complex flows. Again, it would benefit CAS (IMHO) to start the design in the direction of decoupling authentication and SSO subsystems by way of decoupled, distinguished APIs. D. Sent from my iPhone On Dec 3, 2014, at 04:06, Jérôme LELEU < <mailto:lel...@gmail.com> lel...@gmail.com> wrote: Hi, 2014-12-03 1:42 GMT+01:00 Misagh Moayyed < <mailto:mmoay...@unicon.net> mmoay...@unicon.net>: > with the appropriate methods. Maybe put in a ServiceRegistryHelper and in > a ServicesManagerHelper, which would be injected in CASImpl. That sounds fine. It’s a little bit less ambitious than I had hopes but I actually could see a path where we ultimately start to break CASImpl apart. It’s certainly not monstrous :) and does the job, but I can think of a few use cases that abstractions would tremendously help. You're right: it's not very ambitious, but it would help with readability. And it could offer higher abstractions. So helper methods that attempt to be doing duplicate calls are moved to some form of abstraction or utility class. I am in fact sort of reminded of this pull: <https://github.com/Jasig/cas/pull/362> https://github.com/Jasig/cas/pull/362 …which is similar to what you have in mind I suppose. The pull basically decides to move some of these common “operations” out of CASImpl so that the class is less cluttered and, there is also room of extensions. The latter point is very beneficial in scenarios where the authentication flow is broken apart and we need an authentication object without the following dangling TGT. So, we could either start fresh or review this PR and see if it makes sense to rework it back in. Is that any similar to what you were thinking? Not exactly, it's somehow another option to simplify code. There are easy ways to help reading the code: move properties into a parent class, split CASImpl into several parts: CASLoginImpl, CASLogoutImpl, but these are more tricks than real refactoring. I always prefer to provide higher abstractions: adding a StorageHelper in top of the ticket registry for example. At some point though, I’d still like to revisit the idea of decoupling ticket and ticketids. This feels like a step in the right direction. I will propose a pull request with what I have in mind so that you can see the target code and to get some feedback from others as well. Best regards, Jérôme From: Jérôme LELEU [mailto: <mailto:lel...@gmail.com> lel...@gmail.com] Sent: Tuesday, December 2, 2014 1:11 AM To: <mailto:cas-dev@lists.jasig.org> cas-dev@lists.jasig.org Subject: Re: [cas-dev] Reducing CASImpl's complexity: ArgExtractors and more Hi, Working on the CASImpl is a good idea. Though, I'd like to say that this class is not the "terrible nightmare" that some may have imagined. There is something like 400 lines of useful code which is fairly readable. I've seen a lot worse in my developer life. I see two ways of improvment: - indeed, a ticket id generator could hold more logic like the expiration policiy. I'm not sure it should be tied to a service as it has always been a general setting of the CAS server - I would work on code consistency and readabillity. Let's take an example. We have in the delegateTicketGrantingTicket method: final ServiceTicket serviceTicket = this.serviceTicketRegistry.getTicket(serviceTicketId, ServiceTicket.class); if (serviceTicket == null || serviceTicket.isExpired()) { logger.debug("ServiceTicket [{}] has expired or cannot be found in the ticket registry", serviceTicketId); throw new InvalidTicketException(serviceTicketId); } final RegisteredService registeredService = this.servicesManager.findServiceBy(serviceTicket.getService()); verifyRegisteredServiceProperties(registeredService, serviceTicket.getService()); if (!registeredService.getProxyPolicy().isAllowedToProxy()) { logger.warn("ServiceManagement: Service [{}] attempted to proxy, but is not allowed.", serviceTicket.getService().getId()); throw new UnauthorizedProxyingException(); } Would be turned into: final ServiceTicket serviceTicket = getValidServiceTicket(serviceTicketId); final RegisteredService registeredService = getValidService(serviceTicket.getService()); with the appropriate methods. Maybe put in a ServiceRegistryHelper and in a ServicesManagerHelper, which would be injected in CASImpl. In the validateServiceTicket method, we have (again!): final ServiceTicket serviceTicket = this.serviceTicketRegistry.getTicket(serviceTicketId, ServiceTicket.class); if (serviceTicket == null) { <http://logger.info/> logger.info("Service ticket [{}] does not exist.", serviceTicketId); throw new InvalidTicketException(serviceTicketId); } final RegisteredService registeredService = this.servicesManager.findServiceBy(service); verifyRegisteredServiceProperties(registeredService, serviceTicket.getService()); try { synchronized (serviceTicket) { if (serviceTicket.isExpired()) { <http://logger.info/> logger.info("ServiceTicket [{}] has expired.", serviceTicketId); throw new InvalidTicketException(serviceTicketId); } [...] } finally { if (serviceTicket.isExpired()) { this.serviceTicketRegistry.deleteTicket(serviceTicketId); } } This time we do the same checks but not exactly in the same order and we explicitely delete the ticket if it is expired: why not in the delegateTicketGrantingTicket method? If the ticket is expired, it must be evicted from the ticket registry. What do you think? Thanks. Best regards, Jérôme LELEU Founder of CAS in the cloud: <http://www.casinthecloud.com/> www.casinthecloud.com | Twitter: @leleuj Chairman of CAS: <http://www.jasig.org/cas> www.jasig.org/cas | Creator of pac4j: <http://www.pac4j.org/> www.pac4j.org 2014-12-02 6:48 GMT+01:00 Misagh Moayyed < <mailto:mmoay...@unicon.net> mmoay...@unicon.net>: Team, There has been much discussion around reducing the complexity that is now carried by CASImpl. I’d say that simply the ability to remove from CASImpl the mapping between services and ticketed generators would be great improvement [1]. Presently, custom service extensions are sort of forced to register their own ticket id generator, even if they don’t really care for one per se. Furthermore, I have been reminded that removing this configuration from CASImpl would allow our service layer to remain as pure as possible without having any knowledge of the protocol-specific functionality. So, I had in mind that instead of what exists today, every service created by argument extractors would carry/register a default ticket id generator, something like this: service.getTicketIdGenerator().generateTicket(…) …which would remove the need to register one explicitly, of course can be overridden if need be. Now since services are actually created by argument extractors, this would require that each argument extractor expose a parameter so that a ticketid generator be injected in. So the turn of events would be: 1. Argument Extractor (AE) is injected with ticketid generator X 2. AE attempts to extract the service, by calling SomeService.createService(X) 3. SomeService creates the service initialized with X I would like to eliminate step #2, and actually allow the argument extractor itself to do the thing it says it should, which is the extraction of the service. The flow would be: 1. Argument Extractor (AE) is injected with ticketid generator X 2. AE attempts extracts the service initialized with X Here is a pull request that attempts to do that: <https://github.com/Jasig/cas/pull/698> https://github.com/Jasig/cas/pull/698 We have been reviewing the exact meaning of the pull and its pros and cons and it has sort of gone stale. I would like us to come to a decision about the fate of this change, whether there is anything I can help with. I am not really sure where to go from here. So some guidance/clarification would be very helpful. [1] <https://github.com/Jasig/cas/blob/master/cas-server-core/src/main/java/org/jasig/cas/CentralAuthenticationServiceImpl.java#L130> https://github.com/Jasig/cas/blob/master/cas-server-core/src/main/java/org/jasig/cas/CentralAuthenticationServiceImpl.java#L130 -- You are currently subscribed to <mailto:cas-dev@lists.jasig.org> cas-dev@lists.jasig.org as: <mailto:lel...@gmail.com> lel...@gmail.com To unsubscribe, change settings or access archives, see <http://www.ja-sig.org/wiki/display/JSG/cas-dev> http://www.ja-sig.org/wiki/display/JSG/cas-dev -- You are currently subscribed to <mailto:cas-dev@lists.jasig.org> cas-dev@lists.jasig.org as: <mailto:mmoay...@unicon.net> mmoay...@unicon.net To unsubscribe, change settings or access archives, see <http://www.ja-sig.org/wiki/display/JSG/cas-dev> http://www.ja-sig.org/wiki/display/JSG/cas-dev -- You are currently subscribed to <mailto:cas-dev@lists.jasig.org> cas-dev@lists.jasig.org as: <mailto:lel...@gmail.com> lel...@gmail.com To unsubscribe, change settings or access archives, see <http://www.ja-sig.org/wiki/display/JSG/cas-dev> http://www.ja-sig.org/wiki/display/JSG/cas-dev -- You are currently subscribed to <mailto:cas-dev@lists.jasig.org> cas-dev@lists.jasig.org as: <mailto:dkopyle...@unicon.net> dkopyle...@unicon.net To unsubscribe, change settings or access archives, see <http://www.ja-sig.org/wiki/display/JSG/cas-dev> http://www.ja-sig.org/wiki/display/JSG/cas-dev -- You are currently subscribed to <mailto:cas-dev@lists.jasig.org> cas-dev@lists.jasig.org as: <mailto:lel...@gmail.com> lel...@gmail.com To unsubscribe, change settings or access archives, see <http://www.ja-sig.org/wiki/display/JSG/cas-dev> http://www.ja-sig.org/wiki/display/JSG/cas-dev -- You are currently subscribed to <mailto:cas-dev@lists.jasig.org> cas-dev@lists.jasig.org as: <mailto:dkopyle...@unicon.net> dkopyle...@unicon.net To unsubscribe, change settings or access archives, see <http://www.ja-sig.org/wiki/display/JSG/cas-dev> http://www.ja-sig.org/wiki/display/JSG/cas-dev -- You are currently subscribed to <mailto:cas-dev@lists.jasig.org> cas-dev@lists.jasig.org as: <mailto:mmoay...@unicon.net> mmoay...@unicon.net To unsubscribe, change settings or access archives, see <http://www.ja-sig.org/wiki/display/JSG/cas-dev> http://www.ja-sig.org/wiki/display/JSG/cas-dev -- You are currently subscribed to <mailto:cas-dev@lists.jasig.org> cas-dev@lists.jasig.org as: <mailto:dkopyle...@unicon.net> dkopyle...@unicon.net To unsubscribe, change settings or access archives, see <http://www.ja-sig.org/wiki/display/JSG/cas-dev> http://www.ja-sig.org/wiki/display/JSG/cas-dev -- You are currently subscribed to cas-dev@lists.jasig.org <mailto:cas-dev@lists.jasig.org> as: mmoay...@unicon.net <mailto:mmoay...@unicon.net> To unsubscribe, change settings or access archives, see http://www.ja-sig.org/wiki/display/JSG/cas-dev -- You are currently subscribed to cas-dev@lists.jasig.org as: arch...@mail-archive.com To unsubscribe, change settings or access archives, see http://www.ja-sig.org/wiki/display/JSG/cas-dev
