Skip to content
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

Jump to bottom

feat: Add ability to set a shareId #267

Merged
merged 14 commits into from
May 7, 2020
Merged

feat: Add ability to set a shareId #267

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
258d18e
Allow shareToken to be passed in separately so both headers can be us…
May 4, 2020
90e49e5
Rename shareToken to shareId
May 5, 2020
96a1770
Remove ShareDescriptor as possible value for accessToken
May 5, 2020
c3329ca
Update tests
May 5, 2020
fc31e04
Change variable name to match value better
May 6, 2020
ea1784e
Add test
May 6, 2020
16a5335
PR feedback + tests
May 6, 2020
e6592ee
Update ts types
May 6, 2020
fb8df53
Remove maybe from ShareIdentifier string
anthony-j-castro May 6, 2020
43352f2
Inline type
May 6, 2020
fa0c96a
First stab at doc copy
May 6, 2020
b02b2b6
Update ts type too
May 6, 2020
13a29a2
Remove redudant description
May 7, 2020
561ab2e
Merge branch 'master' into allow-share-and-auth-tokens
May 7, 2020
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
8 changes: 6 additions & 2 deletions abstract-sdk.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1832,12 +1832,15 @@ interface CursorPromise<T> extends Promise<T> {
next(): CursorPromise<T>;
}

type AccessToken = string | ShareDescriptor;
type AccessToken = string;
type AccessTokenOption =
| AccessToken // TODO: Deprecate?
| (() => AccessToken) // TODO: Deprecate
| (() => Promise<AccessToken>);

type ShareIdentifier = string | ShareDescriptor;
type ShareIdOption = () => Promise<ShareIdentifier>;

type EndpointMetric = {
duration: number,
endpoint: string,
Expand All @@ -1848,11 +1851,12 @@ type EndpointMetric = {
type AnalyticsCallback = (metric: EndpointMetric) => any

type CommandOptions = {
accessToken: AccessTokenOption,
accessToken?: AccessTokenOption,
apiUrl: string | Promise<string>,
analyticsCallback: AnalyticsCallback,
assetUrl: string | Promise<string>,
previewUrl: string | Promise<string>,
shareId?: ShareIdOption,
transportMode: ("api" | "cli")[],
webUrl: string | Promise<string>
};
Expand Down
22 changes: 16 additions & 6 deletions src/endpoints/Endpoint.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,19 +259,29 @@ export default class Endpoint {
}

async _getFetchHeaders(customHeaders?: { [key: string]: string }) {
const token = await this._getAccessToken();
const tokenHeader =
typeof token === "string"
? { Authorization: `Bearer ${token}` }
: { "Abstract-Share-Id": token && inferShareId(token) };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're removing the ability to pass a share ID as an accessToken. Technically this would be a breaking change, but it's not documented for the public and I'm going to update our only usage in ui after a prerelease is cut.

const accessToken = await this._getAccessToken();
const authorizationHeader = accessToken
? { Authorization: `Bearer ${accessToken}` }
: undefined;

const shareId = this.options.shareId
? await this.options.shareId()
: undefined;
const shareIdHeader = shareId
? {
"Abstract-Share-Id":
typeof shareId === "string" ? shareId : inferShareId(shareId)
}
: undefined;

const headers = {
Accept: "application/json",
"Content-Type": "application/json",
"User-Agent": `Abstract SDK ${minorVersion}`,
"X-Amzn-Trace-Id": `Root=1-${new Date().getTime()}-${uuid()}`,
"Abstract-Api-Version": "8",
...tokenHeader,
...authorizationHeader,
...shareIdHeader,
...customHeaders
};

Expand Down
8 changes: 6 additions & 2 deletions src/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,12 +137,15 @@ export type CollectionsListOptions = {
userId?: string
};

export type AccessToken = ?string | ShareDescriptor;
export type AccessToken = ?string;
export type AccessTokenOption =
| AccessToken // TODO: Deprecate?
| (() => AccessToken) // TODO: Deprecate
| (() => Promise<AccessToken>);

export type ShareIdentifier = ?string | ShareDescriptor;
anthony-j-castro marked this conversation as resolved.
Show resolved Hide resolved
export type ShareIdOption = () => Promise<ShareIdentifier>;

export type EndpointMetric = {
duration: number,
endpoint: string,
Expand All @@ -153,11 +156,12 @@ export type EndpointMetric = {
export type AnalyticsCallback = EndpointMetric => mixed;

export type CommandOptions = {
accessToken: AccessTokenOption,
accessToken?: AccessTokenOption,
analyticsCallback: AnalyticsCallback,
apiUrl: string | Promise<string>,
objectUrl: string | Promise<string>,
previewUrl: string | Promise<string>,
shareId?: ShareIdOption,
transportMode: ("api" | "cli")[],
webUrl: string | Promise<string>
};
Expand Down
185 changes: 124 additions & 61 deletions tests/Client.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,77 +103,140 @@ describe("Client", () => {
]);
});

test("shareDescriptor access token - api", async () => {
const fetchSpy = jest.spyOn(global, "fetch");
describe("authentication methods", () => {
const projectDescriptor = { projectId: "project-id" };

describe("api", () => {
const requestOptions = { transportMode: ["api"] };

beforeEach(() => {
mockAPI("/projects/project-id/branches/?", {
data: {
branches: [
{
id: "branch-id"
}
]
}
});
});

const client = new Client({
...CLIENT_CONFIG,
accessToken: { shareId: "share-id" }
});
describe("authenticated", () => {
test("authenticated and shareId: ShareDescriptor", async () => {
const client = new Client({
...CLIENT_CONFIG,
accessToken: async (): Promise<AccessToken> =>
CLIENT_CONFIG.accessToken,
shareId: async () => ({ shareId: "share-id" })
});

const fetchSpy = jest.spyOn(client.branches, "apiRequest");

const response = await client.branches.list(
projectDescriptor,
requestOptions
);

expect(response).toEqual([{ id: "branch-id" }]);

expect(fetchSpy.mock.calls.length).toBe(1);
expect(fetchSpy.mock.calls[0][1].headers["Authorization"]).toBe(
`Bearer ${CLIENT_CONFIG.accessToken}`
);
expect(fetchSpy.mock.calls[0][1].headers["Abstract-Share-Id"]).toBe(
"share-id"
);
});

test("authenticated and shareId: string", async () => {
const client = new Client({
...CLIENT_CONFIG,
shareId: async () => "share-id-string"
});

const fetchSpy = jest.spyOn(client.branches, "apiRequest");

await client.branches.list(projectDescriptor, requestOptions);

expect(fetchSpy.mock.calls[0][1].headers["Authorization"]).toBe(
`Bearer ${CLIENT_CONFIG.accessToken}`
);
expect(fetchSpy.mock.calls[0][1].headers["Abstract-Share-Id"]).toBe(
"share-id-string"
);
});
});

mockAPI("/projects/project-id/branches/?", {
data: {
branches: [
{
id: "branch-id"
}
]
}
test("public access (not authenticated) and shareId: ShareDescriptor", async () => {
const { accessToken, ...options } = CLIENT_CONFIG;
const client = new Client({
...options,
shareId: async () => ({ shareId: "only-share-id" })
});

const fetchSpy = jest.spyOn(client.branches, "apiRequest");

await client.branches.list(projectDescriptor, requestOptions);

expect(fetchSpy.mock.calls[0][1].headers).not.toHaveProperty(
"Authorization"
);
expect(fetchSpy.mock.calls[0][1].headers["Abstract-Share-Id"]).toBe(
"only-share-id"
);
});
});

const response = await client.branches.list(
{
projectId: "project-id"
},
{
transportMode: ["api"]
}
);
describe("cli", () => {
let spawnSpy;
const requestOptions = { transportMode: ["cli"] };

beforeEach(() => {
spawnSpy = mockCLI(["branches", "list", "--project-id=project-id"], {
branches: [
{
id: "branch-id"
}
]
});
});

expect(response).toEqual([
{
id: "branch-id"
}
]);
test("authenticated and shareId: ShareDescriptor", async () => {
const client = new Client({
...CLIENT_CONFIG,
shareId: async () => ({ shareId: "share-id" })
});

expect(fetchSpy.mock.calls.length).toBe(1);
expect(fetchSpy.mock.calls[0][1].headers["Abstract-Share-Id"]).toBe(
"share-id"
);
});
const response = await client.branches.list(
projectDescriptor,
requestOptions
);

test("shareDescriptor access token - cli", async () => {
const client = new Client({
...CLIENT_CONFIG,
accessToken: (() => ({ shareId: "share-id" }): () => AccessToken)
});
expect(response).toEqual([
{
id: "branch-id"
}
]);
expect((spawnSpy: any).mock.calls.length).toBe(1);
expect((spawnSpy: any).mock.calls[0][1].includes("--user-token")).toBe(
true
);
});

const spawnSpy = mockCLI(["branches", "list", "--project-id=project-id"], {
branches: [
{
id: "branch-id"
}
]
});
test("not authenticated and shareId: ShareDescriptor", async () => {
const { accessToken, ...options } = CLIENT_CONFIG;
const client = new Client({
...options,
shareId: async () => ({ shareId: "share-id" })
});

const response = await client.branches.list(
{
projectId: "project-id"
},
{
transportMode: ["cli"]
}
);
await client.branches.list(projectDescriptor, requestOptions);

expect(response).toEqual([
{
id: "branch-id"
}
]);
expect((spawnSpy: any).mock.calls.length).toBe(1);
expect((spawnSpy: any).mock.calls[0][1].includes("--user-token")).toBe(
false
);
expect((spawnSpy: any).mock.calls[0][1].includes("--user-token")).toBe(
false
);
});
});
});

test("undefined request options", async () => {
Expand Down