Access principal in success view #674
Conversation
|
Changes Unknown when pulling 3d5912e on Unicon:principal-success-view-669 into * on Jasig:master*. |
| @@ -44,7 +44,7 @@ screen.confirmation.message=Click <a href="{0}">here</a> to go to the applicatio | |||
|
|
|||
| #Generic Success Screen Messages | |||
| screen.success.header=Log In Successful | |||
| screen.success.success=You have successfully logged into the Central Authentication Service. | |||
| screen.success.success=You, {0}, have successfully logged into the Central Authentication Service. | |||
There was a problem hiding this comment.
We will need to do that for other languages as well. Would you mind opening an issue for that?
There was a problem hiding this comment.
Sure, https://github.com/Jasig/cas/issues/678 is here.
|
+1 with a few comments |
Conflicts: cas-server-webapp/src/main/webapp/WEB-INF/webflow/login/login-webflow.xml
|
Changes Unknown when pulling 816b667 on Unicon:principal-success-view-669 into * on Jasig:master*. |
|
Is there a reason we're adding official support via this API (which made sense to do when it was an add-on) versus better built-in support on the CentralAuthenticationService interface itself? |
|
Scott, how do you envision CAS (CentralAuthenticationService) be changed to support this? |
|
Bump |
|
Sorry this got lost in my inbox :-(. I guess I'm really asking if we're building this into CAS proper why aren't we thinking about how we can augment the CentralAuthenticationService interface. What do we get from using something like AuthenticationSupport? I think AuthenticationSupport made sense when it was a Unicon add-on, you couldn't go around mucking with interfaces in core. But if we deem this functionality to be important, we would want to encapsulate it behind our main service interface, right? |
|
Sure, I would welcome augmentations to the CAS interface; I am just not sure or seeing how any changes to the interface body would allow access to principal in this particular view. I am just not seeing the link. The op is pretty view-specific and does not have much to do with CAS interface itself. Maybe an option would be to add a method to the interface, to return the principal from a TGT and have that method be invoked by the view. That's marginally relevant at best. The |
|
Conceptually speatking, I think both interfaces do not the share the same objectives: one is protocol-oriented while the other one is authentication-oriented. Might be worth keeping both. |
Conflicts: cas-server-webapp/src/main/webapp/WEB-INF/spring-configuration/applicationContext.xml
|
Bump. @battags have you had a chance to review this? Do the comments make sense? |
|
The reason I'm against the AuthenticationSupport interface is that basically its intended so we can say we don't access the ticket registry directly but really it just accesses the ticket registry directly with no intention of any business logic. The CAS API is intended to be the business layer for ensuring our business logic gets executed when interacting with tickets. |
|
I hear you. At the same time, the business logic really "is" to get into the registry and grab the authentication/principal from the TGT. Looking over this once more, we might be able to do away with this interface. One possible option might be to allow How does that sound? |
|
But there's more business logic that should be executed. We shouldn't return a principal to you if the ticket is expired, etc. Is there a reason not to have a getTicketGrantingTIcket (or getSession if we want to generalize the words to not be CAS specific) on the CentralAuthenticationService interface? Because really AuthenticationViaForm should not be going into the registry directly either :-) |
|
That would be fine too; I just had in mind similar to @leleuj that we'd want to keep that interface to be solely about protocol-specific functions and no other auxiliary cases. Putting those additional checks and etc in the new method would be reassuring as double safety, because I think the generic-success view is only presented if the TGT is valid all around, right? We have got other actions that execute prior to rendering that view that check for the ticket's validity. P.S: getTicketGrantingTicket makes better sense to me, as the rest of the codebase is geared towards that kind of terminology for now. Introducing a session concept, at this point, would be a bit confusing in view of everything else that deals with a TGT vocabulary. |
|
Thinking about this a bit more: I am trying to envision what the getTicketGrantingTIcket() on the CAS interface would be like; it would have to be invoked as an on-entry action when the view renders right? and it would also make the TGT available to the view, from which authentication and then principal would be gleaned? Granted, the flow itself perhaps can be configured to set flowscope attributes for principal, but if getTicketGrantingTIcket() returns null for some reason, wouldn't the flow crash with an NPE when trying to set principal on the flowscope? It seems like we'd have to execute a bunch of code to get the principal across to the view and that would also require a proxy abstraction like the AuthenticationSupport? or am I off here? |
|
You'd possibly require some view level equivalent of a controller (or most likely you could also handle that purely via webflow logic) -- if you want to call that controller/view equivalent AuthenticationSupport I've got no problem with that other than that I think that's a poor name for something encapsulating view level logic :-). But that's completely different than ensuring that business logic is actually executed which is not the responsibility of any layer other than the CentralAuthenticationService. You can't get to a TGT unless its valid and the only thing that checks validity is a service layer. The registry is just for storage/retrieval. |
|
Sounds good, thanks for the suggestion. I like the view level equivalent approach, and will try to get something implemented to that effect. |
|
So, I have included a new method on the CAS interface to return the ticket granting ticket, and a flow action to pass the principal from that TGT onto the flow, plus some tests cases to prove this all works! |
| @@ -202,22 +202,17 @@ public CentralAuthenticationServiceImpl(final TicketRegistry ticketRegistry, | |||
| @Transactional(readOnly = false) | |||
| @Override | |||
| public List<LogoutRequest> destroyTicketGrantingTicket(final String ticketGrantingTicketId) { | |||
| Assert.notNull(ticketGrantingTicketId); | |||
There was a problem hiding this comment.
what do these changes have to do with the issue? Is it because you created that getTicketGrantingTicket method?
There was a problem hiding this comment.
Yes; there were a bunch of calls in CASImpl to get the TGT from the registry; all pretty much doing the same thing a getTicketGrantingTicket. So I made sure all of that is routed to getTicketGrantingTicket now.
There was a problem hiding this comment.
The only thing I wanted to note/call out here (can't remember if I did) was that unless Spring changed their proxying, if getTicketGrantingTicket() had an @transactional(readonly=true) on it and we called it from within another method the internal call wouldn't be through the proxy.
|
+1 just a few minor comments/questions |
|
Any other comments/suggestions? this looks good to everyone? |
|
P.S: Oh, forgot to mention that I did account for Scott's comments. |
|
No other suggestions just called out one thing with respect to transactions that may or may not be a concern. :-) |
|
Good call. I'll account for the change before merging in. |
Access principal in success view
This fixes #669