#132 Add constructor variants taking a cause to AuthException

[arjantijms]

Signed-off-by: arjantijms arjan.tijms@gmail.com

[darranl]

@arjantijms I am not sure if the form of this exception is just about it's age, security exceptions seem to be the one case where we do want to be cautious as to how much information is shared with the caller so keeping the AuthException separated from the underlying error can be beneficial.

[pzygielo]

I am not sure if the form of this exception is just about its age, security exceptions seem to be the one case where we do want to be cautious as to how much information is shared with the caller so keeping the AuthException separated from the underlying error can be beneficial.

Usage of Throwable.initCause is not prevented, like in GF/AuthContext.java.

With proposed changes, once propagated to cses.jauth.AuthException.java, this could be replaced with one line.

Old Ctors remain, so no one would be forced to use new ones.

[darranl]

Developers having to deliberately perform an additional step is probably a good thing if that step could leak sensitive information such as the underlying reason for the authentication failure.

[arjantijms]

The general security exception does have a cause:

public class GeneralSecurityException extends Exception {

 [...]

    /**
     * Creates a {@code GeneralSecurityException} with the specified
     * detail message and cause.
     *
     * @param message the detail message (which is saved for later retrieval
     *        by the {@link #getMessage()} method).
     * @param cause the cause (which is saved for later retrieval by the
     *        {@link #getCause()} method).  (A {@code null} value is permitted,
     *        and indicates that the cause is nonexistent or unknown.)
     * @since 1.5
     */
    public GeneralSecurityException(String message, Throwable cause) {
        super(message, cause);
    }

    /**
     * Creates a {@code GeneralSecurityException} with the specified cause
     * and a detail message of {@code (cause==null ? null : cause.toString())}
     * (which typically contains the class and detail message of
     * {@code cause}).
     *
     * @param cause the cause (which is saved for later retrieval by the
     *        {@link #getCause()} method).  (A {@code null} value is permitted,
     *        and indicates that the cause is nonexistent or unknown.)
     * @since 1.5
     */
    public GeneralSecurityException(Throwable cause) {
        super(cause);
    }

Specifically notice the "@since". As Jakarta Authentication was JDK 1.3/1.4, it stands to reason the age of the API really is the cause (no pun) of the missing cause, and not any explicit concern.

The similarly named exception in Spring Security has a cause as well:

https://docs.spring.io/spring-security/site/docs/current/api/org/springframework/security/core/AuthenticationException.html

And of course Jakarta Security has one as well:

public class AuthenticationException extends GeneralSecurityException {

    private static final long serialVersionUID = 1L;

    /**
     * Constructs a new <code>AuthenticationException</code> exception with <code>null</code> as its detail message.
     */
    public AuthenticationException() {
        super();
    }

    /**
     * Constructs a new <code>AuthenticationException</code> exception with the specified detail message.
     * 
     * @param message
     *            the detail message.
     */
    public AuthenticationException(String message) {
        super(message);
    }

    /**
     * Constructs a new <code>AuthenticationException</code> exception with the specified detail message and cause.
     * 
     * @param message
     *            the detail message.
     * @param cause
     *            the cause.
     */
    public AuthenticationException(String message, Throwable cause) {
        super(message, cause);
    }

[arjantijms]

btw, you could perhaps argue that not having the cause as an argument is more sensitive to security issues, as a developer would be more easily tempted to use "msg + e.toString()". Then the entire cause gets into the message, which might well be displayed to the end user.

(this is not just hypothetical, though anecdotal, I did refactor code a while ago that used AuthException this way into using initClause instead)

[arjantijms]

@darranl what do you think after seeing the other exceptions (Jakarta Security, Spring Security) taking a cause as parameter? Do you still feel strongly about this?

Do you feel strongly enough to block it (-1)?

I hope we can get to a resolution soon so I can then either close this without merging, or merge it in.

[darranl]

I don't think the other exceptions change my view on the risks of leaking information to the caller, but if we acknowledge that this exception is sensitive and shouldn't be shared with the caller I think it would make my objection redundant so no reason to -1.

[arjantijms]

@darranl okay, thanks. I'll add a notice in the javadoc to acknowledge that.

[arjantijms]

See updated javadoc, I added a notice there.