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 built-in support for basic auth #144

Closed
wants to merge 2 commits into from
Closed

Conversation

danra
Copy link

@danra danra commented Jun 19, 2024

This allows using the encoded token for basic auth-based git access.

This is useful for scripts expecting to be able to access private git repos. My specific case was using CPM.cmake in a repo that brings in other private repos as dependencies.

While the encoding to make this work is very simple, the documentation for it is sparse and it is not trivial to figure out, so there's value in providing this out of the box.

@danra danra requested review from gr2m, parkerbxyz and a team as code owners June 19, 2024 03:58
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

I would not add this feature to this action, but we can document the workflow in the README.

It's similar to this pull request: #137 which we closed in favor of #142

I would suggest to add a step which pipes the string x-access-token:${{ steps.app-token.outputs.token }} | bash64 and writes that to a new output as in did in #142

In future, can you please create an issue first before a pull request? We appreciate the effort you put into it, but having a discussion first might help prevent unnecessary work and frustration.

Comment on lines +105 to +110
ref: ${{ github.head_ref }}
# Access allowed to private submodules in the current owner's installation
submodules: recursive
# Make sure the value of GITHUB_TOKEN will not be persisted in repo's config
persist-credentials: false
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove unrelated changes

Suggested change
ref: ${{ github.head_ref }}
# Access allowed to private submodules in the current owner's installation
submodules: recursive
# Make sure the value of GITHUB_TOKEN will not be persisted in repo's config
persist-credentials: false
```

@@ -99,11 +99,37 @@ jobs:
app-id: ${{ vars.APP_ID }}
private-key: ${{ secrets.PRIVATE_KEY }}
owner: ${{ github.repository_owner }}
- uses: peter-evans/create-or-update-comment@v3
- uses: actions/checkout@v4
Copy link
Contributor

Choose a reason for hiding this comment

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

please revert

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- uses: actions/checkout@v4
- uses: peter-evans/create-or-update-comment@v4

@danra
Copy link
Author

danra commented Jun 19, 2024

I would suggest to add a step which pipes the string x-access-token:${{ steps.app-token.outputs.token }} | bash64

Unfortunately, that's not cross-platform (doesn't work on Windows).
I used github-script for cross-platform base64 encoding:

- name: Prepare basic auth credentials
  id: basic-auth-creds
  uses: actions/github-script@v7
  with:
    script: return btoa("x-access-token:${{ steps.app-token.outputs.token }}")
    result-encoding: string

- name: Checkout with private submodules
  env:
    GIT_CONFIG_COUNT: 1
    GIT_CONFIG_KEY_0: http.https://github.com/.extraheader
    GIT_CONFIG_VALUE_0: "AUTHORIZATION: basic ${{ steps.basic-auth-creds.outputs.result }}"
  # Access allowed to private submodules in the current owner's installation
  run: git clone --recurse-submodules --shallow-submodules https://github.com/${{ github.repository }}

In future, can you please create an issue first before a pull request? We appreciate the effort you put into it, but having a discussion first might help prevent unnecessary work and frustration.

Sure, no worries. I'll close the PR. A README addition would be helpful, indeed.

@danra danra closed this Jun 19, 2024
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.

3 participants