Quantcast

A ponion about org.apache.shiro.authc.AbstractAuthenticator.authenticate(AuthenticationToken token)

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

A ponion about org.apache.shiro.authc.AbstractAuthenticator.authenticate(AuthenticationToken token)

喂喂喂
Dear Shiro Team:
    In my opnion, org.apache.shiro.authc.AbstractAuthenticator.authenticate(AuthenticationToken token)  handles exception inappropriately.Below is the code indicates the problem  with my note in red:

// org.apache.shiro.authc.AbstractAuthenticator.authenticate(AuthenticationToken token)
public final AuthenticationInfo authenticate(AuthenticationToken token) throws AuthenticationException {

        if (token == null) {
            throw new IllegalArgumentException("Method argumet (authentication token) cannot be null.");
        }

        log.trace("Authentication attempt received for token [{}]", token);

        AuthenticationInfo info;
        try {
            info = doAuthenticate(token);
            if (info == null) {
                String msg = "No account information found for authentication token [" + token + "] by this " +
                        "Authenticator instance.  Please check that it is configured correctly.";
                throw new AuthenticationException(msg);
            }
        } catch (Throwable t) {
            AuthenticationException ae = null;
            if (t instanceof AuthenticationException) {
                ae = (AuthenticationException) t;
            }
            // Indeed, the Throwable t which is not caused by auhtentication problem but other such as a database problem is wrapped to be  a AuthenticationException instance, but details including exception message and stacktrace are missing and the follow-up handling makes the raw //Throwable t ignored.
            if (ae == null) {
                //Exception thrown was not an expected AuthenticationException.  Therefore it is probably a little more
                //severe or unexpected.  So, wrap in an AuthenticationException, log to warn, and propagate:
                String msg = "Authentication failed for token submission [" + token + "].  Possible unexpected " +
                        "error? (Typical or expected login exceptions should extend from AuthenticationException).";
                ae = new AuthenticationException(msg, t);
            }
            try {
                notifyFailure(token, ae);
            } catch (Throwable t2) {
                if (log.isWarnEnabled()) {
                    String msg = "Unable to send notification for failed authentication attempt - listener error?.  " +
                            "Please check your AuthenticationListener implementation(s).  Logging sending exception " +
                            "and propagating original AuthenticationException instead...";
                    log.warn(msg, t2);
                }
            }


            throw ae;
        }

        log.debug("Authentication successful for token [{}].  Returned account [{}]", token, info);

        notifySuccess(token, info);

        return info;
    }

    And then, the Authencication instance that wraps the orignal Thowable instance is thrown all the way, and get dumped below:
   
    // org.apache.shiro.web.filter.authc.AuthenticatingFilter
    protected boolean executeLogin(ServletRequest request, ServletResponse response) throws Exception {
        AuthenticationToken token = createToken(request, response);
        if (token == null) {
            String msg = "createToken method implementation returned null. A valid non-null AuthenticationToken " +
                    "must be created in order to execute a login attempt.";
            throw new IllegalStateException(msg);
        }
        try {
            Subject subject = getSubject(request, response);
            subject.login(token);
            return onLoginSuccess(token, subject, request, response);
        } catch (AuthenticationException e) {
            // ignore the AuthenticationException instance, only return a boolean, make the original Throwable ignored and it is hard to find out the mistake in the authentication progress.
            return onLoginFailure(token, e, request, response);
        }
    }

    Above shows the problem, that makes the origianl Throwable instance disappeared. In my situation, a Hibernate Exception was thrown, but no message showed, which made me take a long time to find out the mistake .
    English is not my native laguage. I know I express not well in English, hope it does not influence you to understand my opnion.

Yours respectfully,
Liang Weiwei

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: A ponion about org.apache.shiro.authc.AbstractAuthenticator.authenticate(AuthenticationToken token)

Brian Demers
There have been a couple issues on either side of this.

These exceptions should be logged, as 'debug' (if i remember correctly). Increasing the default logging can cause log spam in the cases where multiple realms are enabled, and one is misbehaving (db is down, or some network issue).

The common (and valid complain) is similar to your as this does not provide a good first experience, when setting up Shiro for the first time.

Does anyone have any ideas on ways to both eliminate the log spam and provide make it easier to troubleshoot for new installs?


