Skip to content

refactor: Refactor auth provider #5782

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
Jan 20, 2023
Merged

refactor: Refactor auth provider #5782

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
244845f
Add paywall to the audit route
BrunoQuaresma Jan 18, 2023
b83f9c2
refactor auth provider
BrunoQuaresma Jan 18, 2023
528892f
Move router to be inside of AppRouter
BrunoQuaresma Jan 18, 2023
9f0e0ee
Create dashboard provider
BrunoQuaresma Jan 18, 2023
1714ca5
Minor refactoring
BrunoQuaresma Jan 18, 2023
884dac7
Fix tests
BrunoQuaresma Jan 18, 2023
970e1a5
Merge branch 'bq/refactor-audit-route' into bq/refactor-auth-provider-2
BrunoQuaresma Jan 18, 2023
6e572e3
Merge branch 'main' of github.com:coder/coder into bq/refactor-auth-p…
BrunoQuaresma Jan 19, 2023
6442b3a
Fix Audit Log page test
BrunoQuaresma Jan 19, 2023
5747590
Add long time running test
BrunoQuaresma Jan 19, 2023
7768e95
Fix stories
BrunoQuaresma Jan 19, 2023
1173d16
Increase timeout
BrunoQuaresma Jan 19, 2023
ea4300b
Increse timeout
BrunoQuaresma Jan 19, 2023
d8826c3
Update site/src/components/WorkspaceStats/WorkspaceStats.stories.tsx
BrunoQuaresma Jan 19, 2023
e12f437
Update site/src/components/AuthProvider/AuthProvider.tsx
BrunoQuaresma Jan 19, 2023
9e7a875
Fix format
BrunoQuaresma Jan 19, 2023
3e589f1
Improve name
BrunoQuaresma Jan 19, 2023
dce50e3
Extract audit log paywall
BrunoQuaresma Jan 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
201 changes: 90 additions & 111 deletions site/src/AppRouter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
import { useSelector } from "@xstate/react"
import { FullScreenLoader } from "components/Loader/FullScreenLoader"
import { RequirePermission } from "components/RequirePermission/RequirePermission"
import { TemplateLayout } from "components/TemplateLayout/TemplateLayout"
import { UsersLayout } from "components/UsersLayout/UsersLayout"
import IndexPage from "pages"
Expand All @@ -12,12 +10,9 @@ import { TemplateSettingsPage } from "pages/TemplateSettingsPage/TemplateSetting
import TemplatesPage from "pages/TemplatesPage/TemplatesPage"
import UsersPage from "pages/UsersPage/UsersPage"
import WorkspacesPage from "pages/WorkspacesPage/WorkspacesPage"
import { FC, lazy, Suspense, useContext } from "react"
import { Route, Routes } from "react-router-dom"
import { selectPermissions } from "xServices/auth/authSelectors"
import { selectFeatureVisibility } from "xServices/entitlements/entitlementsSelectors"
import { XServiceContext } from "xServices/StateContext"
import { DashboardLayout } from "./components/DashboardLayout/DashboardLayout"
import { FC, lazy, Suspense } from "react"
import { Route, Routes, BrowserRouter as Router } from "react-router-dom"
import { DashboardLayout } from "./components/Dashboard/DashboardLayout"
import { RequireAuth } from "./components/RequireAuth/RequireAuth"
import { SettingsLayout } from "./components/SettingsLayout/SettingsLayout"
import { DeploySettingsLayout } from "components/DeploySettingsLayout/DeploySettingsLayout"
Expand Down Expand Up @@ -123,135 +118,119 @@ const CreateTemplatePage = lazy(
)

export const AppRouter: FC = () => {
const xServices = useContext(XServiceContext)
const permissions = useSelector(xServices.authXService, selectPermissions)
const featureVisibility = useSelector(
xServices.entitlementsXService,
selectFeatureVisibility,
)

return (
<Suspense fallback={<FullScreenLoader />}>
<Routes>
<Route path="login" element={<LoginPage />} />
<Route path="setup" element={<SetupPage />} />
<Router>
<Routes>
<Route path="login" element={<LoginPage />} />
<Route path="setup" element={<SetupPage />} />

{/* Dashboard routes */}
<Route element={<RequireAuth />}>
<Route element={<DashboardLayout />}>
<Route index element={<IndexPage />} />
{/* Dashboard routes */}
<Route element={<RequireAuth />}>
<Route element={<DashboardLayout />}>
<Route index element={<IndexPage />} />

<Route path="gitauth" element={<GitAuthPage />} />
<Route path="gitauth" element={<GitAuthPage />} />

<Route path="workspaces" element={<WorkspacesPage />} />
<Route path="workspaces" element={<WorkspacesPage />} />

<Route path="starter-templates">
<Route index element={<StarterTemplatesPage />} />
<Route path=":exampleId" element={<StarterTemplatePage />} />
</Route>
<Route path="starter-templates">
<Route index element={<StarterTemplatesPage />} />
<Route path=":exampleId" element={<StarterTemplatePage />} />
</Route>

<Route path="templates">
<Route index element={<TemplatesPage />} />
<Route path="new" element={<CreateTemplatePage />} />
<Route path=":template">
<Route element={<TemplateLayout />}>
<Route index element={<TemplateSummaryPage />} />
<Route
path="permissions"
element={<TemplatePermissionsPage />}
/>
<Route path="templates">
<Route index element={<TemplatesPage />} />
<Route path="new" element={<CreateTemplatePage />} />
<Route path=":template">
<Route element={<TemplateLayout />}>
<Route index element={<TemplateSummaryPage />} />
<Route
path="permissions"
element={<TemplatePermissionsPage />}
/>
</Route>

<Route path="workspace" element={<CreateWorkspacePage />} />
<Route path="settings" element={<TemplateSettingsPage />} />
<Route path="versions">
<Route path=":version" element={<TemplateVersionPage />} />
</Route>
</Route>
</Route>

<Route path="workspace" element={<CreateWorkspacePage />} />
<Route path="settings" element={<TemplateSettingsPage />} />
<Route path="versions">
<Route path=":version" element={<TemplateVersionPage />} />
<Route path="users">
<Route element={<UsersLayout />}>
<Route index element={<UsersPage />} />
</Route>
</Route>
</Route>

<Route path="users">
<Route element={<UsersLayout />}>
<Route index element={<UsersPage />} />
<Route path="create" element={<CreateUserPage />} />
</Route>

<Route path="create" element={<CreateUserPage />} />
</Route>
<Route path="/groups">
<Route element={<UsersLayout />}>
<Route index element={<GroupsPage />} />
</Route>

<Route path="/groups">
<Route element={<UsersLayout />}>
<Route index element={<GroupsPage />} />
<Route path="create" element={<CreateGroupPage />} />
<Route path=":groupId" element={<GroupPage />} />
<Route
path=":groupId/settings"
element={<SettingsGroupPage />}
/>
</Route>

<Route path="create" element={<CreateGroupPage />} />
<Route path=":groupId" element={<GroupPage />} />
<Route path=":groupId/settings" element={<SettingsGroupPage />} />
</Route>
<Route path="/audit" element={<AuditPage />} />

<Route path="/audit">
<Route
index
element={
<RequirePermission
isFeatureVisible={
featureVisibility["audit_log"] &&
Boolean(permissions?.viewAuditLog)
}
>
<AuditPage />
</RequirePermission>
}
/>
</Route>

<Route
path="/settings/deployment"
element={<DeploySettingsLayout />}
>
<Route path="general" element={<GeneralSettingsPage />} />
<Route path="security" element={<SecuritySettingsPage />} />
<Route path="appearance" element={<AppearanceSettingsPage />} />
<Route path="network" element={<NetworkSettingsPage />} />
<Route path="userauth" element={<UserAuthSettingsPage />} />
<Route path="gitauth" element={<GitAuthSettingsPage />} />
</Route>
path="/settings/deployment"
element={<DeploySettingsLayout />}
>
<Route path="general" element={<GeneralSettingsPage />} />
<Route path="security" element={<SecuritySettingsPage />} />
<Route path="appearance" element={<AppearanceSettingsPage />} />
<Route path="network" element={<NetworkSettingsPage />} />
<Route path="userauth" element={<UserAuthSettingsPage />} />
<Route path="gitauth" element={<GitAuthSettingsPage />} />
</Route>

<Route path="settings" element={<SettingsLayout />}>
<Route path="account" element={<AccountPage />} />
<Route path="security" element={<SecurityPage />} />
<Route path="ssh-keys" element={<SSHKeysPage />} />
<Route path="tokens" element={<TokensPage />} />
</Route>
<Route path="settings" element={<SettingsLayout />}>
<Route path="account" element={<AccountPage />} />
<Route path="security" element={<SecurityPage />} />
<Route path="ssh-keys" element={<SSHKeysPage />} />
<Route path="tokens" element={<TokensPage />} />
</Route>

<Route path="/@:username">
<Route path=":workspace">
<Route index element={<WorkspacePage />} />
<Route path="schedule" element={<WorkspaceSchedulePage />} />
<Route
path="builds/:buildNumber"
element={<WorkspaceBuildPage />}
/>
<Route
path="change-version"
element={<WorkspaceChangeVersionPage />}
/>
<Route path="/@:username">
<Route path=":workspace">
<Route index element={<WorkspacePage />} />
<Route path="schedule" element={<WorkspaceSchedulePage />} />
<Route
path="builds/:buildNumber"
element={<WorkspaceBuildPage />}
/>
<Route
path="change-version"
element={<WorkspaceChangeVersionPage />}
/>
</Route>
</Route>
</Route>
</Route>

{/* Terminal and CLI auth pages don't have the dashboard layout */}
<Route
path="/@:username/:workspace/terminal"
element={<TerminalPage />}
/>
<Route path="cli-auth" element={<CliAuthenticationPage />} />
</Route>
{/* Terminal and CLI auth pages don't have the dashboard layout */}
<Route
path="/@:username/:workspace/terminal"
element={<TerminalPage />}
/>
<Route path="cli-auth" element={<CliAuthenticationPage />} />
</Route>

{/* Using path="*"" means "match anything", so this route
{/* Using path="*"" means "match anything", so this route
acts like a catch-all for URLs that we don't have explicit
routes for. */}
<Route path="*" element={<NotFoundPage />} />
</Routes>
<Route path="*" element={<NotFoundPage />} />
</Routes>
</Router>
</Suspense>
)
}
23 changes: 2 additions & 21 deletions site/src/api/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,18 +34,6 @@ export const withDefaultFeatures = (
return fs as TypesGen.Entitlements["features"]
}

// defaultEntitlements has a default set of disabled functionality.
export const defaultEntitlements = (): TypesGen.Entitlements => {
return {
features: withDefaultFeatures({}),
has_license: false,
errors: [],
warnings: [],
experimental: false,
trial: false,
}
}

// Always attach CSRF token to all requests.
// In puppeteer the document is undefined. In those cases, just
// do nothing.
Expand Down Expand Up @@ -625,15 +613,8 @@ export const putWorkspaceExtension = async (
}

export const getEntitlements = async (): Promise<TypesGen.Entitlements> => {
try {
const response = await axios.get("/api/v2/entitlements")
return response.data
} catch (error) {
if (axios.isAxiosError(error) && error.response?.status === 404) {
return defaultEntitlements()
}
throw error
}
const response = await axios.get("/api/v2/entitlements")
return response.data
}

export const getExperiments = async (): Promise<TypesGen.Experiment[]> => {
Expand Down
27 changes: 12 additions & 15 deletions site/src/app.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,26 @@
import CssBaseline from "@material-ui/core/CssBaseline"
import ThemeProvider from "@material-ui/styles/ThemeProvider"
import { AuthProvider } from "components/AuthProvider/AuthProvider"
import { FC } from "react"
import { HelmetProvider } from "react-helmet-async"
import { BrowserRouter as Router } from "react-router-dom"
import { AppRouter } from "./AppRouter"
import { ErrorBoundary } from "./components/ErrorBoundary/ErrorBoundary"
import { GlobalSnackbar } from "./components/GlobalSnackbar/GlobalSnackbar"
import { dark } from "./theme"
import "./theme/globalFonts"
import { XServiceProvider } from "./xServices/StateContext"

export const App: FC = () => {
return (
<Router>
<HelmetProvider>
<ThemeProvider theme={dark}>
<CssBaseline />
<ErrorBoundary>
<XServiceProvider>
<AppRouter />
<GlobalSnackbar />
</XServiceProvider>
</ErrorBoundary>
</ThemeProvider>
</HelmetProvider>
</Router>
<HelmetProvider>
<ThemeProvider theme={dark}>
<CssBaseline />
<ErrorBoundary>
<AuthProvider>
<AppRouter />
<GlobalSnackbar />
</AuthProvider>
</ErrorBoundary>
</ThemeProvider>
</HelmetProvider>
)
}
36 changes: 36 additions & 0 deletions site/src/components/AuthProvider/AuthProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { useActor, useInterpret } from "@xstate/react"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should move all Providers into a provider or state directory instead of them living mixed in with our components.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I would call it contexts. And I might rename these files to AuthContext, DashboardContext, etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see Provider being used more broadly in community but I'm ok with Context too. About where to put them, I think a context directory is good!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as we establish a consistent pattern, I'm not too picky!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally

import { createContext, FC, PropsWithChildren, useContext } from "react"
import { authMachine } from "xServices/auth/authXService"
import { ActorRefFrom } from "xstate"

interface AuthProviderContextValue {
authService: ActorRefFrom<typeof authMachine>
}

const AuthProviderContext = createContext<AuthProviderContextValue | undefined>(
Copy link
Member

@Kira-Pilot Kira-Pilot Jan 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: I would probably just name this AuthContext and createContext would take a AuthContextValue type

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But there is a difference. The AuthContext is the one created from React.createContext, and the Provider is the wrapper around it that define the values like data and additional functions

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get the difference - I would name it AuthContext for brevity, and also because it's named after the thing it's created by: a hook called createContext. You see the React docs using this pattern here. The interface you defined describes the values available on the context (you're right tho - they are enabled through a provider). That being said, if you prefer the longer name, I'm down. We should just establish a pattern since we'll be using React state more frequently.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I see. But in this case the Context is being used directly. We can think like Material UI that has a wrapper around the their theme context and named it ThemeProvider as example

undefined,
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would this type be potentially undefined? Because the machine hasn't initialized yet? If so, could we do something like:

const AuthProviderContext = createContext<AuthProviderContextValue>({
  authService: undefined
})

Or whatever the un-initialized value for authService would be? This way we can be a little more explicit about what the machine is returning early-state, and we also set ourselves up for the future where the context is returning other methods.

Copy link
Collaborator Author

@BrunoQuaresma BrunoQuaresma Jan 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is ok it uses undefined, it should not happen tho but the useAuth hook is doing the type validation already. I see creating empty context values as an anti pattern. There is a good blog post about this somewhere, I will try to find it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do - I wasn't aware and would love to read more. I'm curious how this pattern handles:

  1. data that comes back asynchronously
  2. data that errors differently

For example, if you had an auth context that returned the following values:

type AuthContextValue = {
  error: undefined | ApiError
  anotherSpecificError: undefined | ApiError
  currentUser: null | AuthContextQueryUser
  currentOrg: null | AuthContextQueryOrg
  specificUserPermission: null  | AuthContextQueryPerm
}

It might be nice to not have the entire context return value undefined if currentUser and currentOrg have returned but we are still waiting on specificUserPermission. Similarly, we can specify separate errors with this type of pattern, if we need that distinction within the page.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, at somepoint this provider is going to pass values like these instead of machine actors but I thought it would be too much to do right now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The undefined is just a type thing tho since the initialization happens when the component is used on AuthContext.Provider and pass the value props. It also help us to throw an erro if the user is using the context or the hook outside of the Provider.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About errors I don't think we need to be too much specific. We make that in the machines but I don't see too much vale tho and it adds significantly work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the naming, I think I only was able to understand it now 🤦‍♂️ AuthContext is definitely better than AuthProviderContext for sure.


export const AuthProvider: FC<PropsWithChildren> = ({ children }) => {
const authService = useInterpret(authMachine)

return (
<AuthProviderContext.Provider value={{ authService }}>
{children}
</AuthProviderContext.Provider>
)
}

// The returned type is kinda complex to rewrite it
// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types -- Read above
export const useAuth = () => {
const context = useContext(AuthProviderContext)

if (!context) {
throw new Error("useAuth should be used inside of <AuthProvider />")
}

const auth = useActor(context.authService)

return auth
}
5 changes: 3 additions & 2 deletions ...nents/DashboardLayout/DashboardLayout.tsx → .../components/Dashboard/DashboardLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { ServiceBanner } from "components/ServiceBanner/ServiceBanner"
import { updateCheckMachine } from "xServices/updateCheck/updateCheckXService"
import { usePermissions } from "hooks/usePermissions"
import { UpdateCheckResponse } from "api/typesGenerated"
import { DashboardProvider } from "./DashboardProvider"

export const DashboardLayout: FC = () => {
const styles = useStyles()
Expand All @@ -23,7 +24,7 @@ export const DashboardLayout: FC = () => {
const { error: updateCheckError, updateCheck } = updateCheckState.context

return (
<>
<DashboardProvider>
<ServiceBanner />
<LicenseBanner />

Expand All @@ -50,7 +51,7 @@ export const DashboardLayout: FC = () => {
</Suspense>
</div>
</div>
</>
</DashboardProvider>
)
}

Expand Down
Loading