[Issue #3247] logged in header #3458

doug-s-nava · 2025-01-08T14:19:44Z

Summary

Fixes #3247

Time to review: 45 mins

Changes proposed

updates to session management to allow for decrypting the login.gov JWT in order to pull out user email
creation of a logged in state for the header as per requirements in https://www.figma.com/design/FGxWtAgToKhehLJCiuy1zL/Simpler.Grants.gov-Project?node-id=795-16668&p=f&t=15lzPw6dJlQGDjUi-0

Context for reviewers

Notes

I made as much as possible of USWDS styling, but given that we are going against the grain in a number of ways with this nav implementation I ended up needing to create a few override styles. If anyone sees a way to work around these styles with build in USWDS stuff, that'd be great, but I reached the limit of my powers on a couple of these
while doing this work I created a hook for JS side media queries that I ended up not needing. In case we need it in the future it's here https://github.com/HHS/simpler-grants-gov/compare/use-media-query?expand=1
note that in building this functionality I had some difficulty working around the initialization of the encoded secrets, which relates to testing. If we are ok with leaving this functionality untestable, then this complexity I created here isn't necessary, but if we want to test that the env vars are being encoded properly, we need some way to encode them independent of the file initialization. We also potentially have the difficulty of handling cases where the keys are not available (such as when the session code runs during build). Here's what I tried to work around this that didn't work:
- make a session manager class. This did some things for us, but in the end, we'd need to initialize the class outside of the file, which would entail a lot of useless class instances floating around rather than a single class instance which would be the goal
- use next instrumentation with an initializer function. This also did some things, and effectively stored the encoded keys in memory on server startup. The problem is that the memory and keys got wiped during runtime when the module reloaded for some reason
does not include new e2e tests, ticket for that work is here e2e tests for login / logout #3459

Test steps

Refer to screenshots below throughout

if you haven't yet, run make setup-env-override-file in api
grab the API_JWT_PUBLIC_KEY value from your override.env file
start a local server on this branch with npm run build && SESSION_SECRET='anything' AUTH_LOGIN_URL=http://localhost:8080/v1/users/login API_JWT_PUBLIC_KEY="<yr public key>" FEATURE_AUTH_ON=true npm run start
visit localhost:3000
click sign in
enter any value and click the button to simulate login.gov sign in
VERIFY: you are returned to the User page with a success message
VERIFY: the header shows a logged in state including a fake email
click the email drop down
VERIFY: a sign out option appears
shrink viewport to mobile size
VERIFY: email disappears from header, but drop down still exists where it should with just the icon
click the icon
VERIFY: email and sign out link appear in drop down
click sign out
VERIFY: header returns to logged out state
VERIFY: all screens match Figma designs

Additional information

Logged out

Logged in

Logged in drop down

Logged in mobile

Logged in mobile drop down

…private vs secret jwk

doug-s-nava · 2025-01-08T15:19:17Z

frontend/src/components/USWDSIcon.tsx

@@ -12,7 +14,7 @@ const sprite_uri = SpriteSVG.src as string;
 export function USWDSIcon(props: IconProps) {
  return (
    <svg
-      className={props.className}
+      className={clsx("usa-icon", props.className)}


since all usage of this component had this class added manually, figured we should add it to the shared implementation and make this component a bit more useful. Removal of "usa-icon" from other files is a result of this change

doug-s-nava · 2025-01-08T15:19:59Z

frontend/src/app/[locale]/dev/feature-flags/FeatureFlagsTable.tsx

 /**
 * View for managing feature flags
 */
 export default function FeatureFlagsTable() {
  const { setFeatureFlag, featureFlags } = useFeatureFlags();
-  const { user, isLoading, error } = useUser();


with a working header we no longer need these changes on the feature flag page as a proof of concept

doug-s-nava · 2025-01-08T15:20:24Z

frontend/src/app/api/auth/session/route.ts

@@ -2,12 +2,12 @@ import { getSession } from "src/services/auth/session";

 import { NextResponse } from "next/server";

+export const revalidate = 0;


we don't want to cache anything here

Not sure if this has an effect since a fetch isn't called in the session creation AFAICT. Next says it doesn't cache GET routes if cookies are used. force-dynamic might be the way to make sure that the GET request is re-rendered every time, but can't hurt to tell it to revalidate as well.

Also wondering if this is better as a POST method.

yeah, I had this in there just for safety, but I just verified and the 'no-store' on the fetch call is doing all the work. We can safely pull this out. As for POST vs GET, I'm not sure I see a reason to make a change, but we can discuss

doug-s-nava · 2025-01-08T15:21:00Z

frontend/src/components/Layout.tsx

-      <Footer />
-      <GrantsIdentifier />
-    </div>
+    <UserProvider>


we could be more targeted here, but I don't see a need

doug-s-nava · 2025-01-08T15:22:01Z

frontend/src/components/opportunity/OpportunityStatusWidget.tsx

@@ -62,7 +62,7 @@ const OpportunityStatusWidget = ({ opportunityData }: Props) => {
    switch (status) {
      case "archived":
        return (
-          <div className="usa-tag bg-base-lighter text-ink border-radius-2 border-base-lightest width-100 radius-md margin-right-0 font-sans-sm text-center text-no-uppercase">


width-100 isn't a valid uswds class

doug-s-nava · 2025-01-08T15:26:36Z

frontend/src/services/auth/sessionUtils.ts

@@ -0,0 +1,46 @@
+import { KeyObject } from "crypto";


where I ended up is a little confusing, but the idea was to split out functionality that needs access to JWT keys from functionality that doesn't in order to help with testing and organization. I think we could put everything back in one file at this point, given what I learned in the process, but leaving as is for now

deleteSession seems like it belongs with createSession but the others make sense as utils. Could do as a follow-up though.

doug-s-nava · 2025-01-08T15:27:54Z

frontend/src/services/auth/session.ts

-    console.error("Failed to decrypt session cookie", error);
-    return null;
+// isolate encoding behavior from file execution
+const initializeSessionSecrets = () => {


this implementation probably isn't strictly necessary, but it's a little bit cleaner than instantiating on each module load, and helps deal with situations (like our build) where the keys won't be available

mdragon

Approved

acouch · 2025-01-09T17:35:12Z

frontend/.env.development

@@ -32,4 +32,4 @@ USE_SEARCH_MOCK_DATA=false
 NEW_RELIC_APP_NAME=
 NEW_RELIC_LICENSE_KEY=

-SESSION_SECRET=extraSecretSessionSecretValueSssh
+# SESSION_SECRET=extraSecretSessionSecretValueSssh


It occurs to me we should update the dev docs for the auth work, created a separate ticket #3474

acouch · 2025-01-09T17:38:45Z

frontend/src/services/auth/sessionUtils.ts

+import { cookies } from "next/headers";
+
+export const CLIENT_JWT_ENCRYPTION_ALGORITHM = "HS256";
+export const API_JWT_ENCRYPTION_ALGORITHM = "RS256";


Should these live in src/constants? If not what is our criteria for what goes there vs not?

that's a good question...

in this case I think it's ok to leave them here as they don't take up much space and their usage should be pretty limited. I'm open to moving them, but would maybe want to create a "generalConstants" file to put them in since I don't see the point of having lots of tiny constants files for every individual slice of functionality

acouch · 2025-01-09T18:05:35Z

frontend/src/constants/environments.ts

@@ -41,4 +42,5 @@ export const environment: { [key: string]: string } = {
  NEXT_BUILD: NEXT_BUILD || "false",
  SESSION_SECRET: SESSION_SECRET || "",
  NEXT_PUBLIC_BASE_URL: NEXT_PUBLIC_BASE_URL || "http://localhost:3000",
+  API_JWT_PUBLIC_KEY: API_JWT_PUBLIC_KEY || "",


Thinking out loud, might be useful to error or write debug statements here if certain envars are missing.

one problem there is that these will always be undefined in the build process, so we'd be polluting the logs there unless we check the NEXT_BUILD env var to gate the logging. We're only setting that flag in the Dockerfile at the moment, though. Worth a further discussion

acouch

Some comments but mostly just food for thought. Looks great, lets merge!

* updates to session management to allow for decrypting the login.gov JWT in order to pull out user email * creation of a logged in state for the header

doug-s-nava added 15 commits January 7, 2025 10:13

wip

0204314

got this far trying to use JWK but running into errors re: public vs …

735106c

…private vs secret jwk

working now using KeyObject

b154c3d

reorg, fixups and wip on tests

03921ff

more wip tests

89dc193

wip get rid of the class

f648391

class dismissed, tests working

39642aa

wip header component state

c6b5dc5

fix caching issue / race condition

a921944

update ui on logout

c924df9

styling

0ae107a

more styles

c77d8a2

finish styles

f7aee84

fix linting and ts errors

f0a44b7

fix some more errors, add tf env var for API JWT key

5fd25dd

doug-s-nava force-pushed the dschrashun/3247-logged-in-header branch from 28b2f3e to 5fd25dd Compare January 8, 2025 15:13

doug-s-nava commented Jan 8, 2025

View reviewed changes

cleanup

7b71b55

doug-s-nava marked this pull request as ready for review January 8, 2025 15:52

doug-s-nava requested review from acouch, coilysiren, mdragon, andycochran and btabaska as code owners January 8, 2025 15:52

mdragon approved these changes Jan 9, 2025

View reviewed changes

acouch reviewed Jan 9, 2025

View reviewed changes

acouch approved these changes Jan 9, 2025

View reviewed changes

doug-s-nava merged commit bb662dd into feature/nextjs-auth Jan 9, 2025
14 checks passed

doug-s-nava deleted the dschrashun/3247-logged-in-header branch January 9, 2025 18:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue #3247] logged in header #3458

[Issue #3247] logged in header #3458

doug-s-nava commented Jan 8, 2025 •

edited

Loading

doug-s-nava Jan 8, 2025 •

edited

Loading

doug-s-nava Jan 8, 2025

doug-s-nava Jan 8, 2025

acouch Jan 9, 2025

acouch Jan 9, 2025

doug-s-nava Jan 9, 2025

doug-s-nava Jan 8, 2025

doug-s-nava Jan 8, 2025

doug-s-nava Jan 8, 2025

acouch Jan 9, 2025

doug-s-nava Jan 8, 2025

mdragon left a comment

acouch Jan 9, 2025

acouch Jan 9, 2025

doug-s-nava Jan 9, 2025

acouch Jan 9, 2025

doug-s-nava Jan 9, 2025

acouch left a comment

		@@ -2,12 +2,12 @@ import { getSession } from "src/services/auth/session";

		import { NextResponse } from "next/server";

		export const revalidate = 0;

[Issue #3247] logged in header #3458

[Issue #3247] logged in header #3458

Conversation

doug-s-nava commented Jan 8, 2025 • edited Loading

Summary

Time to review: 45 mins

Changes proposed

Context for reviewers

Notes

Test steps

Additional information

Logged out

Logged in

Logged in drop down

Logged in mobile

Logged in mobile drop down

doug-s-nava Jan 8, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdragon 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

acouch left a comment

Choose a reason for hiding this comment

doug-s-nava commented Jan 8, 2025 •

edited

Loading

doug-s-nava Jan 8, 2025 •

edited

Loading