[Issue #2653] Sign in component #3281

acouch · 2024-12-18T14:41:23Z

Summary

Fixes #2653

Time to review: 5 mins

Changes proposed

Mobile

Figma

Local

Deskstop

Figma

Local

coilysiren · 2024-12-18T16:47:54Z

infra/frontend/app-config/env-config/environment-variables.tf

@@ -44,6 +44,11 @@ locals {
      manage_method     = "manual"
      secret_store_name = "/${var.app_name}/${var.environment}/api-auth-token"
    },
+    # URL for the API login route. 
+    AUTH_LOGIN_URL = {


This isn't secret so it goes next to the NODE_OPTIONS key above

It is the same as the API_URL right?

@coilysiren Secrets knows how to read from SSM where as the standard env vars are more "constants?" Right? Or is there a way to read from SSM there too?

doug-s-nava

can we confirm that the sign in button should get the same hover indicator as the nav items? seems slightly weird to me to have two items showing the same indicator at the same time:

frontend/src/components/Header.tsx

doug-s-nava · 2024-12-18T16:49:58Z

frontend/.env.production

@@ -11,4 +11,5 @@ SENDY_API_KEY=
 SENDY_API_URL=
 SENDY_LIST_ID=

-API_URL=http://api.simpler.grants.gov
+API_URL=https://api.simpler.grants.gov
+AUTH_LOGIN_URL=https://api.simpler.grants.gov/v1/users/login


it's a bit confusing to have this defined in the env file and in SSM. I'm not sure which one takes precedence (looks like it's the env file but haven't tested with Docker) and I'd worry that dev and staging environments would end up pointing to prod for auth with this defined here

Yeah, I guess we have the API one to point to that the behavior is "as expected" but that likely also reinforces that they don't need to be here?

It appears this gets picked up at build time, even though it's in a client component, because... I'm not sure how or why? Which also makes me question how staging is not finding Prod, as at that build it definitely doesn't have access to the SSM stuff (and I can't double check what SSM has for Stage cause I can't SSO into AWS right now).

Maybe, I think, perhaps, the distinction here is Search is doing this from a server component so it's picking up the live running value via SSM where as Login is client so it's getting rendered at build time? It's not inlining the variable (because it's not NEXT_PUBLIC) it's just built the whole HTML structure out and doesn't re-do it at run-time when the ENV VAR gets changed? So in this case it would make this send Dev and Stage login requests to the Prod API URL because that's all it has at build, via this file, not via SSM?

I've unfortunately fought this stuff a bunch with the new relic work. It might be good to talk this out if folks have time

doug-s-nava · 2024-12-18T16:56:01Z

frontend/src/components/Header.tsx

+  const LoginLink = () => {
+    return (
+      <Link
+        href={process.env.auth_login_url as string}


this works with next dev where the variable is defined in the env file, but won't work with build && start when it's coming from SSM. The env var won't be available to the app until run time, and for a client component to pick up directly from process.env the variable needs to be:

prefixed with NEXT_PUBLIC and

available at build time

I'd recommend reading this in from process.env in the environments file, then prop drilling it down to the header component from the nearest server component. It may work to import directly from the environments file, but I doubt it.

I wish local Next/React did a better job of highlighting/blocking this anti-pattern.

@doug-s-nava b/c these pages are statically rendered, there isn't a server side place to put the env for the static pages, unless we use wrap them in a suspense boundary. @mdragon that isn't an anti-pattern, it is the rec from next documentation

That could be the way to go. As of this comment, the CDN is rendering the suspense on the search page without a loading state, meaning the header could potentially use suspense without a "flicker."

If we switch to dynamic rendering, then we could just use this pattern as is.

doug-s-nava · 2024-12-18T16:56:13Z

frontend/next.config.js

@@ -68,6 +68,9 @@ const nextConfig = {
      },
    ];
  },
+  env: {
+    auth_login_url: process.env.AUTH_LOGIN_URL,


what is this doing for us?

The same thing the src/constants/environments.ts is doing. Initially I had this as a NEXT_PUBLIC_ var and that file says home for all interpreted server side environment variables. I'll move this over there, and update that comment since NEXT_PUBLIC files are defined there anyway. Good catch, thanks.

doug-s-nava · 2024-12-18T16:57:37Z

frontend/tests/components/Header.test.tsx

@@ -10,6 +10,8 @@ const props = {
  locale: "en",
 };

+process.env.auth_login_url = "/login-url";


can we mock the environments export instead? that's more in line with what we're doing in tests elsewhere

Seeing that there is a mockProcessEnv() function we aren't using and an example of mocking the environment.ts file in FeatureFlagManager.test.ts which I started off as not using, so no clear pattern yet. I'll use the environments.ts file and follow the pattern in the FeatureFlagManager.test.ts file. Good catch, thanks.

frontend/src/components/Header.tsx

doug-s-nava · 2024-12-18T17:16:47Z

frontend/src/styles/_uswds-theme-custom-styles.scss

@@ -30,3 +30,8 @@ i.e.
    @include u-shadow(4);
  }
 }
