[sakai2-tcc] ROLL CALL VOTE: override objections to retaining Hybrid patches in Login and Providers

[Speelmon, Lance]

JFL, Please see comments inline.  Thanks, L

Lance Speelmon
Scholarly Technologist

On Oct 12, 2010, at 5:36 AM, Jean-Francois Leveque wrote:

> Dear L,
> 
> 
> login
> 
> I've just had a quick look at http://jira.sakaiproject.org/secure/attachment/21877/SAK-17223+2.8.patch
> 
> It seems details are missing in your comments.
> 
> org.sakaiproject.login.filter.NakamuraAuthenticationFilter.principal
> org.sakaiproject.login.filter.NakamuraAuthenticationFilter.hostname
> org.sakaiproject.login.filter.NakamuraAuthenticationFilter.autoProvisionUser
> 
> are not mentioned in your comments.
> Where is the documentation for those properties?

This has been resolved. They are all documented now in javadoc.

> 
> I think such as the following should be fixed before trying to build:
> +	private static final Log LOG = LogFactory
> +			.getLog(NakamuraAuthenticationFilter.class);

Not sure what the issue is you are describing with this Log.  Can you clarify?

> 
> I'm not familiar with login but I think getSecret and getPrincipalLoggedIntoK2 should handle a null secret more carefully.
> 
> Looks like a null principal is dealt with in doFilter.
> 
> I can't find where the jsonObject added to the List returned by getPrincipalLoggedIntoK2 is used.

These three concerns have been addressed. Both the filter and the UDP now share the same logic. See:
http://jira.sakaiproject.org/browse/HYB-1

> 
> I'm afraid I cannot answer
> // what about top.login = false ?
> but I think this has to be answered before your changes get in.

I now test the value and log a warning if it is not enabled. I do think it should be disabled in the hybrid configuration.

> 
> Looks like there are still dependencies on org.sakaiproject.nakamura.utils. Thought they were gone.

That is no longer the case.  There are no dependencies on nakamura artifacts.

> 
> Why is there a jdk15 classifier in the json-lib dependency?

The way their releases are packaged, you must specify the classifier that is compatible with jdk15+.  

> 
> 
> providers
> 
> Now, I'm having a look at http://jira.sakaiproject.org/secure/attachment/21876/SAK-17222+2.8.patch.
> 
> Why are you using a different CONFIG_PREFIX instead of using hybrid shared configuration properties.
> 
> AFAICT, you're duplicating .principal, .hostname and .validateUrl.
> 

1) People may use the UDP and not the login filter (or vice versa).
2) I hate making up arbitrary sakai.properties keys. To avoid this, all my settings begin with the class name as a prefix and then are well documented in the javadoc; i.e. trying to make the settings clean, maintainable, and understandable.  :)

> You're also duplicating:
> private static final String COOKIE_NAME = "SAKAI-TRACKING";
> private static final String ANONYMOUS = "anonymous";
> 
> Why aren't those strings from configuration?

I have made those settings configurable via sakai.properties as well (just in case). See: 
http://jira.sakaiproject.org/browse/HYB-12

> 
> In authenticateUser, I think you should at least issue a WARNING when requested to authenticate with a null eid.

I am reluctant to use a warn.  Does debug work ok with you?

> 
> I think you should also issue a WARNING when asked to findUserByEmail with a null email or to getUser when edit is null.

As above, I have added debug statements.

> 
> I'm not knowledgeable enough about providers, but I think
> +		// What is the best default?
> in authenticateWithProviderFirst should be answered before your changes get in.

Yes I would like to hear from someone that knows more about UDP work flows.

> 
> I wonder how unsupported getUsers should be handled.

I will investigate where this is called.  Maybe it is not an issue, but I will find out. See:
http://jira.sakaiproject.org/browse/HYB-14

> 
> I think my comment about handling a null secret in login also applies to provider.

Should also be handled by http://jira.sakaiproject.org/browse/HYB-1 now.

> 
> Looks like there are still dependencies on org.sakaiproject.nakamura.utils. Thought they were gone.

They are gone. :) Check out mvn dependency:tree now.

> 
> 
> Hybrid
> 
> As for the main hybrid, I don't know for sure if you've answered AZ's questions already but my small knowledge makes me think you didn't.

I think we brought that discussion to conclusion.  

> 
> 
> Thanks, J-F


Lance Speelmon
Scholarly Technologist

On Nov 10, 2010, at 7:40 AM, Aaron Zeckoski wrote:

