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.

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

Ensure callable functions have invoker correctly set to "public" #4335

Merged
merged 12 commits into from
Mar 19, 2022
Merged

Ensure callable functions have invoker correctly set to "public" #4335

merged 12 commits into from
Mar 19, 2022

Conversation

chrisbrown-io
Copy link
Contributor

Description

Implemented fix for issue introduced in v10.3.0 where callable functions were not having their invoker property correctly set to "public" that would in turn cause access issues with the deployed functions (as detailed in #4327).

Will also fix issues such as #3965, that are presumably cause by version mismatches/upgrades introducing this issue.

See comment for explicit info.

Fixes issue by adding specific handling for callable functions and always setting the invoker to "public".

Also fixed issue with taskQueueTrigger endpoints calling wrong method/passing wrong args when trying to set invoker.

Scenarios Tested

Thanks to @taeold we have explicit tests for each function type and the expected outcomes for both createV1Function and createV2Function methods.

Sample Commands

@google-cla
Copy link

google-cla bot commented Mar 18, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

@chrisbrown-io
Copy link
Contributor Author

@taeold - you mentioned in your previous comment that the invoker is not updated on update, however the source code/tests for the update methods look as if they would update the invoker if set (and subsequently not update the invoker for callable functions unless additional logic was added).

Are you saying we don't need to handle this case, as users can't specify an invoker for callable functions, meaning this scenario of changing the invoker on update will never happen?

@taeold taeold self-requested a review March 18, 2022 19:45
Copy link
Contributor

@taeold taeold left a comment

Choose a reason for hiding this comment

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

Are you saying we don't need to handle this case, as users can't specify an invoker for callable functions, meaning this scenario of changing the invoker on update will never happen?

Yes. User may have changed invoker for good reasons. I think we should respect that.

Also - can we add an entry to CHANGELOG.md file?

src/deploy/functions/release/fabricator.ts Outdated Show resolved Hide resolved
src/deploy/functions/release/fabricator.ts Outdated Show resolved Hide resolved
@taeold taeold requested a review from colerogers March 18, 2022 19:57
Copy link
Contributor

@taeold taeold left a comment

Choose a reason for hiding this comment

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

🔥 Amazing contribution!

CHANGELOG.md Outdated Show resolved Hide resolved
@taeold taeold enabled auto-merge (squash) March 18, 2022 22:31
@taeold taeold merged commit 58d860d into firebase:master Mar 19, 2022
tohhsinpei pushed a commit that referenced this pull request Mar 23, 2022
Implemented fix for issue introduced in `v10.3.0` where callable functions were not having their `invoker` property correctly set to `"public"` that would in turn cause access issues with the deployed functions (as detailed in #4327).

Will also fix issues such as #3965, that are presumably cause by version mismatches/upgrades introducing this issue.

Fixes issue by adding specific handling for callable functions and always setting the `invoker` to `"public"`.

Also fixed issue with `taskQueueTrigger` endpoints calling wrong method/passing wrong args when trying to set invoker.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants