Skip to content
This repository was archived by the owner on Jul 7, 2025. It is now read-only.

Add permissions input#37

Closed
temsa wants to merge 2 commits intotibdex:mainfrom
nearform:add-permissions
Closed

Add permissions input#37
temsa wants to merge 2 commits intotibdex:mainfrom
nearform:add-permissions

Conversation

@temsa
Copy link
Copy Markdown

@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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
owner,
privateKey,
repo,
permissions: JSON.parse(permissionsJSON)
Copy link
Copy Markdown

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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Owner

tibdex commented Jul 9, 2022

@temsa
Copy link
Copy Markdown
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
Copy Markdown

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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants