Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add MFA (Phone) related config #2582

Merged
merged 11 commits into from
Aug 13, 2024
Merged

Conversation

J0
Copy link
Contributor

@J0 J0 commented Aug 1, 2024

What kind of change does this PR introduce?

Introduces the supporting Auth configuration for MFA (Phone). This include:

  1. The respective enable / disable flows for each toggle MFA.Phone.EnrollEnabled and MFA.Phone.VerifyEnabled
  2. MFA.MaxEnrolledFactors which is already exposed on the platform
  3. Phone also comes with a configurable OTP Length as well as Template. We don't include the provider specific configuration as they are derived from the primary provider. We also expose MaxFrequency

@J0 J0 force-pushed the j0/add_mfa_phone_config branch from 3dca11a to 63ea584 Compare August 4, 2024 10:09
@coveralls
Copy link

coveralls commented Aug 4, 2024

Pull Request Test Coverage Report for Build 10363863786

Details

  • 6 of 13 (46.15%) changed or added relevant lines in 1 file are covered.
  • 8 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.07%) to 60.053%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/start/start.go 6 13 46.15%
Files with Coverage Reduction New Missed Lines %
internal/debug/postgres.go 3 64.86%
internal/gen/keys/keys.go 5 12.9%
Totals Coverage Status
Change from base Build 10355148573: -0.07%
Covered Lines: 6341
Relevant Lines: 10559

💛 - Coveralls

internal/start/start.go Outdated Show resolved Hide resolved
Copy link
Contributor

@hf hf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine.

@J0
Copy link
Contributor Author

J0 commented Aug 6, 2024

How this was tested:

Place this set of entries in the config.toml

[auth.mfa.totp]
enroll_enabled = true
verify_enabled = true

[auth.mfa]
max_enrolled_factors = 20

[auth.mfa.phone]
enroll_enabled = true
verify_enabled = true
otp_length = 6
template = "Your Code is {{ .Code }}"
max_frequency = "10s"

Run docker inspect on the Auth container and check that the Auth config is appropriately set.

Seeking early review, will only take / merge this PR once we have upgraded the Auth version on all projects across the board.

@J0 J0 marked this pull request as ready for review August 6, 2024 07:48
@J0 J0 requested a review from a team as a code owner August 6, 2024 07:48
pkg/config/config.go Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
@J0
Copy link
Contributor Author

J0 commented Aug 7, 2024

Deploy has completed

@J0 J0 requested a review from sweatybridge August 9, 2024 02:12
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Show resolved Hide resolved
@J0 J0 force-pushed the j0/add_mfa_phone_config branch from ffd3b93 to c0d4349 Compare August 12, 2024 23:46
@sweatybridge sweatybridge merged commit 19f5995 into develop Aug 13, 2024
13 checks passed
@sweatybridge sweatybridge deleted the j0/add_mfa_phone_config branch August 13, 2024 05:06
@github-actions github-actions bot mentioned this pull request Aug 14, 2024
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.

4 participants