CAS-1283: principal attributes in the service validate success payload. #224

Merged
merged 3 commits into from Jun 11, 2013

6 participants

@dima767

WEB-INF/view/jsp/protocol/3.0/casServiceValidationSuccess.jsp and
WEB-INF/view/jsp/protocol/3.0/casServiceValidationFailure.jsp
are now the default service validate views.

@dima767 dima767 CAS-1283: principal attributes in the service validate success payload.
WEB-INF/view/jsp/protocol/3.0/casServiceValidationSuccess.jsp and
WEB-INF/view/jsp/protocol/3.0/casServiceValidationFailure.jsp
are now the default service validate views.
51c5f48
@leleuj leleuj commented on the diff Apr 9, 2013
...r-webapp/src/main/resources/protocol_views.properties
@@ -25,11 +25,18 @@ cas1ServiceSuccessView.(class)=org.jasig.cas.web.view.Cas10ResponseView
cas1ServiceSuccessView.successResponse=true
### CAS 2.0 Response Protocol Views
+cas2ServiceSuccessView.(class)=org.springframework.web.servlet.view.JstlView
+cas2ServiceSuccessView.url=/WEB-INF/view/jsp/protocol/2.0/casServiceValidationSuccess.jsp
+
+cas2ServiceFailureView.(class)=org.springframework.web.servlet.view.JstlView
+cas2ServiceFailureView.url=/WEB-INF/view/jsp/protocol/2.0/casServiceValidationFailure.jsp
@leleuj
leleuj added a line comment Apr 9, 2013

Why do we keep the old responses ? Are they used somewhere ?

@dima767
dima767 added a line comment Apr 9, 2013
@leleuj
leleuj added a line comment Apr 9, 2013

Actually, the version 1 will be still available through the /validate url, which is not the case of the 2.0, replaced by the 3.0 on /serviceValidate and /proxyValidate urls.
That's why I would prefer cleaning than keeping for backward compatibility.

Views defined in the protocol/ directory are also used when you enable the appropriate protocol : OpenID, OAuth, ClearPass, aren't they ?

@dima767
dima767 added a line comment Apr 9, 2013
@leleuj
leleuj added a line comment Apr 9, 2013

Right, I would just keep protocol/3.0 instead of protocol/2.0...

@battags
battags added a line comment Apr 9, 2013

I don't think its valid to remove 2.0. There is a 2.0 protocol, and those files support it explicitly.

@dima767
dima767 added a line comment Apr 9, 2013
@wgthom
Apereo Foundation member
wgthom added a line comment Apr 9, 2013

The 3.0 protocol is meant to supersede 2.0 in way that 2.0 didn't for 1.0. That is, 3.0 is meant to replace the current response at the current 2.0 endpoints (since we're just documenting common practice) rather than inventing a new validation response. This seems to argue in favor of removing the older 2.0 views.

That said, I see no harm in leaving them where they are, and they do provide potential benefit to deployers, as Dima suggests, of being able to fall back to 2.0 if that became necessary for some reason.

+1 for commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@leleuj leleuj commented on the diff Apr 9, 2013
...view/jsp/protocol/3.0/casServiceValidationFailure.jsp
+ except in compliance with the License. You may obtain a
+ copy of the License at the following location:
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+--%>
+<%@ page session="false" contentType="text/plain" %>
+<%@ taglib prefix="fn" uri="http://java.sun.com/jsp/jstl/functions" %>
+<cas:serviceResponse xmlns:cas='http://www.yale.edu/tp/cas'>
@leleuj
leleuj added a line comment Apr 9, 2013

I think we talked about adding version="3.0", don't we ?

@wgthom
Apereo Foundation member
wgthom added a line comment Apr 9, 2013

We did talk about. Any thoughts if this will cause problems with any of the clients?

@leleuj
leleuj added a line comment Apr 9, 2013

I tend to say no, but it's hard to tell without testing. For safety, we can post-pone this change for 4.0 revision...

