Skip to content

Conversation

Elgarni
Copy link

@Elgarni Elgarni commented Oct 11, 2019

Description

This PR is a follow-up on #1669, that when merged should fix permissions issues such as #1668.

Checks were added prior to initializing a feature, such that if the user lacks a permission, they are told so, and the initialization is aborted.

Scenarios Tested

Scenario: The user tries to initialize a firebase feature, with a Viewer role on an existing project called example-project

Before this PR, the output will be similar to this issue #1668.

After this PR, the output on each feature will be as follows:

  • init Database
=== Database Setup

Error: Authorization failed. This account is missing the following required permissions on project example-project:

  firebasedatabase.instances.update
  • init Firestore
=== Firestore Setup

Error: Authorization failed. This account is missing the following required permissions on project example-project:

  datastore.indexes.create
  datastore.indexes.delete
  datastore.indexes.update
  • init Functions
=== Functions Setup

Error: Authorization failed. This account is missing the following required permissions on project example-project:

  cloudfunctions.functions.create
  cloudfunctions.functions.delete
  cloudfunctions.functions.update
  • init Hosting
=== Hosting Setup

Error: Authorization failed. This account is missing the following required permissions on project example-project:

  firebasehosting.sites.update
  • init Storage
=== Storage Setup

Error: Authorization failed. This account is missing the following required permissions on project example-project:

  firebaserules.releases.create
  firebaserules.releases.update
  firebaserules.rulesets.create

Let's discuss if there are potential issues with these new checks.

The rejected promise is resulted from lack of permissions to enable required google apis
Now if the initialization is on an existing project, the user's permissions will be checked, so that the user does not end up in a state where they can not deploy the project
* master:
  change out tslint for eslint, new publish config (firebase#486)
  Remove scripts/package.json and update firebase version. (firebase#1704)
  Fix functions deploy integration test script (firebase#1703)
  Loop the modules properties without prototype methods. Fixes firebase#1687 (firebase#1694)
  Allow customers to configure the db setting 'strictTriggerValidation' (firebase#1702)
  [firebase-release] Removed change log and reset repo after 7.5.0 release
  7.5.0
  Fix export users (firebase#1690)
  Fix port issues in WSL (firebase#1699)
  Unremove but deprecate separate port for WebChannel. (firebase#1698)
  Release Firestore Emulator 1.9.0 and remove WebChannel workaround. (firebase#1689)
  update handlebars dependency (firebase#1686)
  [firebase-release] Removed change log and reset repo after 7.4.0 release
  7.4.0
  remove items from previews list (firebase#488)
* master:
  Remove unnecessary log lines (firebase#1709)
  Integration with firebase-functions 3.3.0 (firebase#1714)
  javascript lint cleanup x3 (firebase#1710)
  update lint to not fix warnings by default (firebase#1715)
  Clarify warning for unknown network requests (firebase#1711)
  Use different method for setting Firepit title (firebase#1712)

# Conflicts:
#	CHANGELOG.md
@googlebot googlebot added the cla: yes Manual indication that this has passed CLA. label Oct 11, 2019
@coveralls
Copy link

coveralls commented Oct 11, 2019

Coverage Status

Coverage decreased (-0.04%) to 65.177% when pulling 1b20ff2 on Elgarni:check-permissions-on-feature-init into c9019ff on firebase:master.

* master:
  Un-escape new line characters from --releaseNotes arg (firebase#1739)
  update firebase-admin dev dependency (firebase#1741)
  Reject rounds=0 for SHA1 hashes (firebase#1701)
  [firebase-release] Removed change log and reset repo after 7.6.1 release
  7.6.1
  Add Firepit to cloudbuild.yaml (firebase#1637)
  lint rules cleanup x4 (firebase#1721)
  Update template dependencies (firebase#1726)
  Send correct update mask during ext:update (firebase#1724)
  add new ts rules to eslintrc (firebase#1727)
  [firebase-release] Removed change log and reset repo after 7.6.0 release
  7.6.0
  add dev tsconfig for scripts directory (firebase#1720)
  Warn when WSL users don't have Java (firebase#1717)

# Conflicts:
#	CHANGELOG.md
@Elgarni Elgarni force-pushed the check-permissions-on-feature-init branch from bc95174 to 1b20ff2 Compare October 26, 2019 02:03
@Elgarni
Copy link
Author

Elgarni commented Oct 26, 2019

@bkendall Any update on this?

@bkendall
Copy link
Contributor

Yes, I have an update.

Thanks for the PR and all the work getting to this point. However, after talking it over with a couple folks, this is not the behavior we want init to have. While a user may not have permissions to do something in a project, we don't want to gate init of any feature on that as a requirement. When the users takes an action that requires those permissions, we want to make sure we provide a useful error message at that time to help them get unblocked and moving forward.

Thank you for the effort, but we won't be moving forward with this PR.

@bkendall bkendall closed this Nov 21, 2019
@Elgarni
Copy link
Author

Elgarni commented Nov 22, 2019

Sure no worries.

Just gave the CLI a try with your PR#1790 changes, and had the same output as in this PR. It's a good thing that we agree on what to show to the user in this case :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Manual indication that this has passed CLA.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants