APP-2887 - Authentication API #173
Conversation
thibauult
requested review from
symphony-hong,
symphonydarlys and
symphony-elias
symphony-bdk-examples/simple-hello-world/src/main/resources/config.yaml
Outdated
Show resolved
Hide resolved
...-examples/simple-hello-world/src/main/java/com/symphony/bdk/examples/AuthenticationMain.java
Outdated
Show resolved
Hide resolved
symphony-bdk-core/src/main/java/com/symphony/bdk/core/config/BdkConfig.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * | ||
| */ | ||
| void refresh() throws AuthenticationException; |
There was a problem hiding this comment.
Is it up to the user of the SDK to refresh the authentication, ideally the SDK should handle that himself and refresh auth when needed, what do you think?
There was a problem hiding this comment.
That's my goal indeed (cf. V4MessageService#L42). However, I thought it was interesting to give the ability to the developer to refresh if he wants to.
What could be the right strategy if we want to make this method used internally only? Through an AbstractAuthSession implements AuthSession?
There was a problem hiding this comment.
Do we need to expose the authsession at all in this case?
symphony-bdk-core/src/main/java/com/symphony/bdk/core/auth/OboAuthenticator.java
Outdated
Show resolved
Hide resolved
symphony-bdk-core/src/main/java/com/symphony/bdk/core/auth/AuthenticatorFactory.java
Outdated
Show resolved
Hide resolved
symphony-bdk-core/src/main/java/com/symphony/bdk/core/auth/AuthSession.java
Outdated
Show resolved
Hide resolved
…lementations using ServiceLoader
…-bootstrap # Conflicts: # symphony-bdk-core/pom.xml
…-bootstrap # Conflicts: # symphony-bdk-core/src/main/java/com/symphony/bdk/core/config/BdkConfigParser.java
symphony-bdk-core/src/main/java/com/symphony/bdk/core/auth/impl/OboAuthenticatorRSAImpl.java
Outdated
Show resolved
Hide resolved
symphony-bdk-core/src/main/java/com/symphony/bdk/core/auth/impl/OboAuthenticatorRSAImpl.java
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,3 @@ | |||
| # Authentication | |||
|
|
|||
| To be done... | |||
There was a problem hiding this comment.
create a new story to create the documentation!!! please mention the JIRA story here and put in the ticket as acceptance criteria : "Documentation must contain explanation about Client Provider and what happens if 2 different providers are configured"
| * @return true if response status is 401, false otherwise | ||
| */ | ||
| public boolean isUnauthorized() { | ||
| return this.code == 401; |
There was a problem hiding this comment.
We should use Enum HttpStatus instead of
| * | ||
| * @return a new {@link ApiClient} instance. | ||
| */ | ||
| ApiClient newInstance(); |
There was a problem hiding this comment.
really good approach, we can use different libraries to generate clients!!!
symphony-bdk-core/pom.xml
Outdated
| <dependency> | ||
| <groupId>com.fasterxml.jackson.dataformat</groupId> | ||
| <artifactId>jackson-dataformat-yaml</artifactId> | ||
| <version>2.11.0</version> |
There was a problem hiding this comment.
management of version in properties sections is better!!!
symphony-bdk-core/pom.xml
Outdated
| <dependency> | ||
| <groupId>com.symphony.platformsolutions</groupId> | ||
| <artifactId>symphony-bdk-core-invoker-jersey2</artifactId> | ||
| <version>1.2.0-SNAPSHOT</version> |
| @API(status = API.Status.EXPERIMENTAL) | ||
| public class SymphonyBdk { | ||
|
|
||
| private final BdkConfig config; |
There was a problem hiding this comment.
some class attributes can be instanced in the methods, except if you think these class will be inherits by other classes which can use them
| private final ApiClient loginApiClient; | ||
| private final ApiClient relayApiClient; | ||
|
|
||
| private JwtHelper jwtHelper = new JwtHelper(); |
| */ | ||
| @Slf4j | ||
| @API(status = API.Status.INTERNAL) | ||
| public class BotAuthenticatorRSAImpl implements BotAuthenticator { |
There was a problem hiding this comment.
RSAImpl -> RsaImpl?
New BDK Authentication API. At the moment, only the RSA authentication has been implemented for regular and ob-behalf-of (OBO) authentication.
TODO
List sorted by order of priority...