Skip to content
This repository was archived by the owner on Jan 22, 2019. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 13 additions & 0 deletions src/browser/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<StorageItems> 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
Expand Down Expand Up @@ -66,6 +77,8 @@ interface ClientConfigurationDetails {

export const defaultStorageItems: StorageItems = {
sourcegraphURL: 'https://sourcegraph.com',
accessTokens: {},

serverUrls: ['https://sourcegraph.com'],
gitHubEnterpriseURL: '',
phabricatorURL: '',
Expand Down
4 changes: 4 additions & 0 deletions src/extension/scripts/background.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,10 @@ permissions.onRemoved(permissions => {
})

storage.addSyncMigration((items, set, remove) => {
if (!items.accessTokens) {
set({ accessTokens: {} })
}

if (items.phabricatorURL) {
remove('phabricatorURL')

Expand Down
37 changes: 37 additions & 0 deletions src/shared/auth/access_token.ts
Original file line number Diff line number Diff line change
@@ -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<string | undefined> =>
new Observable(observer => {
storage.getSync(items => {
observer.next(items.accessTokens[url])

Choose a reason for hiding this comment

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

Careful, what if accessTokens is not defined?

Copy link
Author

Choose a reason for hiding this comment

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

Added to migration. It'll be defined.

observer.complete()
})
})

export const setAccessToken = (url: string) => (tokens: Observable<string>): Observable<string> =>
tokens.pipe(
switchMap(
token =>
new Observable<string>(observer => {
storage.getSync(({ accessTokens }) =>
storage.setSync({ accessTokens: { ...accessTokens, [url]: token } }, () => {
observer.next(token)
observer.complete()
})
)
})
Copy link

@felixfbecker felixfbecker Oct 15, 2018

Choose a reason for hiding this comment

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

Since this operation (storage.getSync()/storage.setSync()) is not cancellable anyway I would use an async function/Promises here, e.g.

switchMap(async token => {
    const { accessTokens } = await new Promise(resolve => storage.getSync(resolve))
    await new Promise(resolve => storage.setSync({ accessTokens: { ...accessTokens, [url]: token } }, resolve))
	return token
})

Note how this code is slightly safer too because it protects against potential exceptions raised within the getSync() and setSync() callbacks.

Copy link
Author

Choose a reason for hiding this comment

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

Is the await at the bottom supposed to be the return?

Copy link
Author

Choose a reason for hiding this comment

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

I tried and the resulting code feels weird to me. It mixes patterns in one function declaration.

Choose a reason for hiding this comment

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

I don't care strongly about this, but I'd say it uses the right pattern in the right context. It's bad to mix equivalent patterns like callbacks and Promises, but Observables and Promises are different patterns (note that the current state mixes the callback pattern with Observables). Observables represent 0..n values, Promises a single value (or an operation / side effect sequence). It is therefor totally okay to e.g. map each single value of the 0..n values of an Observable to a single other value represented by a Promise. That's why switchMap etc all support returning a Promise/taking an async function.
Using the Observable constructor on the other hand is very uncommon inside a switchMap callback, it is usually only used for functions when needed. Usually you'd use an operator like bindCallback (which would also remove the mixing of callbacks and Observables and have the same protection benefit I pointed out in my previous comment).
The goal of the function here is more causing side effects than returning a stream of values, which Promises are more suitable for than Observables.

You are free to decide, just wanted to give some perspective 🙌

)
)

export const removeAccessToken = (url: string): Observable<void> =>
new Observable(observer => {
storage.getSync(({ accessTokens }) =>
storage.setSync({ accessTokens: omit(accessTokens, url) }, () => {
observer.next()
observer.complete()
})
)
})

Choose a reason for hiding this comment

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

Same here

34 changes: 34 additions & 0 deletions src/shared/backend/auth.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
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'

/**
* Create an access token for the current user on the currently configured
* sourcegraph instance.
*/
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
}
}
`,
{ 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
})
)
)
130 changes: 87 additions & 43 deletions src/shared/backend/graphql.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -17,19 +18,33 @@ 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(
ctx: RequestContext,
request: string,
variables: any = {},
url: string = sourcegraphUrl,
retry = true,
{ retry, useAccessToken }: RequestGraphQLOptions = { retry: true, useAccessToken: true },
authError?: AuthRequiredError
): Observable<GQL.IGraphQLResponseRoot> {
// Check if it's a private repo - if so don't make a request to Sourcegraph.com.
Expand All @@ -39,44 +54,67 @@ 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 (headers && headers.authorization) {
// If we got a 401 with a token, get rid of the and

Choose a reason for hiding this comment

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

Typo somewhere

// 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
}

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)
})
return requestGraphQL(
ctx,
request,
variables,
DEFAULT_SOURCEGRAPH_URL,
{ retry, useAccessToken: true },
authError
)
})
)
)
)
}

Expand Down Expand Up @@ -140,9 +178,12 @@ export function queryGraphQLNoRetry(
ctx: RequestContext,
query: string,
variables: any = {},
url: string = sourcegraphUrl
url: string = sourcegraphUrl,
useAccessToken?: boolean
): Observable<QueryResult<IQuery>> {
return requestGraphQL(ctx, query, variables, url, false) as Observable<QueryResult<IQuery>>
return requestGraphQL(ctx, query, variables, url, { retry: false, useAccessToken }) as Observable<
QueryResult<IQuery>
>
}

/**
Expand All @@ -167,7 +208,10 @@ export function mutateGraphQL(ctx: RequestContext, mutation: string, variables:
export function mutateGraphQLNoRetry(
ctx: RequestContext,
mutation: string,
variables: any = {}
variables: any = {},
useAccessToken?: boolean
): Observable<MutationResult> {
return requestGraphQL(ctx, mutation, variables, sourcegraphUrl, false) as Observable<MutationResult>
return requestGraphQL(ctx, mutation, variables, sourcegraphUrl, { retry: false, useAccessToken }) as Observable<
MutationResult
>
}
Loading