@wgthom
Apereo Foundation member
wgthom added a line comment Apr 9, 2013

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@leleuj leleuj commented on the diff Apr 9, 2013
...view/jsp/protocol/3.0/casServiceValidationSuccess.jsp
+ copy of the License at the following location:
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+--%>
+<%@ page session="false" %>
+<%@ taglib prefix="c" uri="http://java.sun.com/jsp/jstl/core" %>
+<%@ taglib uri="http://java.sun.com/jsp/jstl/functions" prefix="fn" %>
+<cas:serviceResponse xmlns:cas='http://www.yale.edu/tp/cas'>
@leleuj
leleuj added a line comment Apr 9, 2013

Same comment about version="3.0"...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@serac

-1 on merge.

The uncertainty around impact to clients is a fairly big red alarm. We should proceed based on facts, not assumptions about expected impact. What integration testing with clients have you done around this change? Hopefully you have done testing and you can just cite the results, and my fears will prove unfounded. The "official" Jasig clients listed at https://wiki.jasig.org/display/CASC/Home would be a suitable minimal set of test cases.

@dima767

I have not done any clients integration testing. Is there a currently an automated suite of integration tests for all the clients that CAS server utilizes? If not, I think it would be a different issue of developing such a test suite.

Cheers,
D.

@wgthom
Apereo Foundation member
@serac

I think some testing prior to acceptance is warranted for a change that could break clients. That seems perfectly reasonable. If you want to vouch for the above clients without explicit testing, I would take you at your word. Wider audience here means the myriad "unofficial" clients, and I would agree the RC well suited to that case. I should note I would feel entirely different about this change if it were bound to a new endpoint such that the impact to existing clients would be optional and explicit. I will be sure to voice that recommendation in CAS protocol feedback thread.

@wgthom
Apereo Foundation member
@serac
@leleuj

Related to : apereo/java-cas-client#19, I don't see the authenticationDate and isFromNewLogin attributes : aren't they missing ?

@dima767

As far as I understand this pull should just bring the existing well-known customization to the protocol response. All the other new features of the CAS 3 protocol would come in separately.

@robertoschwald
@dima767

Understand. It is just that this particular pull request just focuses on principal attributes and nothing else. If it could be merged, then another commit could be made to add these additional pieces of info. I'd hesitate to modify this particular pull request.

@dima767

BTW, can someone point me to the latest spec doc with a full example of the expected new response?

@robertoschwald
@dima767
@mmoayyed

I went ahead and tested this pull locally. Everything is functional as expected and described by the spec. Per the last CAS dev conference call, this pull should be ready to go. Planning to merge by tomorrow afternoon, unless there are any concerns.

@leleuj

+1

@serac serac commented on the diff Jun 11, 2013
...view/jsp/protocol/3.0/casServiceValidationSuccess.jsp
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+--%>
+<%@ page session="false" %>
+<%@ taglib prefix="c" uri="http://java.sun.com/jsp/jstl/core" %>
+<%@ taglib uri="http://java.sun.com/jsp/jstl/functions" prefix="fn" %>
+<cas:serviceResponse xmlns:cas='http://www.yale.edu/tp/cas'>
+ <cas:authenticationSuccess>
+ <cas:user>${fn:escapeXml(assertion.chainedAuthentications[fn:length(assertion.chainedAuthentications)-1].principal.id)}</cas:user>
@serac
serac added a line comment Jun 11, 2013

The changes to Assertion in 31ec59d may afford some opportunity to clean up the JSTL here and in similar usage elsewhere in this file.

@mmoayyed
mmoayyed added a line comment Jun 11, 2013

Certainly, yes. Will keep that in mind after the merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@serac

+1
We will want to review after feature-new-authn-api merge with master.

@mmoayyed mmoayyed merged commit 1ccd262 into apereo:master Jun 11, 2013
@mmoayyed mmoayyed deleted the Unicon:CAS-1283 branch Jun 11, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment