Skip to content

Set Access Control for HTTP Functions (v2) #935

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 7 commits into from
Aug 11, 2021
Merged

Set Access Control for HTTP Functions (v2) #935

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
601061a
adding invoker to v2 http functions
colerogers Aug 10, 2021
ffe16e5
fix format
colerogers Aug 10, 2021
734d6f0
change invoker to always turn into an array, add error handling
colerogers Aug 11, 2021
c4fa3b4
fix trigger annotated in v2 and change v1 to only put invoker array i…
colerogers Aug 11, 2021
4252a52
changing invoker type for better type completion
colerogers Aug 11, 2021
07f3c1d
Merge remote-tracking branch 'origin/master' into colerogers.http-acc…
colerogers Aug 11, 2021
97358a8
Merge remote-tracking branch 'origin/master' into colerogers.http-acc…
colerogers Aug 11, 2021
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions spec/v2/providers/https.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ describe('onRequest', () => {
allowInsecure: false,
},
labels: {},
invoker: 'public',
});
});

Expand All @@ -90,6 +91,7 @@ describe('onRequest', () => {
{
...FULL_OPTIONS,
region: ['us-west1', 'us-central1'],
invoker: ['service-account1', 'service-account2'],
},
(req, res) => {
res.send(200);
Expand All @@ -101,6 +103,7 @@ describe('onRequest', () => {
allowInsecure: false,
},
regions: ['us-west1', 'us-central1'],
invoker: ['service-account1', 'service-account2'],
});
});

Expand All @@ -115,6 +118,7 @@ describe('onRequest', () => {
{
region: ['us-west1', 'us-central1'],
minInstances: 3,
invoker: 'private',
},
(req, res) => {
res.send(200);
Expand All @@ -131,6 +135,7 @@ describe('onRequest', () => {
minInstances: 3,
regions: ['us-west1', 'us-central1'],
labels: {},
invoker: 'private',
});
});

Expand Down
1 change: 1 addition & 0 deletions src/v2/core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ export interface TriggerAnnotation {
vpcConnectorEgressSettings?: string;
serviceAccountEmail?: string;
ingressSettings?: string;
invoker?: string | string[];

// TODO: schedule
}
Expand Down
8 changes: 8 additions & 0 deletions src/v2/providers/https.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,18 @@ export type CallableRequest<T = any> = common.CallableRequest<T>;
export type FunctionsErrorCode = common.FunctionsErrorCode;
export type HttpsError = common.HttpsError;

/**
* Invoker access control type for https functions.
*/
export type Invoker = 'public' | 'private' | string;

export interface HttpsOptions extends Omit<options.GlobalOptions, 'region'> {
region?:
| options.SupportedRegion
| string
| Array<options.SupportedRegion | string>;
cors?: string | boolean;
invoker?: Invoker | Invoker[];
Copy link
Member

Choose a reason for hiding this comment

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

@mbleigh Should we make this an HTTPS only option or should we make it also an event trigger option and set the service account of EventArc accordingly? I guess maybe the former since we don't have a clear answer what to do if the user passes two SAs.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think let's leave this HTTPS-only for now.
}

export type HttpsFunction = ((
Expand Down Expand Up @@ -81,6 +87,7 @@ export function onRequest(
opts = optsOrHandler as HttpsOptions;
}

const invoker = 'invoker' in opts ? opts.invoker : 'public';
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume we validate these values at deploy time, but what happens when users specify a bad/valid-but-wtf values, e.g.

// Bad settings invoker: ['public', 'private'] invoker: ['private', 'service-account@'] // Valid, but wtf? invoker: ['public', 'service-account@']

Is having the following signature more appropriate?

invoker?: public | private | string[]
Copy link
Member

@inlined inlined Aug 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think that's a fine TS signature (if you also allow string) though the CLI should handle the valid-but-wtf case
Copy link
Contributor Author

@colerogers colerogers Aug 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think that all 3 of these cases should be treated as bad and should throw an error. The way I read the type definition is that the user can set 'public', 'private', '$service-account', or [ '$service-account-1', '$service-account-2', ... ]. They should not mix public or private with the array of service accounts.

I've added a helper method to src/common/encoding that will error out of the user sets public or private in the array.
if ('cors' in opts) {
const userProvidedHandler = handler;
handler = (req: Request, res: express.Response) => {
Expand Down Expand Up @@ -113,6 +120,7 @@ export function onRequest(
httpsTrigger: {
allowInsecure: false,
},
invoker,
};
},
});
Expand Down