On Thu, Dec 15, 2016 at 9:17 AM, 喂喂喂 <[hidden email]> wrote:
Dear Shiro Team:
    In my opnion, org.apache.shiro.authc.AbstractAuthenticator.authenticate(AuthenticationToken token)  handles exception inappropriately.Below is the code indicates the problem  with my note in red:

// org.apache.shiro.authc.AbstractAuthenticator.authenticate(AuthenticationToken token)
public final AuthenticationInfo authenticate(AuthenticationToken token) throws AuthenticationException {

        if (token == null) {
            throw new IllegalArgumentException("Method argumet (authentication token) cannot be null.");
        }

        log.trace("Authentication attempt received for token [{}]", token);

        AuthenticationInfo info;
        try {
            info = doAuthenticate(token);
            if (info == null) {
                String msg = "No account information found for authentication token [" + token + "] by this " +
                        "Authenticator instance.  Please check that it is configured correctly.";
                throw new AuthenticationException(msg);
            }
        } catch (Throwable t) {
            AuthenticationException ae = null;
            if (t instanceof AuthenticationException) {
                ae = (AuthenticationException) t;
            }
            // Indeed, the Throwable t which is not caused by auhtentication problem but other such as a database problem is wrapped to be  a AuthenticationException instance, but details including exception message and stacktrace are missing and the follow-up handling makes the raw //Throwable t ignored.
            if (ae == null) {
                //Exception thrown was not an expected AuthenticationException.  Therefore it is probably a little more
                //severe or unexpected.  So, wrap in an AuthenticationException, log to warn, and propagate:
                String msg = "Authentication failed for token submission [" + token + "].  Possible unexpected " +
                        "error? (Typical or expected login exceptions should extend from AuthenticationException).";
                ae = new AuthenticationException(msg, t);
            }
            try {
                notifyFailure(token, ae);
            } catch (Throwable t2) {
                if (log.isWarnEnabled()) {
                    String msg = "Unable to send notification for failed authentication attempt - listener error?.  " +
                            "Please check your AuthenticationListener implementation(s).  Logging sending exception " +
                            "and propagating original AuthenticationException instead...";
                    log.warn(msg, t2);
                }
            }


            throw ae;
        }

        log.debug("Authentication successful for token [{}].  Returned account [{}]", token, info);

        notifySuccess(token, info);

        return info;
    }

    And then, the Authencication instance that wraps the orignal Thowable instance is thrown all the way, and get dumped below:
   
    // org.apache.shiro.web.filter.authc.AuthenticatingFilter
    protected boolean executeLogin(ServletRequest request, ServletResponse response) throws Exception {
        AuthenticationToken token = createToken(request, response);
        if (token == null) {
            String msg = "createToken method implementation returned null. A valid non-null AuthenticationToken " +
                    "must be created in order to execute a login attempt.";
            throw new IllegalStateException(msg);
        }
        try {
            Subject subject = getSubject(request, response);
            subject.login(token);
            return onLoginSuccess(token, subject, request, response);
        } catch (AuthenticationException e) {
            // ignore the AuthenticationException instance, only return a boolean, make the original Throwable ignored and it is hard to find out the mistake in the authentication progress.
            return onLoginFailure(token, e, request, response);
        }
    }

    Above shows the problem, that makes the origianl Throwable instance disappeared. In my situation, a Hibernate Exception was thrown, but no message showed, which made me take a long time to find out the mistake .
    English is not my native laguage. I know I express not well in English, hope it does not influence you to understand my opnion.

Yours respectfully,
Liang Weiwei


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: A ponion about org.apache.shiro.authc.AbstractAuthenticator.authenticate(AuthenticationToken token)

Shawn McKinney

> On Dec 15, 2016, at 11:21 AM, Brian Demers <[hidden email]> wrote:
>
> There have been a couple issues on either side of this.
>
> These exceptions should be logged, as 'debug' (if i remember correctly). Increasing the default logging can cause log spam in the cases where multiple realms are enabled, and one is misbehaving (db is down, or some network issue).
>
> The common (and valid complain) is similar to your as this does not provide a good first experience, when setting up Shiro for the first time.
>
> Does anyone have any ideas on ways to both eliminate the log spam and provide make it easier to troubleshoot for new installs?
>

Hello,

It has been my experience that a security handler should either log or forward a downstream exception, but not both, to prevent the kind of log spam you are referring to.  Furthermore exceptions from downstream resources, i.e. database / ldap servers, should probably be converted into a common exception format, i.e. mysecurityhandlerexception, but the original exception should always be included as a member of the new exception class.

