Hi, To make things clearer of what I have in mind, I sketched out a pull request presenting what would be the storage helper: https://github.com/Jasig/cas/pull/786/files.
I think it goes in the right direction as the 'storage logic' is now gathered in the StorageHelper class. Just let me know what you think about it. 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 10:06 GMT+01:00 Jérôme LELEU <lel...@gmail.com>: > 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
