let’s keep the momentous ball going. Well, let's :-)
> 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 > > 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> 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 > 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 <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. > > Does it meet your expectation? > > Thanks. > Best regards, > > Jérôme LELEU > Founder of CAS in the cloud: www.casinthecloud.com | Twitter: @leleuj > Chairman of CAS: www.jasig.org/cas | Creator of pac4j: www.pac4j.org > > > > 2014-12-03 13:10 GMT+01:00 Dmitriy Kopylenko <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 <lel...@gmail.com> wrote: > > Hi, > > > 2014-12-03 1:42 GMT+01:00 Misagh Moayyed <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 J 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 > > …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:lel...@gmail.com] > Sent: Tuesday, December 2, 2014 1:11 AM > To: 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) { > 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()) { > 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: www.casinthecloud.com | Twitter: @leleuj > Chairman of CAS: www.jasig.org/cas | Creator of pac4j: www.pac4j.org > > 2014-12-02 6:48 GMT+01:00 Misagh Moayyed <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 > > 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 > -- > You are currently subscribed to cas-dev@lists.jasig.org as: lel...@gmail.com > 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: > 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: lel...@gmail.com > 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: > dkopyle...@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: lel...@gmail.com > 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: > dkopyle...@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: > 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: > dkopyle...@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: > 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: > dkopyle...@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
