Skip to content

Conversation

superstructor
Copy link

Summary

  • Add ModicaSMS provider implementing controller.SMSer interface
  • Support provider selection via AUTH_SMS_PROVIDER environment variable
  • Maintain backward compatibility with existing Twilio configuration
  • Add extensive test coverage including mock server and validation tests

This is a re-creation of #667 incorporating the feedback from @dbarrosop, sorry I broke my fork repo.

Configuration

New environment variables:

  • AUTH_SMS_MODICA_USERNAME - Modica API username
  • AUTH_SMS_MODICA_PASSWORD - Modica API password

Provider selection:

  • Set AUTH_SMS_PROVIDER=modica to use Modica Group
  • Defaults to twilio for backward compatibility

Technical Details

  • API Integration: Uses Modica REST v2 API with HTTPS and Basic Authentication
  • Input Validation: Enforces international phone format (+ prefix required)
  • Error Handling: Parses structured API error responses with fallback for malformed responses
  • HTTP Client: Production configuration with 30s timeout, connection pooling, and TLS settings
  • Testing: Comprehensive test suite with mock server, validation tests, and integration test support

Test Coverage

  • Unit tests for provider initialization and SMS sending
  • Mock server tests for various API response scenarios
  • Input validation tests for phone format and message content
  • Integration tests (requires environment variables)
  • Error handling tests for API failures and malformed responses

Before submitting this PR:

Checklist

  • No breaking changes
  • Tests pass
  • New features have new tests
  • Documentation is updated

Breaking changes

No breaking changes. The implementation maintains full backward compatibility with existing Twilio configurations and defaults to Twilio when no
provider is specified.

Tests

  • All existing tests pass with go test -v ./...
  • New comprehensive test suite for Modica provider with 100% coverage
  • Mock server tests validate API request/response format
  • Integration tests support real API testing with credentials
  • Input validation tests ensure proper error handling

Documentation

  • Updated environment-variables.md with new Modica configuration options
  • Updated AUTH_SMS_PROVIDER documentation to show both twilio and modica options
  • Added documentation for AUTH_SMS_MODICA_USERNAME and AUTH_SMS_MODICA_PASSWORD
  • Code follows existing patterns and conventions from CLAUDE.md guidelines (as DEVELOPER.md seemed outdated)

Test Plan

  • Verify Twilio SMS still works (backward compatibility)
  • Test Modica SMS with valid credentials
  • Confirm proper error messages for invalid phone formats
  • Validate provider switching via environment variable
  • Run full test suite: go test -v ./go/notifications/sms/

Warning

Despite extensive tests I havn't actually done e2e testing in the nhost stack (e..g cloud) with this setup and would appreciate your help with doing so, I can provide test Modica group credentials on DM if needed.

@superstructor
Copy link
Author

I just need to work out the lint errors I should fix, and those I should leave alone,

Lint from prior to my commit:

golangci-lint run ./...
go/controller/sign_in_pat_test.go:26:15: string `\x9698157153010b858587119503cbeef0cf288f11775e51cdb6bfd65e930d9310` has 3 occurrences, make it a constant (goconst)
        hashedPat := `\x9698157153010b858587119503cbeef0cf288f11775e51cdb6bfd65e930d9310`
                     ^
go/controller/sign_in_webauthn_test.go:22:24: string `EuKJAraRGDcmHon-EjDoqoU5Yvk` has 3 occurrences, make it a constant (goconst)
        credentialIDString := "EuKJAraRGDcmHon-EjDoqoU5Yvk" //nolint:gosec
                              ^
go/controller/elevate_webauthn_test.go:49:54: directive `//nolint:gosec,goconst` is unused for linter "goconst" (nolintlint)
        credentialIDString := "EuKJAraRGDcmHon-EjDoqoU5Yvk" //nolint:gosec,goconst
                                                            ^
go/controller/refresh_token_test.go:58:86: directive `//nolint:goconst` is unused for linter "goconst" (nolintlint)
        hashedToken := `\x9698157153010b858587119503cbeef0cf288f11775e51cdb6bfd65e930d9310` //nolint:goconst
                                                                                            ^
