> 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. 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? 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. 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 <http://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 <http://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 <http://www.casinthecloud.com> | Twitter: @leleuj Chairman of CAS: www.jasig.org/cas <http://www.jasig.org/cas> | Creator of pac4j: www.pac4j.org <http://www.pac4j.org> 2014-12-02 6:48 GMT+01:00 Misagh Moayyed <mmoay...@unicon.net <mailto: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 <mailto:cas-dev@lists.jasig.org> as: lel...@gmail.com <mailto: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 <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
