Frontend Build Out #41

gbdubs · 2023-10-16T16:07:21Z

NOTE: This PR is a poorly structured amalgomation of a bunch of small changes. In the future I'll try to do better to structure changes like these as smaller, more focused PRs - just during this initial build out, so much is changing so quickly, I'm struggling to find the "right" boundaries for increments of work.

Contentious

Starts Populating UserID in context, and in doing so, moves the logic for extracting a PACTA User ID from a Authn User ID and moves it into a server handler in the main.go method. I don't love this so if you have better Ideas on how this can be done, please let me know.
Creates a Fake Users Modal (since MSAL doesn't have a direct authn mechanism that I can find, I think this kind of "shared test account" strategy may be the best we can do.
merges useSession.signedIn and useMSAL.isAuthenticated to fix a few issues with hydration mismtaches/SSR 500s.

Non-Contentious

Creates OpenAPI methods for
- User Editing + Retrieval
- Initiative Invitation Editing + Claiming
- Initiative Membership Listing
Creates rudimentary frontends for initiatives, users, relationships, etc. The goal of these isn't to be a polished final result, but demonstrate basic functionality. I've added i18n everywhere I expect to be stable/permanent, but have not bothered with it elsewhere.

bcspragu

Main takeaways:

Lots of progress, great work!
Need to refactor UserID populating middleware (i.e. dump into a session package)
Seems like we should talk about API design and stuff
No idea why invitations are the way they are
No composable loops or calling outside of setup for general FE stability

(Approved, but maybe let's chat and do one more round of review before this goes in?)

cmd/server/main.go

bcspragu · 2023-10-17T16:48:35Z

cmd/server/main.go

+
+	return func(next http.Handler) http.Handler {
+		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			ctx, err := fn(r.Context())


nit: I think this will go away with the above comment, but just in case it doesn't: normally, one only returns a new context from functions that are dedicated to contexts, e.g. WithX or context.WithX functions. So I'd change it to something like:

fn := func(c context.Context) (pacta.UserID, error) { ... } [...] return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { uID, err := fn(r.Context()) ctx := session.WithUserID(ctx, uID) next.ServeHTTP(w, r.WithContext(ctx)) } }

Yep - Done.

bcspragu · 2023-10-17T16:48:36Z

cmd/server/pactasrv/initiative_invitation.go

+}
+
+// Claims this initiative invitation, if it exists
+// (POST /initiative-invitation/{id})


nit: I think of "claiming an invitation" as a non-CRUD action that should get a custom verb, like POST /initiative-invitation/{id}:claim. Benefits of this:

Unambigious about what it's for, makes reading logs and stuff easier

If you have another "updating an initiative invitation" endpoint you need to add, you won't have a collision on POST /initiative-invitation/{id}

Some Google Cloud guide that discusses this at length

bcspragu · 2023-10-17T16:48:36Z

cmd/server/pactasrv/initiative_invitation.go

+			return fmt.Errorf("looking up initiative invite: %w", err)
+		}
+		if !ii.UsedAt.IsZero() || ii.UsedBy != nil {
+			if ii.UsedBy != nil && ii.UsedBy.ID == userID {


My usual grumbling about new paradigms I don't like: the whole "We use objects for everything and only populate the IDs" makes all of this logic one conditional comparision more than it otherwise would need to be. E.g. as compared to

if ii.UsedByID == userID { ... }

Yeah I think in most cases you'll come to like this pattern. I've found it increasingly lovely for side projects.

bcspragu · 2023-10-17T16:48:36Z

cmd/server/pactasrv/initiative_invitation.go

+				// We may want to log this, though.
+				return nil
+			} else {
+				return fmt.Errorf("initiative is already used: %+v", ii)


We should be using oapierr.BadRequest (or similar) for this. For this case in particular, we likely want a custom error code so we can show a specific error to the end user. Then you'd just need to update the error logic below to:

if errors.Is(err, oapierr.Error{}) { // or something like that, could also make oapierr.Is(err) or similar return err } else if err != nil { return oapierr.Internal(...) }

You could even make a lil helper, like return oapierr.OrInternal(err)

Done, but through different means, as an external var.

bcspragu · 2023-10-17T16:48:39Z

frontend/pages/user/[id].vue

+const [
+  { data: user },
+] = await Promise.all([
+  useSimpleAsyncData(`${prefix}.findUserById`, () => pactaClient.findUserById(id)),
+])


Same note here, don't need Promise.all if you only got one promise.

Next PR adds more :)

frontend/pages/user/[id]/index.vue

bcspragu · 2023-10-17T16:48:39Z

frontend/plugins/msal.client.ts

-              console.info(message)
+              // console.info(message)
              return
            case LogLevel.Verbose:
-              console.debug(message)
+              // console.debug(message)
              return
            case LogLevel.Warning:
              console.warn(message)
              return
            case LogLevel.Trace:
-              console.trace(message)
+              // console.trace(message)
              return


I've found this logging makes the console semi-unusable - sorting through hundreds of extraneous logs in order to find anything other than authn information. I think our default should be to a clean log. Keeping warning and error messages makes a ton of sense, but the default happy path of console logging should not create noise.

openapi/pacta.yaml

bcspragu

Boosted a few previous comments that should be addressed/we should discuss, but otherwise LGTM

bcspragu · 2023-11-15T15:45:50Z

session/session.go

+	fn := func(c context.Context) (context.Context, error) {
+		token, _, err := jwtauth.FromContext(c)
+		if err != nil {
+			return nil, fmt.Errorf("error getting authorization token: %w", err)
+		}
+		if token == nil {
+			return nil, fmt.Errorf("nil authorization token")
+		}
+		authnID := token.Subject()
+		if authnID == "" {
+			return nil, fmt.Errorf("couldn't find authn id in jwt")
+		}
+		user, err := d.UserByAuthn(d.NoTxn(c), pacta.AuthnMechanism_EmailAndPass, authnID)
+		if err != nil {
+			return nil, fmt.Errorf("failed to get user by authn: %w", err)
+		}
+		return WithUserID(c, user.ID), nil
+	}
+
+	return func(next http.Handler) http.Handler {
+		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			ctx, err := fn(r.Context())
+			if err != nil {
+				// Optionally log errors here when debugging authentication access.
+				// logger.Warn("couldn't authenticate", zap.Error(err))
+				// http.Error(w, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized)
+				next.ServeHTTP(w, r)
+				return
+			}
+			next.ServeHTTP(w, r.WithContext(ctx))
+		})
+	}


