feat: add recaptcha functionality to API registration #574

nlwstein · 2023-02-02T15:53:46Z

Summary of changes

Asana Ticket: 🍎 Add recaptcha to signing up for API

This PR adds recaptcha functionality to the API, and implements it on the non-admin user create form.

A specific commit is pinned for the included library because the older published version causes dependency resolution issues.

NOTE: Before releasing this feature, we must create recaptcha keys on a production/shared Google account, and add the following environment variables to the various deployments: RECAPTCHA_PUBLIC_KEY, RECAPTCHA_PRIVATE_KEY.

…ture

github-actions · 2023-02-02T16:23:07Z

Coverage of commit `41dda13`

Summary coverage rate:
  lines......: 89.9% (3927 of 4368 lines)
  functions..: 70.7% (2159 of 3054 functions)
  branches...: no data found

Files changed coverage rate:
                                                                        |Lines       |Functions  |Branches    
  Filename                                                              |Rate     Num|Rate    Num|Rate     Num
  ============================================================================================================
  apps/api_web/lib/api_web/controllers/portal/user_controller.ex        |80.4%     46|87.5%    16|    -      0
  apps/api_web/lib/api_web/router.ex                                    |95.6%     45|58.7%   150|    -      0

Download coverage report

github-actions · 2023-02-02T18:09:22Z

Coverage of commit `c0a5c54`

Summary coverage rate:
  lines......: 89.9% (3926 of 4368 lines)
  functions..: 70.7% (2159 of 3054 functions)
  branches...: no data found

Files changed coverage rate:
                                                                        |Lines       |Functions  |Branches    
  Filename                                                              |Rate     Num|Rate    Num|Rate     Num
  ============================================================================================================
  apps/api_web/lib/api_web/controllers/portal/user_controller.ex        |80.4%     46|87.5%    16|    -      0
  apps/api_web/lib/api_web/router.ex                                    |95.6%     45|58.7%   150|    -      0

Download coverage report

nlwstein · 2023-02-02T18:22:29Z

@bklebe @paulswartz Do either of you have an opinion on how to handle the updated library assuming the external PR does not get merged? I am leaning towards just copying the code, with documentation in the README or something like that.

github-actions · 2023-02-02T18:28:25Z

Coverage of commit `0358fc1`

Summary coverage rate:
  lines......: 89.9% (3927 of 4368 lines)
  functions..: 70.7% (2159 of 3054 functions)
  branches...: no data found

Files changed coverage rate:
                                                                        |Lines       |Functions  |Branches    
  Filename                                                              |Rate     Num|Rate    Num|Rate     Num
  ============================================================================================================
  apps/api_web/lib/api_web/controllers/portal/user_controller.ex        |80.4%     46|87.5%    16|    -      0
  apps/api_web/lib/api_web/router.ex                                    |95.6%     45|58.7%   150|    -      0

Download coverage report

paulswartz

If it's not merged upstream, we can fork it into the mbta organization. We've done that for other dependencies in the past, and it'll probably help the MBTA.com team in the future as well.

apps/api_web/lib/api_web/templates/client_portal/user/_new.html.heex

apps/api_web/lib/api_web/controllers/portal/user_controller.ex

apps/api_web/lib/api_web/router.ex

…l.heex Co-authored-by: Paul Swartz <[email protected]>

github-actions · 2023-02-02T19:05:28Z

Coverage of commit `2d770e6`

Summary coverage rate:
  lines......: 89.9% (3926 of 4368 lines)
  functions..: 70.7% (2159 of 3054 functions)
  branches...: no data found

Files changed coverage rate:
                                                                        |Lines       |Functions  |Branches    
  Filename                                                              |Rate     Num|Rate    Num|Rate     Num
  ============================================================================================================
  apps/api_web/lib/api_web/controllers/portal/user_controller.ex        |80.4%     46|87.5%    16|    -      0
  apps/api_web/lib/api_web/router.ex                                    |95.6%     45|58.7%   150|    -      0

Download coverage report

github-actions · 2023-02-06T15:34:32Z

Coverage of commit `d9ff22b`

Summary coverage rate:
  lines......: 89.9% (3926 of 4368 lines)
  functions..: 70.7% (2159 of 3054 functions)
  branches...: no data found

