[jira] [Comment Edited] (OAK-2872) ExternalLoginModule should clear state when login was not successful

["angela (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/OAK-2872?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14552282#comment-14552282 ] 

angela edited comment on OAK-2872 at 5/20/15 1:04 PM:
------------------------------------------------------

IMHO the state must be cleared both in LoginModule#commit() and LoginModule#abort() in case of unsuccessful authentication. the abort method is covered by the base class, but i am not totally sure if the fix committed at http://svn.apache.org/r1679503 doesn't 100% correct as the state is not cleared in all cases where the commit method returns false:

{code}
public boolean commit() throws LoginException {
        if (externalUser == null) {
            // login attempt in this login module was not successful
            clearState();
            return false;
        }
        Set<? extends Principal> principals = getPrincipals(externalUser.getId());
        if (!principals.isEmpty()) {
            if (!subject.isReadOnly()) {
                subject.getPrincipals().addAll(principals);
                if (credentials != null) {
                    subject.getPublicCredentials().add(credentials);
                }
                setAuthInfo(createAuthInfo(externalUser.getId(), principals), subject);
            } else {
                log.debug("Could not add information to read only subject {}", subject);
            }
            return true;
        }
        return false; // NOTE: state not cleared in this case ... right?
    }
{code}


was (Author: anchela):
IMHO the state must be cleared both in {@code LoginModule#commit()} and {@code LoginModule#abort()} in case of unsuccessful authentication. the abort method is covered by the base class, but i am not totally sure if the fix committed at http://svn.apache.org/r1679503 doesn't 100% correct as the state is not cleared in all cases where the commit method returns false:

{code}
public boolean commit() throws LoginException {
        if (externalUser == null) {
            // login attempt in this login module was not successful
            clearState();
            return false;
        }
        Set<? extends Principal> principals = getPrincipals(externalUser.getId());
        if (!principals.isEmpty()) {
            if (!subject.isReadOnly()) {
                subject.getPrincipals().addAll(principals);
                if (credentials != null) {
                    subject.getPublicCredentials().add(credentials);
                }
                setAuthInfo(createAuthInfo(externalUser.getId(), principals), subject);
            } else {
                log.debug("Could not add information to read only subject {}", subject);
            }
            return true;
        }
        return false; // NOTE: state not cleared in this case ... right?
    }
{code}

> ExternalLoginModule should clear state when login was not successful
> --------------------------------------------------------------------
>
>                 Key: OAK-2872
>                 URL: https://issues.apache.org/jira/browse/OAK-2872
>             Project: Jackrabbit Oak
>          Issue Type: Bug
>          Components: auth-external
>            Reporter: Alex Parvulescu
>            Assignee: Alex Parvulescu
>             Fix For: 1.3.0
>
>
> As discussed in [1], it looks like the ExternalLoginModule ignores cleaning up its internal state when login was not successful.
> What I assume happens next is the old session (probably the initial one created on the very first login call) would be reused throughout the module's lifetime, which would in the end result in the SNFEs post compaction.
> [1] http://markmail.org/thread/pcmlz74ngxl7sqfy



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)