A few random tweaks, just to abstract some concerns better. E.g. it'd be easier to pull loadUserID into a function and test it independently now:

Suggested change

fn := func(c context.Context) (context.Context, error) {

token, _, err := jwtauth.FromContext(c)

if err != nil {

return nil, fmt.Errorf("error getting authorization token: %w", err)

}

if token == nil {

return nil, fmt.Errorf("nil authorization token")

}

authnID := token.Subject()

if authnID == "" {

return nil, fmt.Errorf("couldn't find authn id in jwt")

}

user, err := d.UserByAuthn(d.NoTxn(c), pacta.AuthnMechanism_EmailAndPass, authnID)

if err != nil {

return nil, fmt.Errorf("failed to get user by authn: %w", err)

}

return WithUserID(c, user.ID), nil

}

return func(next http.Handler) http.Handler {

return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

ctx, err := fn(r.Context())

if err != nil {

// Optionally log errors here when debugging authentication access.

// logger.Warn("couldn't authenticate", zap.Error(err))

// http.Error(w, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized)

next.ServeHTTP(w, r)

return

}

next.ServeHTTP(w, r.WithContext(ctx))

})

}

loadUserID := func(ctx context.Context) (pacta.UserID, error) {

token, _, err := jwtauth.FromContext(ctx)

if err != nil {

return "", fmt.Errorf("error getting jwt from context: %w", err)

}

if token == nil {

return "", errors.New("nil authorization token")

}

authnID := token.Subject()

if authnID == "" {

return "", errors.New("no 'sub' claim in JWT")

}

user, err := d.UserByAuthn(d.NoTxn(ctx), pacta.AuthnMechanism_EmailAndPass, authnID)

if err != nil {

return "", fmt.Errorf("failed to load user %q by authn: %w", authnID, err)

}

return user.ID, nil

}