Files changed coverage rate:
                                                                        |Lines       |Functions  |Branches    
  Filename                                                              |Rate     Num|Rate    Num|Rate     Num
  ============================================================================================================
  apps/api_web/lib/api_web/controllers/portal/user_controller.ex        |80.4%     46|87.5%    16|    -      0
  apps/api_web/lib/api_web/router.ex                                    |95.6%     45|58.7%   150|    -      0

Download coverage report

github-actions · 2023-02-06T16:18:24Z

Coverage of commit `5f44334`

Summary coverage rate:
  lines......: 89.9% (3926 of 4368 lines)
  functions..: 70.7% (2159 of 3054 functions)
  branches...: no data found

Files changed coverage rate:
                                                                        |Lines       |Functions  |Branches    
  Filename                                                              |Rate     Num|Rate    Num|Rate     Num
  ============================================================================================================
  apps/api_web/lib/api_web/controllers/portal/user_controller.ex        |80.4%     46|87.5%    16|    -      0
  apps/api_web/lib/api_web/router.ex                                    |95.6%     45|58.7%   150|    -      0

Download coverage report

paulswartz

@nlwstein code looks good! Can you work with @bklebe to get the creds so we can try this out in one of the dev environments?

bklebe · 2023-02-14T21:12:32Z

apps/api_web/config/config.exs

+  public_key: {:system, "RECAPTCHA_PUBLIC_KEY"},
+  secret: {:system, "RECAPTCHA_PRIVATE_KEY"}


suggestion: for dynamic configuration, we typically load it using System.get_env/2 in config/runtime.exs now.

suggestion: for dynamic configuration, we typically load it using System.get_env/2 in config/runtime.exs now.

I added the env vars to that file, and added a feature flag to disable the recaptcha code outside of prod builds.

I tried it out, and the first thing I noticed was that the form fields were cleared if I submitted without the captcha. Otherwise it seems to work well.

I added some frontend code to only enable the submit button once the user completes the recaptcha, so this won't happen outside of someone attempting to do something weird.

Deployed to dev-green 🙂

github-actions · 2023-02-22T14:02:03Z

Coverage of commit `e3153e8`

Summary coverage rate:
  lines......: 89.5% (3911 of 4371 lines)
  functions..: 70.6% (2158 of 3055 functions)
  branches...: no data found

Files changed coverage rate:
                                                                        |Lines       |Functions  |Branches    
  Filename                                                              |Rate     Num|Rate    Num|Rate     Num
  ============================================================================================================
  apps/api_web/lib/api_web/controllers/portal/user_controller.ex        |66.0%     47|87.5%    16|    -      0
  apps/api_web/lib/api_web/router.ex                                    |95.6%     45|58.0%   150|    -      0

Download coverage report

nlwstein · 2023-02-22T14:12:28Z

This is available for review on dev-green, at least for now. The secrets have been loaded to all environments.

paulswartz · 2023-02-22T14:19:51Z

I tried it out, and the first thing I noticed was that the form fields were cleared if I submitted without the captcha. Otherwise it seems to work well.

…ed recaptcha

github-actions · 2023-02-22T19:05:31Z

Coverage of commit `f1b117b`

Summary coverage rate:
  lines......: 89.5% (3917 of 4376 lines)
  functions..: 70.7% (2160 of 3056 functions)
  branches...: no data found

Files changed coverage rate:
                                                                        |Lines       |Functions  |Branches    
  Filename                                                              |Rate     Num|Rate    Num|Rate     Num
  ============================================================================================================
  apps/api_web/lib/api_web/controllers/portal/user_controller.ex        |71.2%     52|94.1%    17|    -      0
  apps/api_web/lib/api_web/router.ex                                    |95.6%     45|58.0%   150|    -      0

Download coverage report

github-actions · 2023-02-22T19:26:02Z

Coverage of commit `e193989`

Summary coverage rate:
  lines......: 89.5% (3916 of 4376 lines)
  functions..: 70.7% (2160 of 3056 functions)
  branches...: no data found

