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