return func(next http.Handler) http.Handler {

return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

ctx := r.Context()

userID, err := fn(ctx)

if err != nil {

// Optionally log errors here when debugging authentication access.

// logger.Warn("couldn't authenticate", zap.Error(err))

// http.Error(w, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized)

next.ServeHTTP(w, r)

return

}

next.ServeHTTP(w, r.WithContext(WithUserID(ctx, userID)))

})

}

bcspragu · 2023-11-15T15:49:19Z

session/session.go

+				// Optionally log errors here when debugging authentication access.
+				// logger.Warn("couldn't authenticate", zap.Error(err))


Since we don't have any endpoints that are public, and since this runs after the JWT verification flow, if this fails, we want to fail the request here, as this will be a programmer bug 10 times out of 10. Otherwise, it's just going to go onto the handler, which will then fail because it couldn't load the UserID. At least here, we catch it at the source. I'd log at ERROR-level and throw a http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)

FYI We will have non-public endpoints: initiatives are going to be non-public. Does that change your reasoning?

Discussed offline, there will be public endpoints. That said, our current middleware has no way to poke holes for public endpoints, so anything non-auth'd will fail at the moment

bcspragu · 2023-11-15T15:51:33Z

cmd/server/main.go

 			chimiddleware.Recoverer,

 			jwtauth.Verifier(jwtauth.New("EdDSA", nil, jwKey)),
 			jwtauth.Authenticator,
+			session.WithAuthn(logger, db),


nothing needs to change here, but because auth in RMI-land is so different than AS-land, we really don't need a whole package and middleware for this, it could just be a server helper like s.getUser or s.getUserID. It's no more or less code in the endpoints themselves, just the difference between session.UserID(ctx) and s.getUserID(ctx).

bcspragu · 2023-11-15T15:52:55Z

cmd/server/pactasrv/conv/oapi_to_pacta.go