Files changed coverage rate:
                                                                        |Lines       |Functions  |Branches    
  Filename                                                              |Rate     Num|Rate    Num|Rate     Num
  ============================================================================================================
  apps/api_web/lib/api_web/controllers/portal/user_controller.ex        |71.2%     52|94.1%    17|    -      0
  apps/api_web/lib/api_web/router.ex                                    |95.6%     45|58.0%   150|    -      0

Download coverage report

github-actions · 2023-02-22T19:46:20Z

Coverage of commit `66f5ebc`

Summary coverage rate:
  lines......: 89.5% (3916 of 4376 lines)
  functions..: 70.7% (2160 of 3056 functions)
  branches...: no data found

Files changed coverage rate:
                                                                        |Lines       |Functions  |Branches    
  Filename                                                              |Rate     Num|Rate    Num|Rate     Num
  ============================================================================================================
  apps/api_web/lib/api_web/controllers/portal/user_controller.ex        |71.2%     52|94.1%    17|    -      0
  apps/api_web/lib/api_web/router.ex                                    |95.6%     45|58.0%   150|    -      0

Download coverage report

paulswartz

question: should there be some tests?

paulswartz · 2023-02-23T17:02:37Z

apps/api_web/lib/api_web/controllers/portal/user_controller.ex

-        |> put_session(:user_id, user.id)
-        |> configure_session(renew: true)
-        |> redirect(to: portal_path(conn, :index))
+    _create(conn, user_params, nil)


suggestion(non-blocking): You could combine these with Map.get which defaults to nil:

def create(conn, %{"user" => user_params} = params) do recaptcha = Map.get(params, "g-recaptcha-response") ... end

apps/api_web/lib/api_web/controllers/portal/user_controller.ex

Co-authored-by: Paul Swartz <[email protected]>

github-actions · 2023-02-24T13:26:06Z

Coverage of commit `d3c2654`

Summary coverage rate:
  lines......: 89.5% (3916 of 4376 lines)
  functions..: 70.7% (2160 of 3056 functions)
  branches...: no data found

Files changed coverage rate:
                                                                        |Lines       |Functions  |Branches    
  Filename                                                              |Rate     Num|Rate    Num|Rate     Num
  ============================================================================================================
  apps/api_web/lib/api_web/controllers/portal/user_controller.ex        |71.2%     52|94.1%    17|    -      0
  apps/api_web/lib/api_web/router.ex                                    |95.6%     45|58.0%   150|    -      0

Download coverage report

github-actions · 2023-02-24T13:55:16Z

Coverage of commit `289944e`

Summary coverage rate:
  lines......: 89.5% (3917 of 4376 lines)
  functions..: 70.7% (2160 of 3056 functions)
  branches...: no data found

Files changed coverage rate:
                                                                        |Lines       |Functions  |Branches    
  Filename                                                              |Rate     Num|Rate    Num|Rate     Num
  ============================================================================================================
  apps/api_web/lib/api_web/controllers/portal/user_controller.ex        |71.2%     52|94.1%    17|    -      0
  apps/api_web/lib/api_web/router.ex                                    |95.6%     45|58.0%   150|    -      0

Download coverage report

nlwstein · 2023-02-24T13:56:28Z

question: should there be some tests?

Definitely! I added one that proves that the widget shows up when supplying the test creds: https://github.com/mbta/api/pull/574/files#diff-740356136f3337b207b35a2c46f8b47dcab0944918c977f6e8e0d04bd9eab52dR11.

Due to the nature of ReCaptcha, a true end to end test isn't feasible, but this should ensure the path it takes is laid out correctly if that makes sense 🙂

github-actions · 2023-02-24T14:02:14Z

Coverage of commit `bd56f9c`

Summary coverage rate:
  lines......: 89.5% (3917 of 4376 lines)
  functions..: 70.7% (2160 of 3056 functions)
  branches...: no data found

Files changed coverage rate:
                                                                        |Lines       |Functions  |Branches    
  Filename                                                              |Rate     Num|Rate    Num|Rate     Num
  ============================================================================================================
  apps/api_web/lib/api_web/controllers/portal/user_controller.ex        |71.2%     52|94.1%    17|    -      0
  apps/api_web/lib/api_web/router.ex                                    |95.6%     45|58.0%   150|    -      0

Download coverage report

nlwstein · 2023-02-24T14:02:52Z

