From d95e2c46c0a423266d724b275e85ce9665242a46 Mon Sep 17 00:00:00 2001 From: Isaac Snow Date: Fri, 12 Oct 2018 13:41:56 -0700 Subject: [PATCH 1/6] feat: use access tokens for authentication --- src/browser/types.ts | 13 ++ src/shared/auth/access_token.ts | 37 ++++++ src/shared/backend/auth.ts | 54 +++++++++ src/shared/backend/graphql.tsx | 110 ++++++++++------- src/shared/backend/headers.tsx | 113 +++++++++++++----- src/shared/backend/lsp.tsx | 85 ++++++------- src/shared/backend/server.ts | 8 +- .../components/options/ConnectionCard.tsx | 23 ++-- 8 files changed, 312 insertions(+), 131 deletions(-) create mode 100644 src/shared/auth/access_token.ts create mode 100644 src/shared/backend/auth.ts diff --git a/src/browser/types.ts b/src/browser/types.ts index 1ba678a3..1d6725cd 100644 --- a/src/browser/types.ts +++ b/src/browser/types.ts @@ -20,10 +20,21 @@ export const featureFlagDefaults: FeatureFlags = { newInject: false, } +/** A map where the key is the server URL and the value is the token. */ +export interface AccessTokens { + [url: string]: string +} + // TODO(chris) Switch to Partial to eliminate bugs caused by // missing items. export interface StorageItems { sourcegraphURL: string + /** + * The current users access tokens the different sourcegraphUrls they have + * had configured. + */ + accessTokens: AccessTokens + gitHubEnterpriseURL: string phabricatorURL: string inlineSymbolSearchEnabled: boolean @@ -66,6 +77,8 @@ interface ClientConfigurationDetails { export const defaultStorageItems: StorageItems = { sourcegraphURL: 'https://sourcegraph.com', + accessTokens: {}, + serverUrls: ['https://sourcegraph.com'], gitHubEnterpriseURL: '', phabricatorURL: '', diff --git a/src/shared/auth/access_token.ts b/src/shared/auth/access_token.ts new file mode 100644 index 00000000..e19a94a9 --- /dev/null +++ b/src/shared/auth/access_token.ts @@ -0,0 +1,37 @@ +import { omit } from 'lodash' +import { Observable } from 'rxjs' +import { switchMap } from 'rxjs/operators' +import storage from '../../browser/storage' + +export const getAccessToken = (url: string): Observable => + new Observable(observer => { + storage.getSync(items => { + observer.next(items.accessTokens[url]) + observer.complete() + }) + }) + +export const setAccessToken = (url: string) => (tokens: Observable): Observable => + tokens.pipe( + switchMap( + token => + new Observable(observer => { + storage.getSync(({ accessTokens }) => + storage.setSync({ accessTokens: { ...accessTokens, [url]: token } }, () => { + observer.next(token) + observer.complete() + }) + ) + }) + ) + ) + +export const removeAccessToken = (url: string): Observable => + new Observable(observer => { + storage.getSync(({ accessTokens }) => + storage.setSync({ accessTokens: omit(accessTokens, url) }, () => { + observer.next() + observer.complete() + }) + ) + }) diff --git a/src/shared/backend/auth.ts b/src/shared/backend/auth.ts new file mode 100644 index 00000000..dc912900 --- /dev/null +++ b/src/shared/backend/auth.ts @@ -0,0 +1,54 @@ +import { Observable, Subject } from 'rxjs' +import { map, share, switchMap } from 'rxjs/operators' +import { GQL } from '../../types/gqlschema' +import { getPlatformName } from '../util/context' +import { getContext } from './context' +import { createAggregateError } from './errors' +import { mutateGraphQLNoRetry } from './graphql' + +/** + * Creeates an observable pipeline to create access tokens for a user. This + * ensures only one access token is created when multiple requests are going + * out and all need to create a new access token. They all share the created + * token. + */ +const buildAccessTokenCreator = () => { + const userIDs = new Subject() + + const accessTokenCreator = userIDs.pipe( + switchMap(userID => + mutateGraphQLNoRetry( + getContext({ repoKey: '' }), + ` + mutation CreateAccessToken($userID: ID!, $scopes: [String!]!, $note: String!) { + createAccessToken(user: $userID, scopes: $scopes, note: $note) { + id + token + } + } + `, + { userID, scopes: ['user:all'], note: `sourcegraph-${getPlatformName()}` }, + false + ) + ), + map(({ data, errors }) => { + if (!data || !data.createAccessToken || (errors && errors.length > 0)) { + throw createAggregateError(errors) + } + return data.createAccessToken.token + }), + share() + ) + + return (userID: GQL.ID): Observable => { + userIDs.next(userID) + + return accessTokenCreator + } +} + +/** + * Create an access token for the current user on the currently configured + * sourcegraph instance. + */ +export const createAccessToken = buildAccessTokenCreator() diff --git a/src/shared/backend/graphql.tsx b/src/shared/backend/graphql.tsx index 2af202ac..2891284e 100644 --- a/src/shared/backend/graphql.tsx +++ b/src/shared/backend/graphql.tsx @@ -2,8 +2,9 @@ import { QueryResult } from '@sourcegraph/extensions-client-common/lib/graphql' import { IQuery } from '@sourcegraph/extensions-client-common/lib/schema/graphqlschema' import { Observable, throwError } from 'rxjs' import { ajax } from 'rxjs/ajax' -import { catchError, map } from 'rxjs/operators' +import { catchError, map, switchMap } from 'rxjs/operators' import { GQL } from '../../types/gqlschema' +import { removeAccessToken } from '../auth/access_token' import { DEFAULT_SOURCEGRAPH_URL, isPrivateRepository, repoUrlCache, sourcegraphUrl } from '../util/context' import { RequestContext } from './context' import { AuthRequiredError, createAuthRequiredError, NoSourcegraphURLError } from './errors' @@ -30,6 +31,13 @@ function requestGraphQL( variables: any = {}, url: string = sourcegraphUrl, retry = true, + /** + * Whether or not to use an access token for the request. All requests + * except requests used while creating an access token should use an access + * token. i.e. `createAccessToken` and the `fetchCurrentUser` used to get the + * user ID for `createAccessToken`. + */ + useAccessToken = true, authError?: AuthRequiredError ): Observable { // Check if it's a private repo - if so don't make a request to Sourcegraph.com. @@ -39,44 +47,60 @@ function requestGraphQL( const nameMatch = request.match(/^\s*(?:query|mutation)\s+(\w+)/) const queryName = nameMatch ? '?' + nameMatch[1] : '' - return ajax({ - method: 'POST', - url: `${url}/.api/graphql` + queryName, - headers: getHeaders(), - crossDomain: true, - withCredentials: true, - body: JSON.stringify({ query: request, variables }), - async: true, - }).pipe( - map(({ response }) => { - if (shouldResponseTriggerRetryOrError(response)) { - delete repoUrlCache[ctx.repoKey] - throw response - } - if (ctx.isRepoSpecific && response.data.repository) { - repoUrlCache[ctx.repoKey] = url - } - return response - }), - catchError(err => { - if (err.status === 401) { - // Ensure all urls are tried and update authError to be the last seen 401. - // This ensures that the correct URL is used for sign in and also that all possible - // urls were checked. - authError = createAuthRequiredError(url) - } + return getHeaders(url, useAccessToken).pipe( + switchMap(headers => + ajax({ + method: 'POST', + url: `${url}/.api/graphql` + queryName, + headers, + crossDomain: true, + withCredentials: true, + body: JSON.stringify({ query: request, variables }), + async: true, + }).pipe( + map(({ response }) => { + if (shouldResponseTriggerRetryOrError(response)) { + delete repoUrlCache[ctx.repoKey] + throw response + } + if (ctx.isRepoSpecific && response.data.repository) { + repoUrlCache[ctx.repoKey] = url + } + return response + }), + catchError(err => { + if (err.status === 401) { + // Ensure all urls are tried and update authError to be the last seen 401. + // This ensures that the correct URL is used for sign in and also that all possible + // urls were checked. + authError = createAuthRequiredError(url) - if (!retry || url === DEFAULT_SOURCEGRAPH_URL) { - // If there was an auth error and we tried all of the possible URLs throw the auth error. - if (authError) { - throw authError - } - delete repoUrlCache[ctx.repoKey] - // We just tried the last url - throw err - } - return requestGraphQL(ctx, request, variables, DEFAULT_SOURCEGRAPH_URL, retry, authError) - }) + if (headers && headers.authorization) { + // If we got a 401 with a token, get rid of the and + // try again. The token may be invalid and we just + // need to recreate one. + return removeAccessToken(url).pipe( + switchMap(() => + requestGraphQL(ctx, request, variables, url, retry, useAccessToken, authError) + ) + ) + } + } + + if (!retry || url === DEFAULT_SOURCEGRAPH_URL) { + // If there was an auth error and we tried all of the possible URLs throw the auth error. + if (authError) { + throw authError + } + delete repoUrlCache[ctx.repoKey] + // We just tried the last url + throw err + } + + return requestGraphQL(ctx, request, variables, DEFAULT_SOURCEGRAPH_URL, retry, true, authError) + }) + ) + ) ) } @@ -140,9 +164,10 @@ export function queryGraphQLNoRetry( ctx: RequestContext, query: string, variables: any = {}, - url: string = sourcegraphUrl + url: string = sourcegraphUrl, + useAccessToken?: boolean ): Observable> { - return requestGraphQL(ctx, query, variables, url, false) as Observable> + return requestGraphQL(ctx, query, variables, url, false, useAccessToken) as Observable> } /** @@ -167,7 +192,8 @@ export function mutateGraphQL(ctx: RequestContext, mutation: string, variables: export function mutateGraphQLNoRetry( ctx: RequestContext, mutation: string, - variables: any = {} + variables: any = {}, + useAccessToken?: boolean ): Observable { - return requestGraphQL(ctx, mutation, variables, sourcegraphUrl, false) as Observable + return requestGraphQL(ctx, mutation, variables, sourcegraphUrl, false, useAccessToken) as Observable } diff --git a/src/shared/backend/headers.tsx b/src/shared/backend/headers.tsx index 697c6847..f63731c6 100644 --- a/src/shared/backend/headers.tsx +++ b/src/shared/backend/headers.tsx @@ -1,40 +1,93 @@ +import { Observable, of } from 'rxjs' +import { filter, map, switchMap } from 'rxjs/operators' + +import { isDefined } from '@sourcegraph/codeintellify/lib/helpers' +import { getAccessToken, setAccessToken } from '../auth/access_token' import { isInPage, isPhabricator } from '../context' import { getExtensionVersionSync, getPlatformName } from '../util/context' +import { createAccessToken } from './auth' +import { fetchCurrentUser } from './server' + +const withAccessToken = (url: string) => + getAccessToken(url).pipe( + switchMap(token => { + if (token) { + return of(token) + } + + return fetchCurrentUser(false).pipe( + filter(isDefined), + switchMap(user => createAccessToken(user.id).pipe(setAccessToken(url))) + ) + }) + ) /** - * getHeaders returns the required headers for making requests to Sourcegraph server instances. + * getHeaders emits the required headers for making requests to Sourcegraph server instances. * Requests can be blocked for various reasons and therefore the HTTP request MUST use the headers returned here. */ -export function getHeaders(): { [name: string]: string } | undefined { +export function getHeaders( + url: string, + /** + * Whether or not to use an access token for the request. All requests + * except requests used while creating an access token should use an access + * token. i.e. `createAccessToken` and the `fetchCurrentUser` used to get the + * user ID for `createAccessToken`. + */ + useToken = true +): Observable<{ [name: string]: string } | undefined> { if (isPhabricator && isInPage) { - return undefined + return of(undefined) } - // The HTTP request MUST contain at least one of the following for the Sourcegraph server to accept it - // (according to its CORS rules). - // - // - An Origin header with a URI that Sourcegraph trusts. The prod and dev Chrome extension origins - // (chrome-extension://...) are trusted, and site admins can specify other trusted origins in the site config - // "corsOrigin" property. - // - An X-Requested-With header (with any nonempty value). This tells the server that the request is from a - // trusted origin, because that header could not be added to a cross-domain request unless it already passed - // the server's CORS rules. (See - // https://stackoverflow.com/questions/17478731/whats-the-point-of-the-x-requested-with-header for more - // info.) - // - Using application/json as the Content-Type or Accept result in CORS blocking the request. - // - // The browsers all handle this situation differently. - // - // - Chrome (usually) automatically includes "Origin: chrome-extension://..." but does NOT allow us to include - // other headers (such as X-Requested-With), or else they are blocked by the CSP policy of GitHub (or other - // target page). Chrome sometimes sends "Origin: " instead; it's not clear in what - // cases or why this occurs. - // - Safari includes "Origin: " (where "" is the - // `window.location.origin` of the current page). - // - Firefox does NOT include any Origin header, so we need to send an "X-Requested-With" header. - const needsCORSHeader = getPlatformName() === 'firefox-extension' || isPhabricator - if (needsCORSHeader) { - return { 'X-Requested-With': `Sourcegraph - ${getPlatformName()} v${getExtensionVersionSync()}` } - } - return {} + return of(url).pipe( + switchMap(url => (useToken ? withAccessToken(url) : of(undefined))), + map(accessToken => { + const headers = new Headers() + if (accessToken) { + headers.append('Authorization', `token ${accessToken}`) + } + + return headers + }), + map(headers => { + // The HTTP request MUST contain at least one of the following for the Sourcegraph server to accept it + // (according to its CORS rules). + // + // - An Origin header with a URI that Sourcegraph trusts. The prod and dev Chrome extension origins + // (chrome-extension://...) are trusted, and site admins can specify other trusted origins in the site config + // "corsOrigin" property. + // - An X-Requested-With header (with any nonempty value). This tells the server that the request is from a + // trusted origin, because that header could not be added to a cross-domain request unless it already passed + // the server's CORS rules. (See + // https://stackoverflow.com/questions/17478731/whats-the-point-of-the-x-requested-with-header for more + // info.) + // - Using application/json as the Content-Type or Accept result in CORS blocking the request. + // + // The browsers all handle this situation differently. + // + // - Chrome (usually) automatically includes "Origin: chrome-extension://..." but does NOT allow us to include + // other headers (such as X-Requested-With), or else they are blocked by the CSP policy of GitHub (or other + // target page). Chrome sometimes sends "Origin: " instead; it's not clear in what + // cases or why this occurs. + // - Safari includes "Origin: " (where "" is the + // `window.location.origin` of the current page). + // - Firefox does NOT include any Origin header, so we need to send an "X-Requested-With" header. + const needsCORSHeader = getPlatformName() === 'firefox-extension' || isPhabricator + if (needsCORSHeader) { + headers.append('X-Requested-With', `Sourcegraph - ${getPlatformName()} v${getExtensionVersionSync()}`) + } + + return headers + }), + // rxjs ajax seems to not like the Header type so we should reconstruct it as a plain object. + map(headers => { + const objHeaders = {} + for (const [key, value] of headers.entries()) { + objHeaders[key] = value + } + + return objHeaders + }) + ) } diff --git a/src/shared/backend/lsp.tsx b/src/shared/backend/lsp.tsx index f000a10c..5a89e4dc 100644 --- a/src/shared/backend/lsp.tsx +++ b/src/shared/backend/lsp.tsx @@ -3,7 +3,7 @@ import { Controller } from '@sourcegraph/extensions-client-common/lib/client/con import { ConfigurationSubject, Settings } from '@sourcegraph/extensions-client-common/lib/settings' import { from, Observable, of, OperatorFunction, throwError, throwError as error } from 'rxjs' import { ajax, AjaxResponse } from 'rxjs/ajax' -import { catchError, map, tap } from 'rxjs/operators' +import { catchError, map, switchMap, tap } from 'rxjs/operators' import { HoverMerged } from 'sourcegraph/module/client/types/hover' import { TextDocumentPositionParams } from 'sourcegraph/module/protocol' import { Definition, TextDocumentIdentifier } from 'vscode-languageserver-types' @@ -44,6 +44,21 @@ export function isEmptyHover(hover: HoverMerged | null): boolean { return !hover || !hover.contents || (Array.isArray(hover.contents) && hover.contents.length === 0) } +const createRequest = (url: string, path: string, requests: any[]) => + getHeaders(url).pipe( + switchMap(headers => + ajax({ + method: 'POST', + url: `${url}/.api/xlang/${path || ''}`, + headers, + crossDomain: true, + withCredentials: true, + body: JSON.stringify(requests), + async: true, + }) + ) + ) + export function sendLSPHTTPRequests(requests: any[], url: string = sourcegraphUrl): Observable { const sendTo = (urlsToTry: string[]): Observable => { if (urlsToTry.length === 0) { @@ -52,15 +67,8 @@ export function sendLSPHTTPRequests(requests: any[], url: string = sourcegraphUr const urlToTry = urlsToTry[0] const urlPathHint = requests[1] && requests[1].method - return ajax({ - method: 'POST', - url: `${urlToTry}/.api/xlang/${urlPathHint || ''}`, - headers: getHeaders(), - crossDomain: true, - withCredentials: true, - body: JSON.stringify(requests), - async: true, - }).pipe( + + return createRequest(urlToTry, urlPathHint, requests).pipe( // Workaround for https://github.com/ReactiveX/rxjs/issues/3606 tap(response => { if (response.status === 0) { @@ -172,15 +180,7 @@ const fetchHover = memoizeObservable((pos: AbsoluteRepoFilePosition): Observable return of(null) } - return ajax({ - method: 'POST', - url: `${url}/.api/xlang/textDocument/hover`, - headers: getHeaders(), - crossDomain: true, - withCredentials: true, - body: JSON.stringify(body), - async: true, - }).pipe(extractLSPResponse) + return createRequest(url, 'textDocument/hover', body).pipe(extractLSPResponse) }, makeRepoURI) const fetchDefinition = memoizeObservable((pos: AbsoluteRepoFilePosition): Observable => { @@ -213,16 +213,7 @@ const fetchDefinition = memoizeObservable((pos: AbsoluteRepoFilePosition): Obser if (!canFetchForURL(url)) { return of([]) } - - return ajax({ - method: 'POST', - url: `${url}/.api/xlang/textDocument/definition`, - headers: getHeaders(), - crossDomain: true, - withCredentials: true, - body: JSON.stringify(body), - async: true, - }).pipe(extractLSPResponse) + return createRequest(url, 'textDocument/definition', body).pipe(extractLSPResponse) }, makeRepoURI) export function fetchJumpURL( @@ -287,28 +278,22 @@ const fetchServerCapabilities = (pos: AbsoluteRepoLanguageFile): Observable m !== null) - ), - async: true, - }).pipe( + }, + { id: 1, method: 'shutdown' }, + { method: 'exit' }, + ].filter(m => m !== null) + ).pipe( tap(response => { if (response.status === 0) { throw Object.assign(new Error('Ajax status 0'), response) diff --git a/src/shared/backend/server.ts b/src/shared/backend/server.ts index e4fb779c..d7618773 100644 --- a/src/shared/backend/server.ts +++ b/src/shared/backend/server.ts @@ -29,11 +29,12 @@ export const resolveClientConfiguration = (): Observable caught)) ) -export const fetchCurrentUser = (): Observable => +export const fetchCurrentUser = (useToken = true): Observable => queryGraphQLNoRetry( getContext({ repoKey: '' }), `query CurrentUser() { currentUser { + id displayName username avatarURL @@ -43,7 +44,10 @@ export const fetchCurrentUser = (): Observable => } siteAdmin } - }` + }`, + undefined, + undefined, + useToken ).pipe( map(result => { if (!result || !result.data || !result.data.currentUser) { diff --git a/src/shared/components/options/ConnectionCard.tsx b/src/shared/components/options/ConnectionCard.tsx index 94018615..8835cd10 100644 --- a/src/shared/components/options/ConnectionCard.tsx +++ b/src/shared/components/options/ConnectionCard.tsx @@ -14,6 +14,7 @@ import { ListGroupItemHeading, Row, } from 'reactstrap' +import { Subscription } from 'rxjs' import * as permissions from '../../../browser/permissions' import storage from '../../../browser/storage' import { StorageItems } from '../../../browser/types' @@ -37,6 +38,8 @@ export class ConnectionCard extends React.Component { private urlInput: HTMLInputElement | null private contentScriptUrls: string[] + private subscriptions = new Subscription() + constructor(props: Props) { super(props) this.state = { @@ -58,6 +61,10 @@ export class ConnectionCard extends React.Component { this.setContentScriptUrls(nextProps) } + public componentWillUnmount(): void { + this.subscriptions.unsubscribe() + } + private sourcegraphServerAlert = (): JSX.Element => { const { permissionOrigins } = this.props if (isSourcegraphDotCom()) { @@ -225,13 +232,15 @@ export class ConnectionCard extends React.Component { } private checkConnection = (): void => { - fetchSite().subscribe( - site => { - this.setState(() => ({ site })) - }, - () => { - this.setState(() => ({ site: undefined })) - } + this.subscriptions.add( + fetchSite().subscribe( + site => { + this.setState({ site }) + }, + () => { + this.setState({ site: undefined }) + } + ) ) } From eca599cfa7e5126ffcb47e72bb63d265b7f3dd16 Mon Sep 17 00:00:00 2001 From: Isaac Snow Date: Mon, 15 Oct 2018 11:47:48 -0700 Subject: [PATCH 2/6] fix: add access tokens via migration --- src/extension/scripts/background.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/extension/scripts/background.tsx b/src/extension/scripts/background.tsx index 2e0d6fdd..bc5336f4 100644 --- a/src/extension/scripts/background.tsx +++ b/src/extension/scripts/background.tsx @@ -162,6 +162,10 @@ permissions.onRemoved(permissions => { }) storage.addSyncMigration((items, set, remove) => { + if (!items.accessTokens) { + set({ accessTokens: {} }) + } + if (items.phabricatorURL) { remove('phabricatorURL') From f2bef88b4deb944f467848c980b3a30b7dfdc351 Mon Sep 17 00:00:00 2001 From: Isaac Snow Date: Mon, 15 Oct 2018 13:46:18 -0700 Subject: [PATCH 3/6] chore: address review comments --- src/shared/backend/graphql.tsx | 42 ++++++++++++++++++++++++---------- src/shared/backend/lsp.tsx | 12 +++++----- 2 files changed, 36 insertions(+), 18 deletions(-) diff --git a/src/shared/backend/graphql.tsx b/src/shared/backend/graphql.tsx index 2891284e..4e0fe021 100644 --- a/src/shared/backend/graphql.tsx +++ b/src/shared/backend/graphql.tsx @@ -18,11 +18,25 @@ export interface MutationResult { errors?: GQL.IGraphQLResponseError[] } +interface RequestGraphQLOptions { + /** Whether we should use the retry logic to fall back to other URLs. */ + retry?: boolean + /** + * Whether or not to use an access token for the request. All requests + * except requests used while creating an access token should use an access + * token. i.e. `createAccessToken` and the `fetchCurrentUser` used to get the + * user ID for `createAccessToken`. + */ + useAccessToken?: boolean +} + /** * Does a GraphQL request to the Sourcegraph GraphQL API running under `/.api/graphql` * * @param request The GraphQL request (query or mutation) * @param variables A key/value object with variable values + * @param url the url the request is going to + * @param options configuration options for the request * @return Observable That emits the result or errors if the HTTP request failed */ function requestGraphQL( @@ -30,14 +44,7 @@ function requestGraphQL( request: string, variables: any = {}, url: string = sourcegraphUrl, - retry = true, - /** - * Whether or not to use an access token for the request. All requests - * except requests used while creating an access token should use an access - * token. i.e. `createAccessToken` and the `fetchCurrentUser` used to get the - * user ID for `createAccessToken`. - */ - useAccessToken = true, + { retry, useAccessToken }: RequestGraphQLOptions = { retry: true, useAccessToken: true }, authError?: AuthRequiredError ): Observable { // Check if it's a private repo - if so don't make a request to Sourcegraph.com. @@ -81,7 +88,7 @@ function requestGraphQL( // need to recreate one. return removeAccessToken(url).pipe( switchMap(() => - requestGraphQL(ctx, request, variables, url, retry, useAccessToken, authError) + requestGraphQL(ctx, request, variables, url, { retry, useAccessToken }, authError) ) ) } @@ -97,7 +104,14 @@ function requestGraphQL( throw err } - return requestGraphQL(ctx, request, variables, DEFAULT_SOURCEGRAPH_URL, retry, true, authError) + return requestGraphQL( + ctx, + request, + variables, + DEFAULT_SOURCEGRAPH_URL, + { retry, useAccessToken: true }, + authError + ) }) ) ) @@ -167,7 +181,9 @@ export function queryGraphQLNoRetry( url: string = sourcegraphUrl, useAccessToken?: boolean ): Observable> { - return requestGraphQL(ctx, query, variables, url, false, useAccessToken) as Observable> + return requestGraphQL(ctx, query, variables, url, { retry: false, useAccessToken }) as Observable< + QueryResult + > } /** @@ -195,5 +211,7 @@ export function mutateGraphQLNoRetry( variables: any = {}, useAccessToken?: boolean ): Observable { - return requestGraphQL(ctx, mutation, variables, sourcegraphUrl, false, useAccessToken) as Observable + return requestGraphQL(ctx, mutation, variables, sourcegraphUrl, { retry: false, useAccessToken }) as Observable< + MutationResult + > } diff --git a/src/shared/backend/lsp.tsx b/src/shared/backend/lsp.tsx index 5a89e4dc..7f278d30 100644 --- a/src/shared/backend/lsp.tsx +++ b/src/shared/backend/lsp.tsx @@ -44,12 +44,12 @@ export function isEmptyHover(hover: HoverMerged | null): boolean { return !hover || !hover.contents || (Array.isArray(hover.contents) && hover.contents.length === 0) } -const createRequest = (url: string, path: string, requests: any[]) => +const request = (url: string, method: string, requests: any[]): Observable => getHeaders(url).pipe( switchMap(headers => ajax({ method: 'POST', - url: `${url}/.api/xlang/${path || ''}`, + url: `${url}/.api/xlang/${method || ''}`, headers, crossDomain: true, withCredentials: true, @@ -68,7 +68,7 @@ export function sendLSPHTTPRequests(requests: any[], url: string = sourcegraphUr const urlToTry = urlsToTry[0] const urlPathHint = requests[1] && requests[1].method - return createRequest(urlToTry, urlPathHint, requests).pipe( + return request(urlToTry, urlPathHint, requests).pipe( // Workaround for https://github.com/ReactiveX/rxjs/issues/3606 tap(response => { if (response.status === 0) { @@ -180,7 +180,7 @@ const fetchHover = memoizeObservable((pos: AbsoluteRepoFilePosition): Observable return of(null) } - return createRequest(url, 'textDocument/hover', body).pipe(extractLSPResponse) + return request(url, 'textDocument/hover', body).pipe(extractLSPResponse) }, makeRepoURI) const fetchDefinition = memoizeObservable((pos: AbsoluteRepoFilePosition): Observable => { @@ -213,7 +213,7 @@ const fetchDefinition = memoizeObservable((pos: AbsoluteRepoFilePosition): Obser if (!canFetchForURL(url)) { return of([]) } - return createRequest(url, 'textDocument/definition', body).pipe(extractLSPResponse) + return request(url, 'textDocument/definition', body).pipe(extractLSPResponse) }, makeRepoURI) export function fetchJumpURL( @@ -278,7 +278,7 @@ const fetchServerCapabilities = (pos: AbsoluteRepoLanguageFile): Observable Date: Mon, 15 Oct 2018 14:52:03 -0700 Subject: [PATCH 4/6] chore: use memoizeObservable instead of custom logic for access tokens --- src/shared/backend/auth.ts | 46 +++++++++++--------------------------- 1 file changed, 13 insertions(+), 33 deletions(-) diff --git a/src/shared/backend/auth.ts b/src/shared/backend/auth.ts index dc912900..2d02836d 100644 --- a/src/shared/backend/auth.ts +++ b/src/shared/backend/auth.ts @@ -1,25 +1,19 @@ -import { Observable, Subject } from 'rxjs' -import { map, share, switchMap } from 'rxjs/operators' +import { map } from 'rxjs/operators' import { GQL } from '../../types/gqlschema' import { getPlatformName } from '../util/context' +import { memoizeObservable } from '../util/memoize' import { getContext } from './context' import { createAggregateError } from './errors' import { mutateGraphQLNoRetry } from './graphql' /** - * Creeates an observable pipeline to create access tokens for a user. This - * ensures only one access token is created when multiple requests are going - * out and all need to create a new access token. They all share the created - * token. + * Create an access token for the current user on the currently configured + * sourcegraph instance. */ -const buildAccessTokenCreator = () => { - const userIDs = new Subject() - - const accessTokenCreator = userIDs.pipe( - switchMap(userID => - mutateGraphQLNoRetry( - getContext({ repoKey: '' }), - ` +export const createAccessToken = memoizeObservable((userID: GQL.ID) => + mutateGraphQLNoRetry( + getContext({ repoKey: '' }), + ` mutation CreateAccessToken($userID: ID!, $scopes: [String!]!, $note: String!) { createAccessToken(user: $userID, scopes: $scopes, note: $note) { id @@ -27,28 +21,14 @@ const buildAccessTokenCreator = () => { } } `, - { userID, scopes: ['user:all'], note: `sourcegraph-${getPlatformName()}` }, - false - ) - ), + { userID, scopes: ['user:all'], note: `sourcegraph-${getPlatformName()}` }, + false + ).pipe( map(({ data, errors }) => { if (!data || !data.createAccessToken || (errors && errors.length > 0)) { throw createAggregateError(errors) } return data.createAccessToken.token - }), - share() + }) ) - - return (userID: GQL.ID): Observable => { - userIDs.next(userID) - - return accessTokenCreator - } -} - -/** - * Create an access token for the current user on the currently configured - * sourcegraph instance. - */ -export const createAccessToken = buildAccessTokenCreator() +) From ef96c1da97daa4cc52fdbf4eeab161c9a6a52b1e Mon Sep 17 00:00:00 2001 From: Isaac Snow Date: Mon, 15 Oct 2018 14:52:25 -0700 Subject: [PATCH 5/6] chore: format --- src/shared/backend/auth.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/shared/backend/auth.ts b/src/shared/backend/auth.ts index 2d02836d..3697e5e7 100644 --- a/src/shared/backend/auth.ts +++ b/src/shared/backend/auth.ts @@ -14,13 +14,13 @@ export const createAccessToken = memoizeObservable((userID: GQL.ID) => mutateGraphQLNoRetry( getContext({ repoKey: '' }), ` - mutation CreateAccessToken($userID: ID!, $scopes: [String!]!, $note: String!) { - createAccessToken(user: $userID, scopes: $scopes, note: $note) { - id - token - } - } - `, + mutation CreateAccessToken($userID: ID!, $scopes: [String!]!, $note: String!) { + createAccessToken(user: $userID, scopes: $scopes, note: $note) { + id + token + } + } + `, { userID, scopes: ['user:all'], note: `sourcegraph-${getPlatformName()}` }, false ).pipe( From 662bb46c50996c17cb164f1b9df100a3ca6e31d3 Mon Sep 17 00:00:00 2001 From: Isaac Snow Date: Wed, 17 Oct 2018 11:23:44 -0700 Subject: [PATCH 6/6] fix: don't require token usage on sourcegraph.com --- src/shared/backend/headers.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/shared/backend/headers.tsx b/src/shared/backend/headers.tsx index f63731c6..5caf4c6a 100644 --- a/src/shared/backend/headers.tsx +++ b/src/shared/backend/headers.tsx @@ -4,7 +4,7 @@ import { filter, map, switchMap } from 'rxjs/operators' import { isDefined } from '@sourcegraph/codeintellify/lib/helpers' import { getAccessToken, setAccessToken } from '../auth/access_token' import { isInPage, isPhabricator } from '../context' -import { getExtensionVersionSync, getPlatformName } from '../util/context' +import { getExtensionVersionSync, getPlatformName, isSourcegraphDotCom } from '../util/context' import { createAccessToken } from './auth' import { fetchCurrentUser } from './server' @@ -41,7 +41,7 @@ export function getHeaders( } return of(url).pipe( - switchMap(url => (useToken ? withAccessToken(url) : of(undefined))), + switchMap(url => (useToken && !isSourcegraphDotCom(url) ? withAccessToken(url) : of(undefined))), map(accessToken => { const headers = new Headers() if (accessToken) {