Add identity common items for new functionality #1027
Conversation
colerogers
changed the title
| it('should return false on a bad project', () => { | ||
| expect( | ||
| identity.isAuthorizedCloudFunctionURL( | ||
| `https://us-central1-${PROJECT}-old.cloudfunctions.net/function-1`, |
There was a problem hiding this comment.
What's wrong with this? Because the project and URL don't match?
There was a problem hiding this comment.
Yes, we expect the url to be in the form https://${location}-${project}.cloudfunctions.net/..., but the given url does not match with the given project name, so we should return false.
| const rawUserInfo = { | ||
| name: 'John Doe', | ||
| granted_scopes: | ||
| 'openid https://www.googleapis.com/auth/userinfo.email https://www.googleapis.com/auth/userinfo.profile', |
There was a problem hiding this comment.
What? This isn't a list? weird...
| authType: 'USER', | ||
| resource: { | ||
| service: 'identitytoolkit.googleapis.com', | ||
| name: 'projects/project-id/tenants/TENANT_ID', |
There was a problem hiding this comment.
Should we verify that GCIP is always going to have one handler for all of auth and not have a handler per tenant? I would assume that the latter would make sense, but they know their product better than us obv.
There was a problem hiding this comment.
According to their public docs, functions are registered at the project level (one handler per project). If the customer wishes to filter on users registering/logging in under a specific tenant, they can use the user.tenantId field inside the function.
src/common/providers/identity.ts
Outdated
| * Verifies the jwt using the 'jwt' library and decodes the token with the public keys | ||
| * Throws an error if the event types do not match | ||
| */ | ||
| function verifyAndDecodeJWT( |
There was a problem hiding this comment.
OOC, how do you plan to loosen the verification requirements for the emulator in the future?
There was a problem hiding this comment.
I was planning on checking for an environment variable, but until that decision gets made, I added in a function shouldVerifyJWT() to determine if we need to verify the JWT. Right now, it will always return true.
src/common/providers/identity.ts
Outdated
| return async (req: express.Request, res: express.Response): Promise<void> => { | ||
| try { | ||
| const projectId = process.env.GCLOUD_PROJECT; | ||
| const publicKeys = await fetchPublicKeys(); |
There was a problem hiding this comment.
IIRC these keys rotate very infrequently. Can we keep a cache and compare against a key (e.g. exp) to know whether we need to refresh the cache? We might also need some retry logic if the key rotated early. It would be nice to cut a request out of the amortized cost of a function when users are interactively waiting for completion
There was a problem hiding this comment.
Here's how admin SDK solves this problem:
There was a problem hiding this comment.
Added a key cache by performing a similar technique that the admin SDK uses except without using a Class.
In the cache-control header, we can parse for a max-age property in order to figure out when the keys needs to be refreshed.
I also added in a single retry to verification that will force refresh the keys.
src/common/providers/identity.ts
Outdated
| return async (req: express.Request, res: express.Response): Promise<void> => { | ||
| try { | ||
| const projectId = process.env.GCLOUD_PROJECT; | ||
| const publicKeys = await fetchPublicKeys(); |
There was a problem hiding this comment.
Here's how admin SDK solves this problem:
src/common/providers/identity.ts
Outdated
| const userRecord = parseUserRecord(decodedJWT.user_record); | ||
| const authEventContext = parseAuthEventContext(decodedJWT, projectId); | ||
| const authResponse = | ||
| (await handler(userRecord, authEventContext)) || undefined; |
There was a problem hiding this comment.
nit* I'm not sure what the difference between (await handler(userRecord, authEventContext)) vs (await handler(userRecord, authEventContext)) || undefined is. Are we trying to make sure any falsy values are re-assigned to undefined?
There was a problem hiding this comment.
The handler can have a return type of void and we can't use void in boolean checks (like in getUpdateMask) so assigning it to undefined seemed like the right approach here. Curious if you have a better approach?
…-new-common-items
| ) { | ||
| return async (req: express.Request, res: express.Response): Promise<void> => { | ||
| try { | ||
| const projectId = process.env.GCLOUD_PROJECT; |
There was a problem hiding this comment.
I'm a little worried that projectId can be 2 things across GCP:
human reable projectId = 'danielylee-gcp-project'
numeric project number = '1928371928'
Our API guidelines suggests that all Google APIs support both as project identifier interchangeably.
IIUC, GCLOUD_PROJECT env var is set by our CLI and will most likely be the human-readable one. Is there a guideline from the auth team that they will also use this type always?
There was a problem hiding this comment.
The identity platform request that's sent uses the human readable project id. I'll have to check with them if they plan on changing that.
src/common/providers/identity.ts
Outdated
| return async (req: express.Request, res: express.Response): Promise<void> => { | ||
| try { | ||
| const projectId = process.env.GCLOUD_PROJECT; | ||
| validRequest(req); |
There was a problem hiding this comment.
nit* validateRequest to make it consistent w/ other function name like validateAuthResponse.
One overall sense I got reading the code is that I wasn't sure if a function call like this would throw an error or not. I think one way to improve this issue would've been to consolidate logic for throwing HTTP error within this function, e.g.
if (!isValidRequest(req)) {
logger.error('Invalid request, unable to process: ' + e.message);
throw new HttpsError('invalid-argument', 'Bad Request');
}Not meant to be a comment that blocks PR - just a point I wanted to discuss as team.
There was a problem hiding this comment.
That's a fair point, I re-wrote it to match what we do in common/https
|
Please merge this PR. |
|
What is the plan for this PR? |
|
@inlined plan was to wait for the Admin SDK changes, merge them in here, and complete the PR. Since this is taking much longer than anticipated, do you recommend closing this and re-opening a PR later? |
4 participants
Adds most of the common code pieces for auth blocking functions. A lot of this functionality is re-implemented and refactored from the gcip blocking function library.
Since this is a pretty large change, I can try and split this into separate PRs if needed.