+	if i == nil {
+		return nil, oapierr.Internal("initiativeInvitationToOAPI: can't convert nil pointer")
+	}
+	if !initiativeIDRegex.MatchString(i.Id) {


I can't remember if I asked this before, but why are we validating IDs, aren't we the ones generating them? And if we aren't the ones generating them, let's chat.

bcspragu · 2023-11-15T15:54:23Z

cmd/server/pactasrv/conv/pacta_to_oapi.go

+	if i.UsedBy != nil {
+		usedBy = ptr(string(i.UsedBy.ID))
+	}


If you expect to be doing this a lot, you could make a generic nullablePtr or similar helper, then inline this in the return

frontend/composables/useTime.ts

bcspragu · 2023-11-15T17:39:41Z

frontend/pages/initiative/[id]/index.vue

+  useSimpleAsyncData(`${prefix}.getInitiative`, () => pactaClient.findInitiativeById(id)),
+  useSimpleAsyncData(`${prefix}.getInvitations`, () => pactaClient.listInitiativeInvitations(id)),
+  useSimpleAsyncData(`${prefix}.getRelationships`, () => pactaClient.listInitiativeUserRelationshipsByInitiative(id)),


bcspragu · 2023-11-15T17:39:54Z

frontend/pages/initiative/[id]/internal.vue

+const [
+  { data: initiative },
+] = await Promise.all([
+  useSimpleAsyncData(`${prefix}.getInitiative`, () => pactaClient.findInitiativeById(id)),
+  useSimpleAsyncData(`${prefix}.getInvitations`, () => pactaClient.listInitiativeInvitations(id)),
+  useSimpleAsyncData(`${prefix}.getRelationships`, () => pactaClient.listInitiativeUserRelationshipsByInitiative(id)),
+])


bcspragu · 2023-11-15T17:40:36Z

frontend/pages/initiative/[id]/relationships.vue

+const [
+  { data: relationships },
+] = await Promise.all([
+  useSimpleAsyncData(`${prefix}.getRelationships`, () => pactaClient.listInitiativeUserRelationshipsByInitiative(id)),
+])


(this was resolved, but it's still a Promise.all on a single promise)

bcspragu · 2023-11-15T17:40:57Z

frontend/plugins/msal.client.ts

-              console.info(message)
+              // console.info(message)
              return
            case LogLevel.Verbose:
-              console.debug(message)
+              // console.debug(message)
              return
            case LogLevel.Warning:
              console.warn(message)
              return
            case LogLevel.Trace:
-              console.trace(message)
+              // console.trace(message)
              return


bcspragu · 2023-11-15T17:42:34Z

frontend/composables/useIsAuthenticated.ts

@@ -0,0 +1,5 @@
+export const useIsAuthenticated = () => {
+  const prefix = 'useIsAuthenticated'
+  const isAuthenticated = useState<boolean>(`${prefix}.isAuthenticated`, () => true)


Sorry, straggling comment: why does this default to true?

gbdubs · 2023-11-16T00:38:26Z

Discussed offline - hitting the timebox on this. Smaller, better PRs going forward.

gbdubs added 4 commits October 12, 2023 14:15

WIP

0b80a20

Merge branch 'main' into grady/invitations

bcfa77d

Oh No Too Much

44021ba

Self Review

c15a2f9

gbdubs requested a review from bcspragu October 16, 2023 16:08

bcspragu approved these changes Oct 17, 2023

View reviewed changes

gbdubs added 3 commits November 13, 2023 15:33

Merge branch 'main' into grady/invitations

5ba55e0

WIP

7e3091b

Addresses Review Comments

9b5c911

gbdubs requested a review from bcspragu November 15, 2023 15:18

bcspragu approved these changes Nov 15, 2023

View reviewed changes

gbdubs added 2 commits November 15, 2023 17:26

Merge branch 'main' into grady/invitations

63a29d5

Fixes Lint

42c03aa

gbdubs enabled auto-merge (squash) November 16, 2023 00:38

gbdubs merged commit 09a2315 into main Nov 16, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Frontend Build Out #41

Frontend Build Out #41

gbdubs commented Oct 16, 2023 •

edited

Loading

bcspragu left a comment

bcspragu Oct 17, 2023

gbdubs Nov 15, 2023

bcspragu Oct 17, 2023

gbdubs Nov 15, 2023

bcspragu Oct 17, 2023

gbdubs Nov 15, 2023

bcspragu Oct 17, 2023

gbdubs Nov 15, 2023

bcspragu Oct 17, 2023

gbdubs Nov 14, 2023

bcspragu Oct 17, 2023

gbdubs Nov 14, 2023

bcspragu Nov 15, 2023

bcspragu left a comment

bcspragu Nov 15, 2023

bcspragu Nov 15, 2023

gbdubs Nov 15, 2023

bcspragu Nov 15, 2023

bcspragu Nov 15, 2023

bcspragu Nov 15, 2023

bcspragu Nov 15, 2023

bcspragu Nov 15, 2023

bcspragu Nov 15, 2023

bcspragu Nov 15, 2023

bcspragu Nov 15, 2023

bcspragu Nov 15, 2023

gbdubs commented Nov 16, 2023

		// Optionally log errors here when debugging authentication access.
		// logger.Warn("couldn't authenticate", zap.Error(err))

Frontend Build Out #41

Frontend Build Out #41

Conversation

gbdubs commented Oct 16, 2023 • edited Loading

bcspragu 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

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

bcspragu 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

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

gbdubs commented Nov 16, 2023

gbdubs commented Oct 16, 2023 •

edited

Loading