If a user provides bad credentials, throw an error. #75
Conversation
There was a problem hiding this comment.
Nice. Why not just raise a descriptive error from https://github.com/jekyll/github-metadata/blob/master/lib/jekyll-github-metadata/client.rb#L65 if the user provides a token? Why not provide if even if they don't (to let them know they need to provide a token).
| @@ -71,13 +72,25 @@ def inspect | |||
| "#<#{self.class.name} @client=#{client_inspect}>" | |||
| end | |||
|
|
|||
| def authenticated? | |||
| !@client.access_token.nil? && !@client.access_token.empty? | |||
There was a problem hiding this comment.
Is this better as to_s.empty??
| # Raise an error if credentials are provided and they cause a 401. | ||
| def check_credentials! | ||
| return unless authenticated? | ||
| @client.user |
There was a problem hiding this comment.
Why make an extra call to check credentials, when we can just rescue if the call we need fails due to bad auth?
There was a problem hiding this comment.
If you are not an owner of jekyll/github-metadata and you fetch the /repos/jekyll/github-metadata/pages endpoint, will you get a 404 or a 401? If 401 always means bad credentials and not "this resource isn't available to you" then I think we can fail more loudly on those calls, too.
| @@ -8,6 +8,9 @@ class GHPMetadataGenerator < Jekyll::Generator | |||
| def generate(site) | |||
| Jekyll::GitHubMetadata.log :debug, "Initializing..." | |||
|
|
|||
| # Fail loudly if credentials are provided but aren't valid. | |||
| Jekyll::GitHubMetadata.client.check_credentials! | |||
There was a problem hiding this comment.
Won't this be a change in behavior? Right now, I can have an invalid credential, but if I don't use any site.github metadata, the drop isn't called, and thus it's okay, right? I don't know that we need to go out of our way to fail a user's build if they're not relying on the plugin for their site to build.
|
@benbalter Ready for another round of review! |
| @@ -15,6 +16,7 @@ class Client | |||
| releases | |||
| list_repos | |||
| organization_public_members | |||
| user | |||
|
@parkr my bad. One question, which I think is left over from the previous iteration. Can this go in a point release, or does it need its own minor release? |
|
@benbalter OK. Ready for another review. |
parkr
force-pushed
the
fail-with-invalid-token
branch
from
8fe4d82
to
5f1679f
Compare
|
@jekyllbot: merge +minor |
This fixes #67. If you give me
ACCESS_TOKEN=1234, you'll receive a loud error when you runjekyll servethat it's invalid. Providing either no token or a valid token will solve the error./cc @jekyll/gh-pages for review.