[Networking] - useAPIClients Init Custom Hook + Auth changes to enable mutations/subscriptions #1068

drewjhart · 2024-05-01T15:13:46Z

This completes the work on the Auth layer. For a full description of this layer, as well as an overview of the new API client and a step-by-step review of the setup process, see the below doc in the Wiki:

https://github.com/rightoneducation/righton-app/wiki/Backend-%E2%80%90-Authentication-and-Authorization

The PRs break down to:
-> [Backend] - Google OAuth Integration #988
->-> [Central_v2] - Container for authorization #1013
->->-> [Authentication/Authorization] - Networking updates #1058
->->->-> [Networking] - Upgrade Amplify v5 to v6 #1059
->->->->-> [Networking] - AuthAPIClient #1060
->->->->->-> [Networking] - useAPIClients Init Custom Hook + Auth changes to enable mutations/subscriptions

Review Cheat Sheet
Hey @maniramezan , this set of PR's is a bit confusing for two reasons:

A lot of the file changes you are seeing will be auto-generated from Amplify (resolvers and config files etc)
A lot of the previous PRs have initial implementations of Auth methods that are refined in later PRs

Therefore:

I'd focus primarily on this PR as the main PR to review. This represents the final AuthAPIClient and the initialization and deployment of it throughout the code (using the custom hook). Implementations in previous PRs are overwritten in this one.
I've added a bunch of comments throughout this PR to explain how this works and the set up of it.
If you want insight into the actual auth setup, I've created a Wiki document here: https://github.com/rightoneducation/righton-app/wiki/Backend-%E2%80%90-Authentication-and-Authorization

Hopefully, this makes your job a bit easier when reviewing this stuff. Definitely let me know if anything is confusing though!

drewjhart · 2024-05-09T13:50:54Z

central/src/App.tsx