go/testhelpers/gocmp.go:7:1: directive `//nolint:ireturn` is unused for linter "ireturn" (nolintlint)
//nolint:ireturn
^
5 issues:
* goconst: 2
* nolintlint: 3

Lint after my commit:

golangci-lint run ./...
go/controller/sign_in_pat_test.go:26:15: string `\x9698157153010b858587119503cbeef0cf288f11775e51cdb6bfd65e930d9310` has 3 occurrences, make it a constant (goconst)
        hashedPat := `\x9698157153010b858587119503cbeef0cf288f11775e51cdb6bfd65e930d9310`
                     ^
go/controller/sign_in_provider_test.go:134:31: string `.Location` has 3 occurrences, make it a constant (goconst)
                                                return last.String() == ".Location" || last.String() == ".SetCookie"
                                                                        ^
go/controller/sign_in_webauthn_test.go:22:24: string `EuKJAraRGDcmHon-EjDoqoU5Yvk` has 3 occurrences, make it a constant (goconst)
        credentialIDString := "EuKJAraRGDcmHon-EjDoqoU5Yvk" //nolint:gosec
                              ^
go/controller/elevate_webauthn_test.go:49:54: directive `//nolint:gosec,goconst` is unused for linter "goconst" (nolintlint)
        credentialIDString := "EuKJAraRGDcmHon-EjDoqoU5Yvk" //nolint:gosec,goconst
                                                            ^
go/controller/refresh_token_test.go:58:86: directive `//nolint:goconst` is unused for linter "goconst" (nolintlint)
        hashedToken := `\x9698157153010b858587119503cbeef0cf288f11775e51cdb6bfd65e930d9310` //nolint:goconst
                                                                                            ^
go/controller/sign_in_provider_callback_get_test.go:1210:43: directive `//nolint:goconst` is unused for linter "goconst" (nolintlint)
                                                return last.String() == ".Location" //nolint:goconst
                                                                                    ^
go/testhelpers/gocmp.go:7:1: directive `//nolint:ireturn` is unused for linter "ireturn" (nolintlint)
//nolint:ireturn
^
7 issues:
* goconst: 3
* nolintlint: 4

@superstructor
Copy link
Author

@dbarrosop I've added an additional commit that resolves lint / removes goconst lint supressions in favour of extracting repeating strings into consts, but it increases scope as touches a bunch of files unrelated to my change. Let me know if that approach is too heavy and I'll dial it back.

golines -w --base-formatter=gofumpt . passes

@dbarrosop
Copy link
Member

@superstructor yeah, please, remove the linter fixes that are unrelated to this wo. The issue is that there are a couple of linters that are not very deterministic under some circumstances and I was thinking in disabling those linters in those circumstances instead so don't worry about them, I will fix them myself and push to your branch os ask you to rebase afterwards. Thanks!

@dbarrosop
Copy link
Member

what the... I rebased your branch because I merged a couple of other PRs that caused several conflicts and git closed your PR... let me see if I can figure it out...

@dbarrosop
Copy link
Member

Ok, looks like the issue might be related to the fact your PR came from your main branch... Anyway, could you try to open this PR:

https://github.com/nhost/hasura-auth/compare/superstructor/modica?expand=1

I don't know if you will be able, otherwise feel free to copy the contents, or cherry-pick or whatever you think it's easier. Just want to make sure the git history credits you for your work...

@superstructor
Copy link
Author

no worries @dbarrosop , I also had some bizzare issues with my fork, probably user error on my part 🤦🏻 I'll fix and open a fresh PR incorporating all feedback to date today.

@dbarrosop
Copy link
Member

Yeah, I don't know if it's a git or a github issue but rebasing/committing as a maintainer on PR branches when the PR is coming from the default branch is always a mess. It is fine if the PR came from a different branch on the fork so if you need to open a new PR, please, try to do it from a branch instead of main as that usually just works if I need to do some small push/change myself before merging. Thanks again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants