[alexandreferris]

---

PR Submission Checklist for internal contributors

• The PR Title

  • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
  • contains a reference JIRA issue number like SQPIT-764
  • answers the question: If merged, this PR will: ... ³

• The PR Description

  • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

---

What's new in this PR?

Issues

Code had redis operations laying around in different parts of the codebase

Solutions

• Centralize Redis GET/SET operations in RedisRepository
• Update services and dependency injection to use the repository
• Add repository and credential behaviour tests

Testing

Test Coverage (Optional)

• I have added automated test to this contribution

[claude]

Review: Move Redis operations to RedisRepository

Nice refactor. Putting the key format behind one class and injecting it, instead of using GlobalContext.get() inside SignatureValidator, makes the code cleaner and easier to test. The new tests for RedisRepository, the generate-and-store path in GitHubCommandHandler, and SignatureValidator fill real gaps. A few comments:

🐛 Behavior change: a non-UUID conversation ID on the webhook route now returns 500 instead of 403

SignatureValidator.isValid now calls UUID.fromString(conversationId) before it looks anything up. That call throws IllegalArgumentException for any path segment that isn't a valid UUID. In Routing.kt:88-108 the route only catches IOException, and there's no StatusPages handler, so a request like POST /not-a-uuid/example.com now returns 500. Before this PR it got a Redis miss, then an IOException, then a 403.

The comment in the route says it returns 403 so that GitHub doesn't keep redelivering a permanently failing delivery. A 5xx undoes that, and anyone can trigger 500s and error logs just by guessing URLs.

Suggestion: parse the ID in the route the way the Actions route already does (Routing.kt:182-186), with runCatching { UUID.fromString(...) }, and return 400 or 403. Then change SignatureValidator.isValid to take a QualifiedId directly. That puts parsing at the HTTP boundary and keeps the two routes consistent. A test for an invalid conversation ID on the webhook route, like the existing /actions/not-a-uuid/... test, would cover this.

Minor: lookups now use the lowercased UUID

The webhook key used to be built from the raw path string. It's now built from UUID.toString(), which is lowercase. Secrets are stored under the lowercase form, so this is probably a small improvement: an uppercase UUID in the URL will now match. It's still a behavior change, so it might be worth a line in the PR description.

Naming consistency

The repository methods are getActionSecret/setActionSecret, but everywhere else, including the Redis key suffix (:actions-token), generateActionsToken, ActionsTokenValidator, and the /github actions command, calls this the "Actions token". Something like getActionsToken/setActionsToken would make it easier to search for. The new test name Actions help stores and returns a generated secret has the same mix of terms.

Optional: get-or-create isn't atomic (existing issue)

getOrCreateSecret/getOrCreateActionsToken still do a GET followed by a SET. If two help commands arrive for the same conversation at the same time, both can generate a secret, and the last write wins. The first user would then hold a token that no longer validates. This isn't new, but now that the logic lives in one repository, a getOrCreate… method using SET key value NX (storage.set(key, value, SetArgs().nx())) followed by a GET would fix it. Not blocking.

Small things

• RedisRepository sits in the root com.wire.github package next to EventsHandler and similar classes. A storage/repository package might be clearer as more persistence is added, but that's a matter of taste.
• In ApplicationTest, the single { redisRepository } binding plus explicitly passing redisRepository to the validators works fine. get() would mirror Modules.kt more closely, if you prefer.
• EventsHandler still resolves dependencies through GlobalContext. That's out of scope here, but it's the same pattern this PR removed from SignatureValidator, so it could be a follow-up.

Summary: The structure is good and so is the test coverage. Please fix the 500-on-invalid-UUID regression on the webhook route before merging. The rest are suggestions.

[spoonman01]

Yes good stuff. I would suggest going even further and try to remove completely ActionsTokenValidator and SignatureValidator.
They just have 2 functions wrapping the RedisRepository, they can be moved directly inside RedisRepository.

Maybe actually it's a RedisService (as there is business logic inside) but with just one class you have fetching / setting / validation of tokens and signatures, and the classes using that are abstracted away from the internal checks.

[alexandreferris]

Yes good stuff. I would suggest going even further and try to remove completely ActionsTokenValidator and SignatureValidator. They just have 2 functions wrapping the RedisRepository, they can be moved directly inside RedisRepository.

Maybe actually it's a RedisService (as there is business logic inside) but with just one class you have fetching / setting / validation of tokens and signatures, and the classes using that are abstracted away from the internal checks.

Awesome stuff! I changed to RedisService and kept all the logic there, also removing the webhook and actions validators and concentrating them all inside RedisService

[spoonman01]

Amazing