@@ -27,18 +27,20 @@ const theme = createTheme({

 function App() {
  const [alert, setAlert] = useState<Alert | null>(null);
-  const apiClients = new APIClients(Environment.Developing);
  const alertContext = {
    alert,
    setAlert,
  };



useApiClient custom hook for initialization of AuthAPIClient first, and then other APIClients

drewjhart · 2024-05-09T13:51:32Z

central_v2/src/pages/Login.tsx

@@ -4,7 +4,7 @@ import { styled } from "@mui/system";
 import { Link } from "react-router-dom";


central_v2 will be reworked in a subsequent set of PRs in Q3 (Design is still working on it)

drewjhart · 2024-05-09T13:52:16Z

networking/amplify/backend/api/mobile/custom-roles.json

@@ -0,0 +1,5 @@
+{


this role is required to ensure that the createGame Lambda function is explicitly provided with its Execution Role. See the Wiki article for a link to the AWS docs

drewjhart · 2024-05-09T13:53:29Z

networking/amplify/backend/api/mobile/resolvers/Mutation.createTeam.auth.1.req.vtl

@@ -7,12 +7,12 @@ $util.qr($ctx.stash.put("hasAuth", true))
 $util.unauthorized()
 #end


I think there is a bug with Amplify that requires us to manually set these resolvers. Pretty annoying so I'll talk to our AWS contact next time. See Wiki for a link to an Amplify issue that confirms this and recommends the below edits to resolve

aww yeah this sounds like a bug as these are auto-gen and we'd need to keep updating them after each code changes

drewjhart · 2024-05-09T13:53:59Z

networking/amplify/backend/api/mobile/schema.graphql

@@ -1,10 +1,18 @@
 type Mutation {
-  createGameSessionFromTemplate(input: CreateGameSessionFromTemplateInput!): String @function(name: "createGame-${env}")
+  createGameSessionFromTemplate(input: CreateGameSessionFromTemplateInput!): String 


@auth directive required for custom mutations and subscriptions as well as the @types

drewjhart · 2024-05-09T13:55:43Z

networking/src/APIClients/BaseAPIClient.ts

@@ -44,11 +45,13 @@ export interface IQueryParameters {



pass AuthAPIClient to other APIClients to provide auth to the API requests

drewjhart · 2024-05-09T13:56:27Z

networking/src/Hooks/useAPIClients.ts

@@ -0,0 +1,25 @@
+import { useState, useEffect } from 'react';


custom Hook to provide proper sequence for APIClient init. First init AuthAPIClients and then pass those clients down into the other APIClients

maniramezan

Great job on this, this looks great, and I love how you have organized the files and methods ❤️

maniramezan · 2024-05-15T15:23:10Z

networking/amplify/backend/api/mobile/resolvers/Mutation.createTeam.auth.1.req.vtl

@@ -7,12 +7,12 @@ $util.qr($ctx.stash.put("hasAuth", true))
 $util.unauthorized()
 #end


aww yeah this sounds like a bug as these are auto-gen and we'd need to keep updating them after each code changes

maniramezan · 2024-05-15T15:26:15Z

networking/amplify/backend/function/createGame/src/index.mjs


 async function createAndSignRequest(query, variables) {
+  const credentials = await defaultProvider()();


Two API calls here? If so, makes sense to break it into two lines IMO. But if it's a common pattern in JS, then lets leave it

maniramezan · 2024-05-15T15:30:02Z

networking/src/APIClients/APIClients.ts

+  private constructor(env: Environment, authClient: IAuthAPIClient) {
+    this.auth = authClient;
+    this.gameTemplate = new GameTemplateAPIClient(env, this.auth);
+    this.questionTemplate = new QuestionTemplateAPIClient(env, this.auth);
+    this.gameQuestions = new GameQuestionsAPIClient(env, this.auth);
+    this.gameSession = new GameSessionAPIClient(env, this.auth);
+    this.question = new QuestionAPIClient(env, this.auth);
+    this.team = new TeamAPIClient(env, this.auth);
+    this.teamMember = new TeamMemberAPIClient(env, this.auth);
+    this.teamAnswer = new TeamAnswerAPIClient(env, this.auth);
+  }


If this list grows or becomes hard to maintain, we can probably write a script to auto-gen this and this whole file actually. Following the name XXXAPIClient for any API client would help with this as the script could just find all XXXAPIClient files and then generate this file and include one line for each. So lets make sure we follow the same naming pattern for any new api client we might add in future

maniramezan · 2024-05-15T15:33:11Z

networking/src/APIClients/APIClients.ts

+  static async create(env: Environment): Promise<APIClients> {
+    const authClient = new AuthAPIClient();
+    await authClient.init(); // Ensure the auth client is initialized
+    return new APIClients(env, authClient); 


hmm, wondering if we should do lazy loading / singleton here instead of create method as create can be called multiple times to create new instances which I doubt it's what we want. This is very minor feedback so we can figure it out later as some also suggest against singleton pattern when possible

maniramezan · 2024-05-15T15:36:34Z

networking/src/APIClients/BaseAPIClient.ts

@@ -57,7 +60,21 @@ export abstract class BaseAPIClient {
    query: any,
    options?: GraphQLOptions
  ): Promise<GraphQLResult<T>> {
-    const response = client.graphql({query: query, variables: options, authMode: "userPool" as GraphQLAuthMode}) as unknown;
+    const authMode = this.auth.isUserAuth ? "userPool" : "iam";


Always recommended to use an enum with associated string values, especially in cases like this that same strings are referenced in multiple locations. Other pattern is just creating a static const variable and then use that. This reduces typo issues and also if you need to add any new cases or something, you have one place to add and then compiler would throw an error

Another comment here is that maybe we can create a method that returns authMode, something like:

const getAuthMode => { this.auth.isUserAuth ? "userPool" : "iam"; }

I know it's juts a one liner abstraction, but I think it makes the code more readable and moves the focus to what the actual method is doing rather than thinking why authMode is userPool vs. iam etc

maniramezan · 2024-05-15T15:46:37Z

networking/src/APIClients/auth/AuthAPIClient.ts

  configAmplify(awsconfig: any): void {
    Amplify.configure(awsconfig);
+    // change userPools auth storage to cookies so that auth persists across central/host apps for signed-in teachers
+    cognitoUserPoolsTokenProvider.setKeyValueStorage(new CookieStorage());


hmm, I need to read on it, but looks like there're some other storages as well like browser storage or session storage. I don't think we've tested our product in cookie-less environment, but worth to think about what would happen if cookie is disabled in browser. I remember back in the day, some were disabling it for security reasons or tracking purposes.

maniramezan · 2024-05-15T15:53:52Z

networking/src/APIClients/APIClients.ts

+    const authClient = new AuthAPIClient();
+    await authClient.init(); // Ensure the auth client is initialized


Why not combining these together and create a method like AuthAPIClient.create? That way, we can mark the constructor as private so it indicates that `AuthClient can't be instantiated through constructor

maniramezan · 2024-05-15T15:59:15Z

networking/src/APIClients/gamesession/interfaces/ITeamMemberAPIClient.ts

@@ -1,3 +1,4 @@
+import { Subscription } from 'rxjs'; // this is used in aws-amplify to handle subscriptions


maniramezan · 2024-05-15T16:00:02Z

networking/src/APIClients/gamesession/TeamMemberAPIClient.ts

-  ): Promise<any> {
+  ) {


Intentionally removing the return type?

maniramezan · 2024-05-15T16:01:11Z

networking/src/APIClients/auth/interfaces/IAuthAPIClient.ts

@@ -1,6 +1,7 @@

 export interface IAuthAPIClient {
  isUserAuth: boolean;
+  init(): Promise<void>;


See the comment above regarding combining this with constructor into one method

updates apiclients and authclinet

b1d5ec4

drewjhart changed the title ~~Dev release 05/01/2024~~ WIP May 1, 2024

drewjhart changed the base branch from main to drew-hart--auth-federatedsignin May 1, 2024 15:17

drewjhart added 4 commits May 3, 2024 12:49

auth troubleshooting

54afd83

fixes updateGameSession mutation

67499b7

update app apiclients for play

99d9b2f

fixes auth on mutations and subscriptions

92697dc

drewjhart changed the title ~~WIP~~ [Backend] - Auth changes to enable mutations/subscriptions May 8, 2024

drewjhart added 2 commits May 8, 2024 15:18

clean up

11b1e97

formatting

8ee7252

drewjhart changed the title ~~[Backend] - Auth changes to enable mutations/subscriptions~~ [Networking] - Auth changes to enable mutations/subscriptions May 8, 2024

drewjhart changed the title ~~[Networking] - Auth changes to enable mutations/subscriptions~~ [Networking] - useAuthAPI Custom Hook + Auth changes to enable mutations/subscriptions May 9, 2024

drewjhart changed the title ~~[Networking] - useAuthAPI Custom Hook + Auth changes to enable mutations/subscriptions~~ [Networking] - useAPIClients Init Custom Hook + Auth changes to enable mutations/subscriptions May 9, 2024

drewjhart added 2 commits May 9, 2024 09:15

formatting

a67b15c

corrections from merge

7947db7

drewjhart commented May 9, 2024

View reviewed changes

drewjhart requested a review from maniramezan May 9, 2024 14:10

drewjhart mentioned this pull request May 9, 2024

[Networking] - AuthAPIClient #1060

Open

maniramezan approved these changes May 15, 2024

View reviewed changes

fixes

f96cf9e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Networking] - useAPIClients Init Custom Hook + Auth changes to enable mutations/subscriptions #1068

[Networking] - useAPIClients Init Custom Hook + Auth changes to enable mutations/subscriptions #1068

drewjhart commented May 1, 2024 •

edited

Loading

drewjhart May 9, 2024

drewjhart May 9, 2024

drewjhart May 9, 2024 •

edited

Loading

drewjhart May 9, 2024

maniramezan May 15, 2024

drewjhart May 9, 2024 •

edited

Loading

drewjhart May 9, 2024

drewjhart May 9, 2024

maniramezan left a comment

maniramezan May 15, 2024

maniramezan May 15, 2024

maniramezan May 15, 2024

maniramezan May 15, 2024

maniramezan May 15, 2024

maniramezan May 15, 2024

maniramezan May 15, 2024

maniramezan May 15, 2024

maniramezan May 15, 2024

maniramezan May 15, 2024

maniramezan May 15, 2024

		@@ -4,7 +4,7 @@ import { styled } from "@mui/system";
		import { Link } from "react-router-dom";

		@@ -7,12 +7,12 @@ $util.qr($ctx.stash.put("hasAuth", true))
		$util.unauthorized()
		#end

		@@ -0,0 +1,25 @@
		import { useState, useEffect } from 'react';


		async function createAndSignRequest(query, variables) {
		const credentials = await defaultProvider()();

		const authClient = new AuthAPIClient();
		await authClient.init(); // Ensure the auth client is initialized

		@@ -1,3 +1,4 @@
		import { Subscription } from 'rxjs'; // this is used in aws-amplify to handle subscriptions

[Networking] - useAPIClients Init Custom Hook + Auth changes to enable mutations/subscriptions #1068

Are you sure you want to change the base?

[Networking] - useAPIClients Init Custom Hook + Auth changes to enable mutations/subscriptions #1068

Conversation

drewjhart commented May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewjhart May 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewjhart May 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maniramezan 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

drewjhart commented May 1, 2024 •

edited

Loading

drewjhart May 9, 2024 •

edited

Loading

drewjhart May 9, 2024 •

edited

Loading