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

Feature/init command #273

Open
wants to merge 28 commits into
base: develop
Choose a base branch
from
Open

Feature/init command #273

wants to merge 28 commits into from

Conversation

nheeb
Copy link

@nheeb nheeb commented Sep 3, 2024

The init command is now working with Oauth. Zenodo Oauth saves the refresh token in the secret and deposit can now use a refresh token to get an access token.

Copy link
Contributor

@zyzzyxdonta zyzzyxdonta left a comment

Choose a reason for hiding this comment

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

I had a quick first look at this and wrote some comments. :-)

Some more that don't fit in the code:

  • The Zenodo oauth application says "Application 'hermes-init-oauth' by '' wants permission to access your 'd.pape@hzdr.de' account." It would be nice if this was the hermes mailbox address instead of your private one.
  • The same for GitHub which currently says "hermes-init-oauth by nheeb". Maybe the softwarepub group can be used here?
  • During GitHub OAuth, I run into an error. Afterwards the CLI was stuck.
An error occurred during execution of init
ERROR:hermes.cli:An error occurred during execution of init
DEBUG:hermes.cli:Original exception was: Failed to retrieve public key: 404 {"message":"Not Found","documentation_url":"https://docs.github.com/rest/actions/secrets#get-a-repository-public-key","status":"404"}

Unfortunately, I'm clueless about OAuth, so I have no idea what is going wrong.

src/hermes/commands/init/github_permissions.py Outdated Show resolved Hide resolved
src/hermes/commands/init/github_permissions.py Outdated Show resolved Hide resolved
src/hermes/commands/init/github_secrets.py Outdated Show resolved Hide resolved
src/hermes/commands/init/base.py Outdated Show resolved Hide resolved
src/hermes/commands/init/base.py Outdated Show resolved Hide resolved
src/hermes/commands/init/oauth_github.py Outdated Show resolved Hide resolved
src/hermes/commands/init/oauth_zenodo.py Outdated Show resolved Hide resolved
Comment on lines 9 to 10
client_id = 'Ov23ctl0gNzr9smeVIHR'
client_secret = 'd516303374f7e55189fe74fb2af77f31a965ad57'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this supposed to be here?

Copy link
Author

Choose a reason for hiding this comment

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

At least the secrets should be stored somewhere else in the end.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/authorizing-oauth-apps the only way to avoid storing this secret somewhere is by using a different flow, like the device flow.

It might be a good idea to switch to this, as there is a possibility this first idea of using a browser redirect might not work with JupyterHub. Needs testing.

Copy link
Member

@poikilotherm poikilotherm Sep 5, 2024

Choose a reason for hiding this comment

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

On a related note: let's create an organization based OAuth app for this, avoiding a bus factor = 1.

https://github.com/organizations/softwarepub/settings/applications

In the same go, we should probably also create an app on Helmholtz Codebase (https://codebase.helmholtz.cloud/groups/softwarepub/-/settings/applications)

Comment on lines 14 to 17
sandbox_client_id = 'QJ8Q9GBI78uOdNmVNK1Vd0oAOJHqmYGvxRxiSFxt'
sandbox_client_secret = 'nGuOqoDtd2tckP6lmQS3If3cY39lPLKLU8skcv72JeowNupMD2bnLparsGO9'
real_client_id = 'L0d9HQVW4Ig9PnC6qh6zkOAwgvYy08GcmHJqVVvV'
real_client_secret = '0HIvtC2D2aPvpq2W0GtfWdeivwkqvnvrOTGx14nUJA5lDXrEDSaQAnqxHbLH'
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this supposed to be here?

Copy link
Member

Choose a reason for hiding this comment

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

It should be possible to use a public client with Zenodo, at least you can create a client of type "public"

grafik

Copy link
Member

@poikilotherm poikilotherm Sep 5, 2024

Choose a reason for hiding this comment

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

Also: we should probably create a shared softwarepub account to create these clients, avoiding a bus factor = 1.

(I created an account on Zenodo + Sandbox explicitely for Oauth, happy to share)

pyproject.toml Outdated Show resolved Hide resolved
@zyzzyxdonta
Copy link
Contributor

Oh I see. I got a 404 because I gave a random git remote URI that doesn't exist but the code actually accesses my repo. 🤦🏻‍♂️

nheeb and others added 4 commits September 4, 2024 12:39
Co-authored-by: David Pape <d.pape@hzdr.de>
Co-authored-by: David Pape <d.pape@hzdr.de>
Co-authored-by: David Pape <d.pape@hzdr.de>
Co-authored-by: David Pape <d.pape@hzdr.de>
Copy link
Member

@poikilotherm poikilotherm left a comment

Choose a reason for hiding this comment

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

Overall this is a very good start. I agree with all of the points from @zyzzyxdonta and added some more 🙈

@nheeb I also noticed flake8 is unhappy. You can execute it locally using poetry run task flake8 with our configuration.

src/hermes/commands/init/__init__.py Outdated Show resolved Hide resolved
src/hermes/commands/init/base.py Outdated Show resolved Hide resolved
src/hermes/commands/init/base.py Outdated Show resolved Hide resolved
src/hermes/commands/init/github_permissions.py Outdated Show resolved Hide resolved
src/hermes/commands/init/github_secrets.py Outdated Show resolved Hide resolved
Comment on lines 9 to 10
client_id = 'Ov23ctl0gNzr9smeVIHR'
client_secret = 'd516303374f7e55189fe74fb2af77f31a965ad57'
Copy link
Member

@poikilotherm poikilotherm Sep 5, 2024

Choose a reason for hiding this comment

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

On a related note: let's create an organization based OAuth app for this, avoiding a bus factor = 1.

https://github.com/organizations/softwarepub/settings/applications

In the same go, we should probably also create an app on Helmholtz Codebase (https://codebase.helmholtz.cloud/groups/softwarepub/-/settings/applications)

src/hermes/commands/init/oauth_zenodo.py Outdated Show resolved Hide resolved
# 3. Check if you want to run with '--initial', as this may potentially create a completely new record (collection),
# rather than a new version of the same collection!
- run: hermes deposit --initial -O invenio_rdm.auth_token ${{ secrets.ZENODO_SANDBOX }} --file showcase.zip --file README.md
- if: ${{ env.ZENODO_TOKEN_REFRESH != '' }}
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure this will not work. Within GH Actions you need to use sth like echo "key=value" >> "${GITHUB_ENV}" to actually get the desired var out ouf one step into the runner env. I suggest using files to cache this.

src/hermes/commands/init/github_permissions.py Outdated Show resolved Hide resolved
src/hermes/commands/init/oauth_zenodo.py Outdated Show resolved Hide resolved
if line.startswith("*"):
info.current_branch = line.split()[1].strip()
break
info.uses_github = "github" in info.git_remote_url
Copy link
Contributor

Choose a reason for hiding this comment

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

info.git_remote_url.startswith("https://github.com") would be safest (or use urlparse and check netloc == "github.com").

src/hermes/commands/init/slim_click.py Outdated Show resolved Hide resolved
src/hermes/commands/init/slim_click.py Outdated Show resolved Hide resolved
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