Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.
1 change: 1 addition & 0 deletions client/web/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -1712,6 +1712,7 @@ ts_project(
"src/user/settings/RedirectToUserPage.tsx",
"src/user/settings/RedirectToUserSettings.tsx",
"src/user/settings/ScimAlert.tsx",
"src/user/settings/UserGitHubAppsArea.tsx",
"src/user/settings/UserSettingsArea.tsx",
"src/user/settings/UserSettingsSidebar.tsx",
"src/user/settings/accessTokens/UserSettingsCreateAccessTokenCallbackPage.tsx",
Expand Down
15 changes: 12 additions & 3 deletions client/web/src/components/gitHubApps/GitHubAppPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,12 @@ import {
// eslint-disable-next-line no-restricted-imports
import type { BreadcrumbItem } from '@sourcegraph/wildcard/src/components/PageHeader'

import { GitHubAppDomain, type GitHubAppByIDResult, type GitHubAppByIDVariables } from '../../graphql-operations'
import {
GitHubAppDomain,
type GitHubAppByIDResult,
type GitHubAppByIDVariables,
GitHubAppKind,
} from '../../graphql-operations'
import { ExternalServiceNode } from '../externalServices/ExternalServiceNode'
import { ConnectionList, ConnectionSummary, SummaryContainer } from '../FilteredConnection/ui'
import { PageTitle } from '../PageTitle'
Expand All @@ -44,6 +49,7 @@ interface Props extends TelemetryProps, TelemetryV2Props {
* The parent breadcrumb item to show for this page in the header.
*/
headerParentBreadcrumb: BreadcrumbItem
isSiteAdmin: boolean
/** An optional annotation to show in the page header. */
headerAnnotation?: React.ReactNode
}
Expand All @@ -53,6 +59,7 @@ export const GitHubAppPage: FC<Props> = ({
telemetryRecorder,
headerParentBreadcrumb,
headerAnnotation,
isSiteAdmin,
}) => {
const { appID } = useParams()
const navigate = useNavigate()
Expand Down Expand Up @@ -81,7 +88,9 @@ export const GitHubAppPage: FC<Props> = ({

const onAddInstallation = async (app: NonNullable<GitHubAppByIDResult['gitHubApp']>): Promise<void> => {
try {
const req = await fetch(`/githubapp/state?id=${app?.id}&domain=${app?.domain}`)
const req = await fetch(
`/githubapp/state?id=${app?.id}&domain=${app?.domain}&kind=${GitHubAppKind.USER_CREDENTIAL}`
Copy link
Member

Choose a reason for hiding this comment

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

Q: why did this change? Does adding the kind here still work for site admin setup?

)
const state = await req.text()
const trailingSlash = app.appURL.endsWith('/') ? '' : '/'
window.location.assign(`${app.appURL}${trailingSlash}installations/new?state=${state}`)
Expand All @@ -100,7 +109,7 @@ export const GitHubAppPage: FC<Props> = ({
{removeModalOpen && (
<RemoveGitHubAppModal
onCancel={() => setRemoveModalOpen(false)}
afterDelete={() => navigate('/site-admin/github-apps')}
afterDelete={() => navigate(`/${isSiteAdmin ? 'site-admin' : 'user/settings'}/github-apps`)}
app={app}
/>
)}
Expand Down
48 changes: 33 additions & 15 deletions client/web/src/components/gitHubApps/GitHubAppsPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,13 @@ import styles from './GitHubAppsPage.module.scss'

interface Props extends TelemetryV2Props {
batchChangesEnabled: boolean
isSiteAdmin: boolean
}

export const GitHubAppsPage: React.FC<Props> = ({ batchChangesEnabled, telemetryRecorder }) => {
export const GitHubAppsPage: React.FC<Props> = ({ batchChangesEnabled, telemetryRecorder, isSiteAdmin }) => {
Copy link
Member

Choose a reason for hiding this comment

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

nit: non-blocking, but given the amount of branching here, it kinda feels like this should just be a separate component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree. The flag isSiteAdmin is used a couple times, but only in ternary statements. I find it still easy to read and understand how we're switching things around. If you feel strongly about this, I'm happy to change it.

const { data, loading, error, refetch } = useQuery<GitHubAppsResult, GitHubAppsVariables>(GITHUB_APPS_QUERY, {
variables: {
domain: GitHubAppDomain.REPOS,
domain: isSiteAdmin ? GitHubAppDomain.REPOS : GitHubAppDomain.BATCHES,
},
})
const gitHubApps = useMemo(() => data?.gitHubApps?.nodes ?? [], [data])
Expand Down Expand Up @@ -63,28 +64,45 @@ export const GitHubAppsPage: React.FC<Props> = ({ batchChangesEnabled, telemetry
className={classNames(styles.pageHeader, 'mb-3')}
description={
<>
Create and connect a GitHub App to better manage GitHub code host connections.{' '}
<Link to="/help/admin/code_hosts/github#using-a-github-app" target="_blank">
See how GitHub App configuration works.
</Link>
{batchChangesEnabled && (
{isSiteAdmin ? (
<>
Create and connect a GitHub App to better manage GitHub code host connections.{' '}
<Link to="/help/admin/code_hosts/github#using-a-github-app" target="_blank">
See how GitHub App configuration works.
</Link>
</>
) : batchChangesEnabled ? (
<>Use personal GitHub Apps to act on your behalf when running Batch Changes.</>
) : (
<>
Personal GitHub Apps are currently only used for Batch Changes, but this feature is not
enabled on your instance.
</>
Copy link
Member

Choose a reason for hiding this comment

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

If batch changes is not enabled, should we just hide the personal github apps page entirely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This already happens via a condition in routes.tsx. I added this text here in case the other condition breaks, and we accidentally render this page. Customers shouldn't see it, but I thought it'd be nicer to display a helpful message if that assumption breaks.

)}
{batchChangesEnabled && isSiteAdmin ? (
<>
{' '}
To create a GitHub App to sign Batch Changes commits, visit{' '}
<Link to="/site-admin/batch-changes">Batch Changes settings</Link>.
</>
) : (
<> To create a GitHub App to sign Batch Changes commits, ask your site admin.</>
)}
</>
}
actions={
<ButtonLink
to="/site-admin/github-apps/new"
className="ml-auto text-nowrap"
variant="primary"
as={Link}
>
<Icon aria-hidden={true} svgPath={mdiPlus} /> Create GitHub App
</ButtonLink>
isSiteAdmin ? (
<ButtonLink
to="/site-admin/github-apps/new"
className="ml-auto text-nowrap"
variant="primary"
as={Link}
>
<Icon aria-hidden={true} svgPath={mdiPlus} /> Create GitHub App
</ButtonLink>
) : (
<></>
)
}
/>
<Container className="mb-3">
Expand Down
2 changes: 2 additions & 0 deletions client/web/src/site-admin/SiteAdminGitHubAppsArea.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ export const SiteAdminGitHubAppsArea: FC<Props> = props => {
<GitHubAppsPage
batchChangesEnabled={props.batchChangesEnabled}
telemetryRecorder={props.platformContext.telemetryRecorder}
isSiteAdmin={true}
/>
}
/>
Expand All @@ -98,6 +99,7 @@ export const SiteAdminGitHubAppsArea: FC<Props> = props => {
<GitHubAppPage
headerParentBreadcrumb={{ to: '/site-admin/github-apps', text: 'GitHub Apps' }}
telemetryRecorder={props.platformContext.telemetryRecorder}
isSiteAdmin={true}
{...props}
/>
}
Expand Down
1 change: 1 addition & 0 deletions client/web/src/site-admin/routes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,7 @@ export const otherSiteAdminRoutes: readonly SiteAdminAreaRoute[] = [
headerAnnotation={<FeedbackBadge status="beta" feedback={{ mailto: 'support@sourcegraph.com' }} />}
telemetryService={props.telemetryService}
telemetryRecorder={props.platformContext.telemetryRecorder}
isSiteAdmin={true}
/>
),
condition: ({ batchChangesEnabled }) => batchChangesEnabled,
Expand Down
67 changes: 67 additions & 0 deletions client/web/src/user/settings/UserGitHubAppsArea.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import type { FC } from 'react'

import { Route, Routes } from 'react-router-dom'

import { useQuery } from '@sourcegraph/http-client'
import type { AuthenticatedUser } from '@sourcegraph/shared/src/auth'
import type { PlatformContextProps } from '@sourcegraph/shared/src/platform/context'
import type { TelemetryProps } from '@sourcegraph/shared/src/telemetry/telemetryService'
import { lazyComponent } from '@sourcegraph/shared/src/util/lazyComponent'
import { ErrorAlert, LoadingSpinner } from '@sourcegraph/wildcard'

import { type SiteExternalServiceConfigResult, type SiteExternalServiceConfigVariables } from '../../graphql-operations'
import { SITE_EXTERNAL_SERVICE_CONFIG } from '../../site-admin/backend'

const GitHubAppPage = lazyComponent(() => import('../../components/gitHubApps/GitHubAppPage'), 'GitHubAppPage')
const GitHubAppsPage = lazyComponent(() => import('../../components/gitHubApps/GitHubAppsPage'), 'GitHubAppsPage')

interface Props extends TelemetryProps, PlatformContextProps {
authenticatedUser: AuthenticatedUser
batchChangesEnabled: boolean
}

export const UserGitHubAppsArea: FC<Props> = props => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should allow creation of GitHub apps in here, why?
Because the GitHub app will be orphaned and not tied to any Sourcegraph services.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's reasonable, and since we have text there indicating that github apps are for batch changes, they should find their way to the batch changes settings. I updated the code to hide the button if we're not in the siteAdmin context.

const { data, error, loading } = useQuery<SiteExternalServiceConfigResult, SiteExternalServiceConfigVariables>(
SITE_EXTERNAL_SERVICE_CONFIG,
{}
)

if (error && !loading) {
return <ErrorAlert error={error} />
}

if (loading && !error) {
return <LoadingSpinner />
}

if (!data) {
return null
}

return (
<Routes>
<Route
index={true}
element={
<GitHubAppsPage
batchChangesEnabled={props.batchChangesEnabled}
telemetryRecorder={props.platformContext.telemetryRecorder}
isSiteAdmin={false}
/>
}
/>

<Route
path=":appID"
element={
<GitHubAppPage
headerParentBreadcrumb={{ to: '/user/github-apps', text: 'GitHub Apps' }}
telemetryRecorder={props.platformContext.telemetryRecorder}
isSiteAdmin={false}
{...props}
/>
}
/>
</Routes>
)
}
6 changes: 6 additions & 0 deletions client/web/src/user/settings/routes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,12 @@ export const userSettingsAreaRoutes: readonly UserSettingsAreaRoute[] = [
),
condition: shouldRenderBatchChangesPage,
},
{
path: 'github-apps/*',
render: lazyComponent(() => import('./UserGitHubAppsArea'), 'UserGitHubAppsArea'),
// GitHub Apps are currently only relevant for users who use them with batch changes. If they are used for other things too, you can remove this condition.
condition: shouldRenderBatchChangesPage,
},
]

interface UserSettingAreaIndexPageProps extends PlatformContextProps, SettingsCascadeProps, TelemetryProps {
Expand Down
5 changes: 5 additions & 0 deletions client/web/src/user/settings/sidebaritems.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,4 +61,9 @@ export const userSettingsSideBarItems: UserSettingsSidebarItems = [
label: 'Event log',
condition: ({ user: { viewerCanAdminister } }) => viewerCanAdminister,
},
{
to: '/github-apps',
label: 'GitHub Apps',
condition: ({ user: { viewerCanAdminister } }) => viewerCanAdminister,
},
]
2 changes: 2 additions & 0 deletions cmd/frontend/internal/githubapp/httpapi.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ func (srv *gitHubAppServer) stateHandler(w http.ResponseWriter, r *http.Request)
gqlID := r.URL.Query().Get("id")
domain := r.URL.Query().Get("domain")
baseURL := r.URL.Query().Get("baseURL")
kind := r.URL.Query().Get("kind")
if gqlID == "" {
// we marshal an empty `gitHubAppStateDetails` struct because we want the values saved in the cache
// to always conform to the same structure.
Expand All @@ -159,6 +160,7 @@ func (srv *gitHubAppServer) stateHandler(w http.ResponseWriter, r *http.Request)
AppID: int(id64),
Domain: domain,
BaseURL: baseURL,
Kind: kind,
Copy link
Member

Choose a reason for hiding this comment

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

Were we accidentally omitting kind before?

})
if err != nil {
http.Error(w, fmt.Sprintf("Unexpected error when marshalling state: %s", err.Error()), http.StatusInternalServerError)
Expand Down