Repository navigation
Feat/add teams channel plugin - #96
AlirezaValikhani wants to merge 3 commits into
Conversation
arefbehboudi
left a comment
arefbehboudi
left a comment
There was a problem hiding this comment.
Thanks for this, the overall structure follows the other plugins nicely, and the JWT validation and tests are good to see. I ran :plugins:teams:test (all passing) and booted the app. Three things need to change before merge: the committed default config, the open-by-default user check, and reply routing to the wrong conversation. The rest are should-fix or nits. Also, most of the new files are missing a trailing newline.
|
|
||
| /** | ||
| * Optional: Azure AD object ID of the single Teams user the assistant should respond to. | ||
| * When blank, the first user who messages the bot is accepted (not recommended for |
There was a problem hiding this comment.
The Javadoc says “the first user who messages the bot is accepted,” but isAllowedUser accepts every user when this is blank. Please update the comment to match whatever behavior we settle on in the controller.
| channel.updateConversationReference(new TeamsConversationReference(activity.serviceUrl(), conversationId)); | ||
|
|
||
| channelRegistry.publishMessageReceivedEvent( | ||
| new TeamsChannelMessageReceivedEvent(channel.getName(), text, conversationId)); | ||
| executor.execute(() -> handleMessage(conversationId, text)); | ||
| return ResponseEntity.ok().build(); | ||
| } | ||
|
|
||
| private void handleMessage(String conversationId, String text) { | ||
| try { | ||
| String response = agent.respondTo(conversationId, text); | ||
| channel.sendMessage(response); |
There was a problem hiding this comment.
Replies can be sent to the wrong conversation. handleMessage calls channel.sendMessage(response), which uses the single global reference that is overwritten on every incoming message. With the queued single-thread executor, this sequence is possible: A messages, B messages, and A’s answer is posted into B’s chat. Please capture the reference per message:
TeamsConversationReference ref = new TeamsConversationReference(activity.serviceUrl(), conversationId);
channel.updateConversationReference(ref); // keep for proactive sends
...
executor.execute(() -> handleMessage(ref, text));
Then call a new channel.sendMessage(ref, response) overload, similar to Telegram’s sendMessage(chatId, threadId, message).
| } | ||
|
|
||
| @Override | ||
| public void sendMessage(String message) { |
There was a problem hiding this comment.
Related to the comment above: please add sendMessage(TeamsConversationReference ref, String message) and have sendMessage(String) delegate to it with the last known reference.
|
|
||
| /** @return true if {@code bearerToken} is a currently valid Bot Framework Connector token for this bot */ | ||
| boolean isValid(String bearerToken) { | ||
| try { |
There was a problem hiding this comment.
The Bot Framework spec also requires the token’s serviceUrl claim to match the activity’s serviceUrl. That check is important here, because we send the bot’s outbound access token to activity.serviceUrl(). Please pass the activity’s serviceUrl into isValid(...) and compare it to the claim.
Separately, the issuer, audience and expiry checks can be handed to Nimbus, which handles clock skew:
processor.setJWTClaimsSetVerifier(new DefaultJWTClaimsVerifier<>(
properties.getAppId(),
new JWTClaimsSet.Builder().issuer(EXPECTED_ISSUER).build(),
Set.of("exp", "serviceUrl")));
| LOGGER.warn("Rejected Teams webhook JWT with unexpected audience {}", claims.getAudience()); | ||
| return false; | ||
| } | ||
| Date expiration = claims.getExpirationTime(); |
There was a problem hiding this comment.
This exp check duplicates what DefaultJWTProcessor already does, and it has no clock-skew tolerance. If the claims verifier above is added, this check (and the issuer and audience checks at lines 69–76) can be removed.
| return ResponseEntity.ok().build(); | ||
| } | ||
|
|
||
| String conversationId = activity.conversation() == null ? null : activity.conversation().id(); |
There was a problem hiding this comment.
conversationId can be null here, and it’s passed raw to agent.respondTo. Other channels use a prefix (telegram-). Suggest returning early if it’s null, and passing "teams-" + conversationId to the agent.
| } | ||
| TeamsConversationReference reference = conversationReference.get(); | ||
| if (reference == null) { | ||
| LOGGER.warn("No known Teams conversation yet, cannot send message '{}'", message); |
There was a problem hiding this comment.
Nit: these log the full message content at warn/error level. Consider leaving it out or truncating it.
| } | ||
|
|
||
| @Override | ||
| public String processStep(Map<String, String> formParams, Map<String, Object> session) { |
There was a problem hiding this comment.
Please add an allowed-user-id field here (required, given the blocking comment above) and persist it in saveConfiguration (line 67).
| implementation project(':base') | ||
| implementation 'org.springframework.boot:spring-boot-starter' | ||
| implementation 'org.springframework.boot:spring-boot-starter-webmvc' | ||
| implementation 'com.nimbusds:nimbus-jose-jwt:10.9.1' |
There was a problem hiding this comment.
Nit: Spring Boot already manages the nimbus-jose-jwt version, so :10.9.1 can be dropped.
| } | ||
|
|
||
| private boolean isAllowedUser(TeamsActivityPayload.From from) { | ||
| String allowed = properties.getAllowedUserId(); |
There was a problem hiding this comment.
When allowed-user-id is blank, every user is accepted, and onboarding never asks for this value. So a normal setup lets anyone in the tenant who can message the bot drive the agent, including its browser and CLI tools. Telegram rejects messages when there’s no match. Suggest:
if (allowed == null || allowed.isBlank()) {
return false;
}
and adding an “Allowed user ID” field to the onboarding step.
2 participants
Closes #80