Hi Mark, My suspicion is that there's no "great reason" anymore. I don't know historically why the code was written this way. My speculation would be that the idea might have been to have a single login form which attempted authentication via several services. But, that is not really a plausible "real life" scenario, as modern authentication services often send you off to another location anyhow (e.g. Shibboleth).
That section of the AuthenticationManager, according to git "blame", may date back to 2005 when the idea of "stackable" auth was first introduced into DSpace: https://github.com/DSpace/DSpace/blame/master/dspace-api/src/main/java/org/dspace/authenticate/AuthenticationManager.java#L63 So, rather than dwell too much on the reason why it was built this way, it seems like we should investigate options to modernize our Authentication system in general. Personally, I feel this is an area where we really should just be using a third-party Authentication plugin (as noted in DS-1566) rather than continually maintaining a 10 year old custom Authentication class. https://jira.duraspace.org/browse/DS-1566 Just my two cents here though! :) - Tim On 5/4/2015 2:23 PM, Mark H. Wood wrote: > Followup to IRC conversation with hpottinger. > > Please remind me why we do this. If there are two stacked > AuthenticationMethods which happen to use the same identifiers, we > could ignore the user's choice and always authenticate using the first > one. At most one method is allowed to succeed before > AuthenticationManager.authenticate() returns, so the reason can't be > to let every method get a look at the login request. > > Should we not rather have an AuthenticationMethod.authorize() in > addition to .authenticate()? A UI would tell AuthenticationManager > which method to use for authentication, and then AuthenticationManager > would call authorize() on every method. authenticate would *only* > verify credentials; authorize() would be for whatever a method would > like to do, such as decorating the session with additional > information, updating the EPerson or other records, etc. It might be > sensible to have authorize() take up the function of adding special > groups to the session. > > > > ------------------------------------------------------------------------------ > One dashboard for servers and applications across Physical-Virtual-Cloud > Widest out-of-the-box monitoring support with 50+ applications > Performance metrics, stats and reports that give you Actionable Insights > Deep dive visibility with transaction tracing using APM Insight. > http://ad.doubleclick.net/ddm/clk/290420510;117567292;y > > > > _______________________________________________ > Dspace-devel mailing list > Dspace-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/dspace-devel > ------------------------------------------------------------------------------ One dashboard for servers and applications across Physical-Virtual-Cloud Widest out-of-the-box monitoring support with 50+ applications Performance metrics, stats and reports that give you Actionable Insights Deep dive visibility with transaction tracing using APM Insight. http://ad.doubleclick.net/ddm/clk/290420510;117567292;y _______________________________________________ Dspace-devel mailing list Dspace-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dspace-devel
