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

Add permissions input #37

Closed
wants to merge 2 commits into from
Closed

Add permissions input #37

wants to merge 2 commits into from

Conversation

temsa
Copy link

@temsa temsa commented Apr 29, 2022

This PR allows to provide a permissions input string containing a JSON object providing the permissions as described by https://docs.github.com/en/rest/apps/apps#create-an-installation-access-token-for-an-app

I have tested it and it just works

Comment on lines +30 to +31
# permissions: >-
# {"members": "read"}
Copy link

Choose a reason for hiding this comment

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

would it be possible/reasonable to have this as yaml which it then turned into json before sending it to the GH api? it would be more consistent with the representation we use in the workflow files

Copy link
Author

Choose a reason for hiding this comment

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

@simoneb I may have missed something, but AFAICT the input values should always be a string.

The action.yml documentation is a bit vague, yet assumes the values can be converted to environment variables:

Optional Input parameters allow you to specify data that the action expects to use during runtime. GitHub stores input parameters as environment variables. Input ids with uppercase letters are converted to lowercase during runtime. We recommended using lowercase input ids.

Also, the default input value of the first example provided by this doc, despite being a number, is quoted as a string:

inputs:
  numOctocats:
    description: 'Number of Octocats'
    required: false
    default: '1'
  octocatEyeColor:
    description: 'Eye color of the Octocats'
    required: true

Also, actions on the marketplace that need a more complex structure are often referring a file path (e.g to a JSON or YAML ) instead

Copy link

Choose a reason for hiding this comment

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

fair enough

@temsa
Copy link
Author

temsa commented May 3, 2022

@tibdex does this PR has a chance to get merged at some point (after your feedback and associated fixes, if any, of course )?

src/index.ts Outdated
@@ -25,6 +27,7 @@ const run = async () => {
owner,
privateKey,
repo,
permissions: JSON.parse(permissionsJSON)
Copy link

Choose a reason for hiding this comment

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

Since this input is not required, what happens if it is not specified?

Copy link

Choose a reason for hiding this comment

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

fair point, without tests it's hard to say but I suspect this may break. maybe the default value of this input should be something that parses to null or another sensible default

Copy link
Author

Choose a reason for hiding this comment

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

Fair point:

permissionsJSON comes from getInput.

If the value is not provided, then permissionsJSON is the empty string according to the doc, and unfortunately JSON.parse('') throws. So it needs a small update.

Copy link
Author

Choose a reason for hiding this comment

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

This case should now properly be handled :)

@simoneb
Copy link

simoneb commented Jul 5, 2022

@tibdex we've had this PR open for some time now and we're using a fork of this action. Would you mind taking a look?

@tibdex
Copy link
Owner

tibdex commented Jul 9, 2022

@temsa
Copy link
Author

temsa commented Jul 11, 2022

Can you please allow me to edit your branch as explained in section 7. of https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request-from-a-fork?

It seems I can't allow this (I don't have the checkbox when editing the PR), because of some security setting maybe, as it could allow you to access our corporate secrets if I understand that part of the doc you mentioned correctly:

Warning: If your fork contains GitHub Actions workflows, the option is Allow edits and access to secrets by maintainers. Allowing edits on a fork's branch that contains GitHub Actions workflows also allows a maintainer to edit the forked repository's workflows, which can potentially reveal values of secrets and grant access to other branches.

@simoneb
Copy link

simoneb commented Jul 11, 2022

I believe the culprit is "On user-owned forks":

On user-owned forks, if you want to allow anyone with push access to the upstream repository to make changes to your pull request, select Allow edits from maintainers.

While this is a fork from an org instead.

@tibdex feel free to just create a new branch in this repo so you have freedom to change anything.

@tibdex tibdex mentioned this pull request Jul 12, 2022
@tibdex tibdex closed this in #40 Jul 12, 2022
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.

4 participants