Refactor TokenResult

[DominikPinsel]

Hello everyone,

currently the TokenResult looks like this

public class TokenResult {

    /**
     * Returns true if the flow was successful.
     */   
    boolean success() {
        ...
    }


    /**
     * Returns the bearer token if the flow was successful; otherwise null.
     */
    public String getToken() {
        ...
    }

    /**
     * Returns the token expiration.
     */
    public long expiresIn() {
        ...
    }


    /**
     * Returns the error details if the flow was not successful; otherwise null.
     */
    public String error() {
        ...
    }

}

I'd suggest we make the following two changes.

1. The TokenReuslt should not have an success() and error() method. I'd like it more if the functions, that create the result, would throw an exception in case an error occurs.
2. The expiresIn is difficult to use, as it is now, because the code that want to check the expiration needs to know then the TokenResult was created. An internal expirationDate with a getter and an isExpired() method would work better.

_{Dominik Pinsel [email protected], Daimler TSS GmbH, legal info/Impressum}

[jimmarino]

I don't think we should use exceptions for flow control. If an unexpected exception occurs, it should throw an unchecked one, extending from EdcException if there is not an appropriate exception in the JDK.

Error is for things like invalid tokens or other "problems".

This is a general pattern we use throughout the codebase.

We need to be careful with expiresId being a fixed date. How will time zones be handled?

[DominikPinsel]

I think if a library throws an exception, it should only be catched to be wrapped in another exception or to be handled. Catching an exception and putting it in a message and Boolean variable looks for me like an anti-pattern (e.g. hiding stack trace, unspecific error).

Exceptions provide the means to separate the details of what to do when something out of the ordinary happens from the main logic of a program. For example exceptions that allow a retry mechanism (NetworkPartitioning) vs. exception that don't (InvalidRequest).

We need to be careful with expiresId being a fixed date. How will time zones be handled?

I don't understand the question. Is there a scenario where the time zone makes a difference for the implementation? Why does it work using the current 'expiresIn' property?

_{Dominik Pinsel [email protected], Daimler TSS GmbH, legal info/Impressum}

[jimmarino]

Sorry, to clarify, I'm not talking about ignoring unexpected exceptions and returning booleans. The TokenResponse error is when there is something about the request that is in "error" (hence the term "error" instead of exception).

If there is an unexpected exception, it could be thrown (as an unchecked exception, which the codebase standardizes on). However, this must be done with care to avoid exceptions leaking security-related information. If something needs to be logged/handled in this sensitive part of the code that may potentially contain sensitive information, it should be done at that point and not leave it to code higher up in the stack. As background, we spent a lot of time on this when we did a security audit of the codebase in this area.

Since this is all abstract, let me ask a specific question: what problem are you attempting to address in the current codebase?

On the timezone question, I'm not against changing "expiresIn", but if you specify a fixed time, you need to have a strategy for handing timezones.

[DominikPinsel]

Ah ok, thanks for clarification 😄

My code, using TokenResult, currently looks like this

@Override
    public TokenResult obtainClientCredentials(final String scope) {
        try {
            final DynamicAttributeToken dynamicAttributeToken = dynamicAttributeTokenProvider.getDynamicAttributeToken();
            final Duration expiresIn = Duration.between(dynamicAttributeToken.getExpiration(), Instant.now());
            return TokenResult.Builder.newInstance()
                    .token(dynamicAttributeToken.getToken())
                    .expiresIn(expiresIn.getSeconds()).build();

        } catch (DynamicAttributeTokenException e) {
            monitor.severe("cannot obtain client credentials", e); // last chance for the stacktrace to survive
            // TODO exception should not be stored in TokenResult; https://github.com/eclipse-dataspaceconnector/DataSpaceConnector/issues/140
            return TokenResult.Builder.newInstance().error(e.getMessage()).build();
        }
    }

But looking at your explanation I think I understood the intent of the error field wrong. I assume in this case throwing the exception is fine? Please excuse the name of the exception. It's all still in draft state.

_{Dominik Pinsel [email protected], Daimler TSS GmbH, legal info/Impressum}

[jimmarino]

OK, thanks for the example.

I think we need to unpack the issues here. My preliminary questions are:

1. What is DynamicAttributeTokenException intending to signify?
2. Related, what does "can't obtain client credentials" mean?

Based on that, it depends.

One other thing issue to note is : Duration.between(dynamicAttributeToken.getExpiration(), Instant.now()); The timzone is UTC. What timezone is the token set for (is it specified)? That combination can cause subtle bugs.

Since there are a lot of PRs out, my suggestion would be to first focus on the AssetIndex- and IDS- related ones and then we can come back to these other PRs. Should we take care of those first?

[jimmarino]

Closing as I believe these questions have been addressed.