That way information isn’t duplicated (in the logs) or lost, across the entire lifecycle of the exception.

HTH,
Shawn
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: A ponion about org.apache.shiro.authc.AbstractAuthenticator.authenticate(AuthenticationToken token)

Brian Demers
After looking at this, and the original comment, I _think_ I see what you are getting at.

We should defiantly NOT be dropping/ignoring any exceptions. After my first read through, I was thinking this was a logging level issue.

Liang, can you open a JIRA issue, and include I high level overview of how we could reproduce this in a test? Of course providing a test would be ideal!





On Thu, Dec 15, 2016 at 1:55 PM, Shawn McKinney <[hidden email]> wrote:

> On Dec 15, 2016, at 11:21 AM, Brian Demers <[hidden email]> wrote:
>
> There have been a couple issues on either side of this.
>
> These exceptions should be logged, as 'debug' (if i remember correctly). Increasing the default logging can cause log spam in the cases where multiple realms are enabled, and one is misbehaving (db is down, or some network issue).
>
> The common (and valid complain) is similar to your as this does not provide a good first experience, when setting up Shiro for the first time.
>
> Does anyone have any ideas on ways to both eliminate the log spam and provide make it easier to troubleshoot for new installs?
>

Hello,

It has been my experience that a security handler should either log or forward a downstream exception, but not both, to prevent the kind of log spam you are referring to.  Furthermore exceptions from downstream resources, i.e. database / ldap servers, should probably be converted into a common exception format, i.e. mysecurityhandlerexception, but the original exception should always be included as a member of the new exception class.

That way information isn’t duplicated (in the logs) or lost, across the entire lifecycle of the exception.

HTH,
Shawn

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

回复: A ponion about org.apache.shiro.authc.AbstractAuthenticator.authenticate(AuthenticationTokentoken)

喂喂喂
I am new to Shiro and have not opened a issue before, but I am willing to do it.


------------------ 原始邮件 ------------------
发件人: "Brian Demers";<[hidden email]>;
发送时间: 2016年12月16日(星期五) 凌晨5:49
主题: Re: A ponion about org.apache.shiro.authc.AbstractAuthenticator.authenticate(AuthenticationTokentoken)

After looking at this, and the original comment, I _think_ I see what you are getting at.

We should defiantly NOT be dropping/ignoring any exceptions. After my first read through, I was thinking this was a logging level issue.

Liang, can you open a JIRA issue, and include I high level overview of how we could reproduce this in a test? Of course providing a test would be ideal!





On Thu, Dec 15, 2016 at 1:55 PM, Shawn McKinney <[hidden email]> wrote:

> On Dec 15, 2016, at 11:21 AM, Brian Demers <[hidden email]> wrote:
>
> There have been a couple issues on either side of this.
>
> These exceptions should be logged, as 'debug' (if i remember correctly). Increasing the default logging can cause log spam in the cases where multiple realms are enabled, and one is misbehaving (db is down, or some network issue).
>
> The common (and valid complain) is similar to your as this does not provide a good first experience, when setting up Shiro for the first time.
>
> Does anyone have any ideas on ways to both eliminate the log spam and provide make it easier to troubleshoot for new installs?
>

Hello,

It has been my experience that a security handler should either log or forward a downstream exception, but not both, to prevent the kind of log spam you are referring to.  Furthermore exceptions from downstream resources, i.e. database / ldap servers, should probably be converted into a common exception format, i.e. mysecurityhandlerexception, but the original exception should always be included as a member of the new exception class.

That way information isn’t duplicated (in the logs) or lost, across the entire lifecycle of the exception.

HTH,
Shawn

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: A ponion about org.apache.shiro.authc.AbstractAuthenticator.authenticate(AuthenticationToken token)

scSynergy
In reply to this post by Brian Demers
I believe the only way to achieve the desired behavior is to make 'log spam' the default behavior and allow us to override it via an option in Shiro's configuration, instead of hard coding log levels.

Brian Demers wrote
There have been a couple issues on either side of this.

These exceptions should be logged, as 'debug' (if i remember correctly).
Increasing the default logging can cause log spam in the cases where
multiple realms are enabled, and one is misbehaving (db is down, or some
network issue).

The common (and valid complain) is similar to your as this does not provide
a good first experience, when setting up Shiro for the first time.

Does anyone have any ideas on ways to both eliminate the log spam and
provide make it easier to troubleshoot for new installs?
Loading...