+@include at-media("desktop") {


Imagine you've tried some things out, but is there a way to avoid this? Can we mirror styling of the navbar items, or is there something throwing this off that is too difficult to deal with?

There is no 5px margin from USWDS. The extra pixel did make a difference IMHO.

frontend/src/components/Header.tsx

doug-s-nava · 2024-12-18T17:22:21Z

Also if we're not updating the button to a logged in state when user is logged in in this change I think we need a ticket to handle that

acouch · 2024-12-20T15:58:28Z

Also if we're not updating the button to a logged in state when user is logged in in this change I think we need a ticket to handle that

@doug-s-nava this seemed like enough to chew on for a PR and the provider wasn't available when I started. I can do a follow-up for the same ticket now that the user provider is available.

* adds route at /api/auth/callback to be hit by the flask API at the completion of the login flow * adds jwt token parsing logic * adds support for a "SESSION_SECRET" env var * adds a User page for temporary testing

acouch requested review from andycochran, btabaska, doug-s-nava, coilysiren and mdragon as code owners December 18, 2024 14:41

acouch force-pushed the acouch/issue-2653-login-component branch from 3d6abd3 to dd80b68 Compare December 18, 2024 16:01

mdragon approved these changes Dec 18, 2024

View reviewed changes

coilysiren reviewed Dec 18, 2024

View reviewed changes

doug-s-nava reviewed Dec 18, 2024

View reviewed changes

acouch force-pushed the acouch/issue-2653-login-component branch from 5b3b8af to a28c443 Compare December 19, 2024 13:46

doug-s-nava force-pushed the feature/nextjs-auth branch from 80f4046 to 99f501f Compare December 30, 2024 21:19

doug-s-nava requested review from chouinar and widal001 as code owners December 30, 2024 21:19

doug-s-nava force-pushed the feature/nextjs-auth branch from ec9bc75 to 9016bbc Compare January 2, 2025 17:20

doug-s-nava mentioned this pull request Jan 2, 2025

[Issue #3249] Feature flag manager refactor #3374

Open

1 task

doug-s-nava and others added 8 commits January 2, 2025 16:21

[Issue #2654] Next login flow backend (#3182)

71eaf1c

* adds route at /api/auth/callback to be hit by the flask API at the completion of the login flow * adds jwt token parsing logic * adds support for a "SESSION_SECRET" env var * adds a User page for temporary testing

Add sign in link

b9cfc1b

Update Header.tsx and styles

b64362c

Update Header.tsx for pa11y fix

5351586

Update environment-variables.tf for envar

ad40438

Update index.spec.ts for nav items

13c2aa4

Update index.spec.ts for e2e fix

387efeb

Update Header.tsx

0179346

acouch force-pushed the acouch/issue-2653-login-component branch from a28c443 to 0179346 Compare January 2, 2025 21:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue #2653] Sign in component #3281

[Issue #2653] Sign in component #3281

acouch commented Dec 18, 2024 •

edited

Loading

coilysiren Dec 18, 2024

acouch Dec 18, 2024

mdragon Dec 19, 2024

doug-s-nava left a comment

doug-s-nava Dec 18, 2024

mdragon Dec 19, 2024

mdragon Dec 20, 2024

mdragon Dec 20, 2024

doug-s-nava Dec 20, 2024

doug-s-nava Dec 18, 2024

mdragon Dec 19, 2024 •

edited

Loading

acouch Dec 20, 2024

doug-s-nava Dec 18, 2024

acouch Dec 20, 2024

doug-s-nava Dec 18, 2024

acouch Dec 20, 2024

doug-s-nava Dec 18, 2024

acouch Dec 20, 2024

doug-s-nava commented Dec 18, 2024

acouch commented Dec 20, 2024

[Issue #2653] Sign in component #3281

Are you sure you want to change the base?

[Issue #2653] Sign in component #3281

Conversation

acouch commented Dec 18, 2024 • edited Loading

Summary

Time to review: 5 mins

Changes proposed

Mobile

Figma

Local

Deskstop

Figma

Local

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doug-s-nava left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdragon Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doug-s-nava commented Dec 18, 2024

acouch commented Dec 20, 2024

acouch commented Dec 18, 2024 •

edited

Loading

mdragon Dec 19, 2024 •

edited

Loading