@paulswartz I accepted your proposed change on one of your comments so the conversation got swallowed, but with regard to the false question: the library returns an object like this by default.

I went with false but perhaps nil would be a better choice: I don't want any code to confuse our dev mode path for a real ReCaptcha response. If that is too loosely typed, I could return their object with all the fields set to nil or something like that?

But really, we should only ever evaluate whether it gives :ok or :error which can happen here when there's an API issue, most likely bad keys or here when the challenge fails. The result in that other object would only be something we would want to look at as devs vs. presenting on the frontend I think.

paulswartz

🍰

nlwstein added 4 commits February 2, 2023 10:47

wip: add nlwstein/recaptcha fork temporarily to support recaptcha fea…

925b877

…ture

wip: basic recaptcha functionality

fc3fc03

wip: resolve credo complaints

7219602

wip: better error handling, passes dialyzer

41dda13

wip: remove js disable

c0a5c54

wip: better formatting for recaptcha

0358fc1

nlwstein requested a review from bfauble February 2, 2023 18:21

paulswartz reviewed Feb 2, 2023

View reviewed changes

apps/api_web/lib/api_web/templates/client_portal/user/_new.html.heex Outdated Show resolved Hide resolved

apps/api_web/lib/api_web/controllers/portal/user_controller.ex Outdated Show resolved Hide resolved

apps/api_web/lib/api_web/router.ex Outdated Show resolved Hide resolved

nlwstein and others added 2 commits February 2, 2023 13:52

Update apps/api_web/lib/api_web/templates/client_portal/user/_new.htm…

29cc03c

…l.heex Co-authored-by: Paul Swartz <[email protected]>

wip: revert default-src

2d770e6

wip: attempt to use specific commit on original repo

d9ff22b

wip: use with statement for recaptcha logic

5f44334

nlwstein marked this pull request as ready for review February 6, 2023 16:19

nlwstein requested a review from paulswartz February 8, 2023 21:15

paulswartz reviewed Feb 9, 2023

View reviewed changes

nlwstein temporarily deployed to dev-blue February 14, 2023 18:10 — with GitHub Actions Inactive

nlwstein temporarily deployed to dev February 14, 2023 18:46 — with GitHub Actions Inactive

bklebe reviewed Feb 14, 2023

View reviewed changes

nlwstein had a problem deploying to dev-green February 21, 2023 19:59 — with GitHub Actions Error

nlwstein temporarily deployed to dev-green February 21, 2023 20:17 — with GitHub Actions Inactive

nlwstein had a problem deploying to dev-blue February 21, 2023 20:23 — with GitHub Actions Error

nlwstein temporarily deployed to dev-green February 21, 2023 20:27 — with GitHub Actions Inactive

Merge branch 'master' into nlws-registration-recaptcha

ac6ce1a

nlwstein added 3 commits February 21, 2023 17:41

wip: only show recaptcha when MIX_ENV is prod

40beccb

wip: added feature flag and moved env var loading

21b1e82

chore: apply formatter

e3153e8

nlwstein temporarily deployed to dev-green February 22, 2023 14:02 — with GitHub Actions Inactive

nlwstein requested a review from bklebe February 22, 2023 14:19

nlwstein removed request for bfauble and bklebe February 22, 2023 17:46

wip: better dev mode behavior, added changeset preservation when fail…

f1b117b

…ed recaptcha

wip: disable submit until recaptcha complete

e193989

nlwstein temporarily deployed to dev-green February 22, 2023 19:27 — with GitHub Actions Inactive

wip: move function definition so the higher specificity one comes first

66f5ebc

nlwstein temporarily deployed to dev-green February 22, 2023 19:40 — with GitHub Actions Inactive

nlwstein requested a review from paulswartz February 22, 2023 21:02

paulswartz reviewed Feb 23, 2023

View reviewed changes

Update apps/api_web/lib/api_web/controllers/portal/user_controller.ex

d3c2654

Co-authored-by: Paul Swartz <[email protected]>

nlwstein added 2 commits February 24, 2023 08:48

test: ensure recaptcha widget is displayed when feature is enabled

289944e

test: add testing credentials for recaptcha

bd56f9c

paulswartz approved these changes Feb 24, 2023

View reviewed changes

nlwstein merged commit 66a3550 into master Feb 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add recaptcha functionality to API registration #574

