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

Support GitHub Personal Access Tokens #1393

Merged

Conversation

tinchodias
Copy link
Collaborator

Intends to fix #1392. Should I write something about this new credentials? where?

Following, the steps to test this PR in a Pharo 9 (there is another issue open for Pharo 8).

First, add a token to the image:

  1. In your GitHub account, create a Personal Access Token (Guide: https://docs.github.com/en/free-pro-team@latest/github/authenticating-to-github/creating-a-personal-access-token)
  2. In Pharo, open Iceberg -> Settings -> Edit credentials -> +Token, as shown in the screenshot below.

Lastly, you can try the token with this adhoc test in a Workspace:

"https://docs.github.com/en/free-pro-team@latest/rest/reference/rate-limit"

rateLimitWithAuthentication := ((IceGitHubAPI new get: 'rate_limit') at: 'rate') at: 'limit'. 
rateLimitWithoutAuthentication := ((IceGitHubAPI new beAnonymous get: 'rate_limit') at: 'rate') at: 'limit'.

self assert: rateLimitWithAuthentication = 5000.
self assert: rateLimitWithoutAuthentication = 60.

Maybe we can add create a testing token in some "official account" (e.g. pharo-vcs) and use it from a CI test...

Screenshot:

Screen Shot 2020-11-06 at 12 46 40

@guillep
Copy link
Member

guillep commented Nov 7, 2020

Should I write something about this new credentials? where?

The wiki seems the proper place!

Also, should we remove/forbid password authentication to github all the way?
If this is going to be removed on github, probably best is to remove it on our side to to avoid mistakes.
Opinions?

Thanks Martin!!

@astares
Copy link
Contributor

astares commented Nov 7, 2020

We should include also the magic expression to set the token via script. Some people script their IDE using startup.st file, Launcher init scripts or other

@tinchodias
Copy link
Collaborator Author

@astares Good point. I will explain that too in the wiki. BTW, it's:

IceCredentialStore current
	storeCredential: (IceTokenCredentials new
		username: '[email protected]';
		token: '4a01...e072';
		yourself) 
	forHostname: 'github.com'.

@tinchodias
Copy link
Collaborator Author

@guillep

IIUC:
The old user/pass credentials will be still perfectly valid for authenticating git operations via https in GH.

What stops soon is the access to the API for both: v3 (REST) and v4 (GraphQL).

BUT when you have the Token, they allow you to use the Token as a password for HTTPS git operations. Then, we can recommend to leave using old user/password credentials for GH but it's not mandatory.

The entry in the wiki should explain all this...

@tinchodias
Copy link
Collaborator Author

There is another point to discuss, too:

What is the default dialog/modal window that pops up when the authentication failed. Even in the branch 1.9, that's not touched... if you open a fresh image, do an operation that requires credentials, then the user/password is what pops up and it would be nice that the user can choose what kind of credential to add. I think we can do it for P9. Like it is in my PR, when this happens, the user has to close the dialog window, add the token in settings, and then re-try the operation (which should work without asking again).

But note this is only for GH. I didn't see a deprecation message from other providers such as GitLab or BitBucket.

@tinchodias
Copy link
Collaborator Author

@guillep @tesonep I'm reading the Github docs about wiki and see they are versioned by separate... what is the docs/ directory? (my first time with GH's wikis)

@guillep
Copy link
Member

guillep commented Nov 10, 2020

Ah, the github's wiki and the docs/ directory are synchronized using travis!
We were at some point tired of having the docs in two different places, because people couldn't contribute to it.

The idea is that when travis runs, there is a sync-wiki job (See https://github.com/pharo-vcs/iceberg/blob/dev-1.8/.travis.yml)

- if [[ ${JOB} == "sync-wiki" ]] && [[ ${TRAVIS_BRANCH} == "master" ]] && [[ ${TRAVIS_PULL_REQUEST} == "false" ]]; then ./scripts/sync-wiki.sh; fi

That job is only run on the master branch and when it is not a pull request.

That will call the this script that takes care of pushing all the changes from the docs directory to the wiki.

I see two problems with this now:

  • we seem to not be updating the master branch anymore. Maybe we should :)?
  • the master branch is not the default branch anymore, meaning that people will not do their PRs to it.

ideas?

@tinchodias
Copy link
Collaborator Author

Thanks foor explaining, I see...

As Github repos have the concept of "default branch", I searched in travis environment variables but didn't find something like ${TRAVIS_BRANCH_IS_DEFAULT} or ${TRAVIS_BRANCH} == ${REPO_DEFAULT_BRANCH}. I didn't search more... But for the moment it's enough to replace "master" by "dev-1.8". I can do it.

@tinchodias
Copy link
Collaborator Author

Done in branch dev-1.8: commit 71e12a0

@tinchodias
Copy link
Collaborator Author

@guillep something went wrong with that script:

remote: Invalid username or password.
fatal: Authentication failed for 'https://github.com/pharo-vcs/iceberg.wiki.git/'

Source: https://travis-ci.com/github/pharo-vcs/iceberg/jobs/439107332

@StephanEggermont
Copy link
Contributor

Do we want to have the token in the image instead of the location of the token?

@tinchodias
Copy link
Collaborator Author

@StephanEggermont good point. Right now, the tokens are stored exactly as it was done with the passwords. We should take the no-memory approach in both. Do you know if there is an issue created for this?

@tinchodias
Copy link
Collaborator Author

I mean, we can merge it like now (GH API doesn't work with password currently), and implement what you propose in a posterior version.

@StephanEggermont
Copy link
Contributor

I don't see an issue for this. And yes, I prefer being able to share images without leaking tokens. Also without leaking absolute paths

@StephanEggermont
Copy link
Contributor

I mean, we can merge it like now (GH API doesn't work with password currently), and implement what you propose in a posterior version.

Definitely.

@tesonep tesonep merged commit 02da533 into dev-1.8 Nov 24, 2020
@tinchodias tinchodias deleted the 1391-Support-GitHub-Personal-Access-Tokens-in-Pharo-9 branch January 29, 2021 15:42
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.

Support GitHub Personal Access Tokens in Pharo 8
5 participants