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 <[email protected]> wrote: > > Hi, > > > 2014-12-03 1:42 GMT+01:00 Misagh Moayyed <[email protected]>: >> > 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:[email protected]] >> Sent: Tuesday, December 2, 2014 1:11 AM >> To: [email protected] >> 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 <[email protected]>: >> >> 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 [email protected] as: [email protected] >> To unsubscribe, change settings or access archives, see >> http://www.ja-sig.org/wiki/display/JSG/cas-dev >> >> >> >> -- >> You are currently subscribed to [email protected] as: >> [email protected] >> To unsubscribe, change settings or access archives, see >> http://www.ja-sig.org/wiki/display/JSG/cas-dev >> -- >> You are currently subscribed to [email protected] as: [email protected] >> To unsubscribe, change settings or access archives, see >> http://www.ja-sig.org/wiki/display/JSG/cas-dev > > -- > You are currently subscribed to [email protected] as: > [email protected] > To unsubscribe, change settings or access archives, see > http://www.ja-sig.org/wiki/display/JSG/cas-dev -- You are currently subscribed to [email protected] as: [email protected] To unsubscribe, change settings or access archives, see http://www.ja-sig.org/wiki/display/JSG/cas-dev