feat: add recaptcha functionality to API registration #574

nlwstein commented Feb 2, 2023 •

edited

Loading

github-actions bot commented Feb 2, 2023

github-actions bot commented Feb 2, 2023

nlwstein commented Feb 2, 2023 •

edited

Loading

github-actions bot commented Feb 2, 2023

paulswartz left a comment

github-actions bot commented Feb 2, 2023

github-actions bot commented Feb 6, 2023

github-actions bot commented Feb 6, 2023

paulswartz left a comment

bklebe Feb 14, 2023

nlwstein Feb 22, 2023

nlwstein Feb 22, 2023

github-actions bot commented Feb 22, 2023

nlwstein commented Feb 22, 2023 •

edited

Loading

paulswartz commented Feb 22, 2023

github-actions bot commented Feb 22, 2023

github-actions bot commented Feb 22, 2023

github-actions bot commented Feb 22, 2023

paulswartz left a comment

paulswartz Feb 23, 2023

github-actions bot commented Feb 24, 2023

github-actions bot commented Feb 24, 2023

nlwstein commented Feb 24, 2023 •

edited

Loading

github-actions bot commented Feb 24, 2023

nlwstein commented Feb 24, 2023 •

edited

Loading

paulswartz left a comment

		public_key: {:system, "RECAPTCHA_PUBLIC_KEY"},
		secret: {:system, "RECAPTCHA_PRIVATE_KEY"}

feat: add recaptcha functionality to API registration #574

feat: add recaptcha functionality to API registration #574

Conversation

nlwstein commented Feb 2, 2023 • edited Loading

Summary of changes

github-actions bot commented Feb 2, 2023

Coverage of commit 41dda13

github-actions bot commented Feb 2, 2023

Coverage of commit c0a5c54

nlwstein commented Feb 2, 2023 • edited Loading

github-actions bot commented Feb 2, 2023

Coverage of commit 0358fc1

paulswartz left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 2, 2023

Coverage of commit 2d770e6

github-actions bot commented Feb 6, 2023

Coverage of commit d9ff22b

github-actions bot commented Feb 6, 2023

Coverage of commit 5f44334

paulswartz left a comment

Choose a reason for hiding this comment

bklebe Feb 14, 2023

Choose a reason for hiding this comment

nlwstein Feb 22, 2023

Choose a reason for hiding this comment

nlwstein Feb 22, 2023

Choose a reason for hiding this comment

github-actions bot commented Feb 22, 2023

Coverage of commit e3153e8

nlwstein commented Feb 22, 2023 • edited Loading

paulswartz commented Feb 22, 2023

github-actions bot commented Feb 22, 2023

Coverage of commit f1b117b

github-actions bot commented Feb 22, 2023

Coverage of commit e193989

github-actions bot commented Feb 22, 2023

Coverage of commit 66f5ebc

paulswartz left a comment

Choose a reason for hiding this comment

paulswartz Feb 23, 2023

Choose a reason for hiding this comment

github-actions bot commented Feb 24, 2023

Coverage of commit d3c2654

github-actions bot commented Feb 24, 2023

Coverage of commit 289944e

nlwstein commented Feb 24, 2023 • edited Loading

github-actions bot commented Feb 24, 2023

Coverage of commit bd56f9c

nlwstein commented Feb 24, 2023 • edited Loading

paulswartz left a comment

Choose a reason for hiding this comment

nlwstein commented Feb 2, 2023 •

edited

Loading

Coverage of commit `41dda13`

Coverage of commit `c0a5c54`

nlwstein commented Feb 2, 2023 •

edited

Loading

Coverage of commit `0358fc1`

Coverage of commit `2d770e6`

Coverage of commit `d9ff22b`

Coverage of commit `5f44334`

Coverage of commit `e3153e8`

nlwstein commented Feb 22, 2023 •

edited

Loading

Coverage of commit `f1b117b`

Coverage of commit `e193989`

Coverage of commit `66f5ebc`

Coverage of commit `d3c2654`

Coverage of commit `289944e`

nlwstein commented Feb 24, 2023 •

edited

Loading

Coverage of commit `bd56f9c`

nlwstein commented Feb 24, 2023 •

edited

Loading