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 <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 | 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: arch...@mail-archive.com To unsubscribe, change settings or access archives, see http://www.ja-sig.org/wiki/display/JSG/cas-dev