> Jean-Francois,
> I believe your comments have not been addressed. This is why I voted no.
> 
> Since the code does not appear to adhere to the best practices the MT
> has spent the last year trying to bring Sakai up to, it seems to me
> that it is a step backwards to put this code into the core as is. Just
> my opinion though as my vote is purely based on the technical aspects
> and related risks.
> 
> -AZ
> 
> 
> On Wed, Nov 10, 2010 at 3:43 AM, Jean-Francois Leveque
> <jean-francois.leveque at upmc.fr> wrote:
>> I'm not voting because I didn't have time to review the code again
>> before or during the vote.
>> 
>> Can one who has voted YES confirm it has improved on the points I've
>> highlighted before?
>> 
>> J-F
>> 
>> Anthony Whyte a écrit :
>>> Two TCC members have objected to retaining in 2.8.x two hybrid-related
>>> patches to login and providers that, in my opinion, are low risk
>>> additions that help simplify adding the hybrid module to a 2.8 build.
>>> 
>>> As I noted in my previous email I believe it important that we avoid
>>> erecting barriers to those who wish to pursue the evolving hybrid
>>> approach between the CLE and OLE--removing the patches will add
>>> additional steps to those who wish to deploy the hybrid 1.0 module, a
>>> scenario that benefits no one.
>>> 
>>> 1. Neither patch is active by default
>>> 2. No Nakamura dependencies are added to 2.8 (Lance has refactored
>>> Hybrid removing Nakamura dependencies)
>>> 3. No precedent set here (mirrors what we did for GB2 in 2.7)
>>> 4. The patches help simplify adding the hybrid module to a 2.8 installation
>>> 5. Hybrid itself is packaged as an indie and the project is
>>> well-maintained.  Any additional documentation required will be
>>> cheerfully generated by Lance.
>>> 6. Where we can assist our OAE colleagues we should; this is a modest
>>> step towards greater collaboration.
>>> 
>>> 
>>> MOTION
>>> 
>>> This is a public, on-list vote on the following question:
>>> 
>>> "Shall the TCC override objections to retaining the inclusion of hybrid
>>> patches to login (SAK-17223) and providers (SAK-17222) that were merged
>>> to 2.8.x for inclusion in the upcoming sakai-2.8.0 release."
>>> 
>>> 
>>> VOTING
>>> 
>>> Active TCC members will vote "YES" or "NO" where "YES" is a vote
>>> to override the -1 blocker vote and approve the Hybrid patches proposal
>>> for inclusion in 2.8.
>>> 
>>> Voting is OPEN now until 20:00 UTC, WEDNESDAY, 10 NOV 2010.
>>> 
>>> 
>>> CODE (PLEASE REVIEW IT)
>>> 
>>> Providers:
>>> https://source.sakaiproject.org/svn/providers/branches/sakai-2.8.x/hybrid/
>>> Login: https://source.sakaiproject.org/svn/login/branches/sakai-2.8.x/login-tool/tool/src/java/org/sakaiproject/login/filter/NakamuraAuthenticationFilter.java
>>> 
>>> 
>>> PATCH JIRAS
>>> 
>>> Hybrid UserDirectoryProvider http://jira.sakaiproject.org/browse/SAK-17222
>>> Hybrid container AuthN filter http://jira.sakaiproject.org/browse/SAK-17223
>>> 
>>> 
>>> HYBRID PROJECT
>>> 
>>> http://jira.sakaiproject.org/browse/HYB
>>> 
>>> Open/resolved HYB jiras:
>>> http://jira.sakaiproject.org/secure/IssueNavigator.jspa?reset=true&jqlQuery=project+%3D+HYB
>>> <http://jira.sakaiproject.org/secure/IssueNavigator.jspa?reset=true&jqlQuery=project+%3D+HYB>
>>> 
>>> 
>>> EARLIER DISCUSSIONS
>>> 
>>> Lance's answers to questions raised about Hybrid prior to the TCC vote
>>> to exclude Hybrid from 2.8.0
>>> 
>>> http://confluence.sakaiproject.org/display/TCC/2.8.0+Inclusion+Voting+Tally+and+Comments
>>> http://confluence.sakaiproject.org/display/TCC/List+of+Planned+2.8.0+Changes
>>> 
>>> 
>>> RECENT CONCERNS:
>>> 
>>> http://collab.sakaiproject.org/pipermail/sakai2-tcc/2010-October/000524.html
>>> 
>>> 
>>> 
>>> Cheers,
>>> 
>>> Anth
>>> 
>>> 
>>> Begin forwarded message:
>>> 
>>>> *From: *Anthony Whyte <arwhyte at umich.edu <mailto:arwhyte at umich.edu>>
>>>> *Date: *October 15, 2010 11:36:04 AM EDT
>>>> *To: *"sakai2-tcc at collab.sakaiproject.org
>>>> <mailto:sakai2-tcc at collab.sakaiproject.org> Committee"
>>>> <sakai2-tcc at collab.sakaiproject.org
>>>> <mailto:sakai2-tcc at collab.sakaiproject.org>>
>>>> *Subject: **[sakai2-tcc] SAK-19326: hybrid removal / patch retention*
>>>> 
>>>> I've removed the hybrid module assembly dependency from the 2.8.x
>>>> core-deploy pom as well as the version <property> in the master pom.
>>>>  I have not reverted the patches to login and providers (SAK-17222,
>>>> SAK-17223) as myself and others believe retaining the patches will
>>>> simplify the installation and deployment process for deployers who
>>>> wish to add hybrid to their 2.8 installations.  There is certainly
>>>> precedent for this approach; most recently we provided "hooks" for
>>>> gradebook2 in sakai-2.7.
>>>> 
>>>> I suggest we leave them in place for inclusion in the upcoming
>>>> sakai-2.8.0-a02 QA tag.  I believe it important that we avoid erecting
>>>> barriers to those who wish to pursue the evolving hybrid approach
>>>> between 2x and the Sakai OAE--removing the patches will add additional
>>>> steps to those who wish to deploy the hybrid 1.0 module, a scenario
>>>> that benefits no one.
>>>> 
>>>> Cheers,
>>>> 
>>>> Anthony
>> _______________________________________________
>> sakai2-tcc mailing list
>> sakai2-tcc at collab.sakaiproject.org
>> http://collab.sakaiproject.org/mailman/listinfo/sakai2-tcc
>> 
> 
> 
> 
> -- 
> Aaron Zeckoski - Software Engineer - http://tinyurl.com/azprofile