Diagnose command: Add GitHub OAuth token expiration date information

[Ayesh]

GitHub's new fine-grained tokens have a cumpulsory expiration date, and their classic tokens also support an expiration date.

https://github.blog/changelog/2021-07-26-expiration-options-for-personal-access-tokens/

This improves the composer diagnose command to display the expiration date and time if it is provided by the response headers (via GitHub-Authentication-Token-Expiration).

[Ayesh]

[ktomk]

I like the approach to show additional information of the personal access token if available and gave these changes a try, then played with some more changes of my preference which I'd like to share.

As passing the header value as-is into the users terminal is a no-go in my book, parsing the expiration date is mandatory. This allows to normalize the timezone to UTC which has the benefit to hide some details of the token and furthermore, present it more informative:

The screenshot shows a fine-grained access token which expires in 27 minutes. The text is lowercase and the outer parenthesis has been removed, to make it more plain and when there are no colors it allows to keep the focus on the OK of the message.

Checking github.com oauth access: OK expires in 27 minutes

Next to fine-grained personal access tokens, there are classic ones and then Github's REST API has the scopes in the response headers (X-OAuth-Scopes):

Checking github.com oauth access: OK token (no scopes) never expires

This allows to show the scopes of a classic token, which allows a user to reflect usage with the expiration as a by-catch for running composer diagnose. In the previous screenshot it is a token with no scopes that never expires. Variants are with the expiration and scopes, e.g. a token should expire:

Checking github.com oauth access: OK token (no scopes) expires in 23 days

Or if a classic token has scopes, the number of scopes show up:

Checking github.com oauth access: OK token (6 scopes) never expires

Per my current implementation, relative times are relative to the timestamp of the composer diagnose command ($_SERVER['REQUEST_TIME']) plus one minute so that any expiration calculation is less ambiguous on the minutes level and a bit ahead of time.

For the expiration display I started with the following:

• If there is no expiration header, the access token "never expires".
• If there is an expiration header, but it can not be parsed, access token is of "unknown expiration". This means that effectively no DateTime object could be created from the expiration header value, but there was an expiration header value and this fact is not hidden (secure practice permits us to show it verbatim, perhaps on the debug/-vvv severity).
• If no DateInterval object can be created (despite an expiration DateTime object could), it is the absolute "expiry date ..." in Y-m-d H:i:s P format in the UTC timezone (or if the runtime does not have that timezone, the original zone from the expiration header value is the fall back timezone).
• Relative time display breakpoints:
  1. If there is no minute left, access "expired just now" (if access really expired, composer shows the command to remove the token from the config, GitHub HTTP response code was 401 then IIRC and the expiration does not affect the exit status of composer diagnose).
  2. If there are 90 or less minutes left, access shows "expires in %d minute%s".
  3. If there are 6 or less hours left, access shows "expires in %d hour%s and %d minute%s", this is minute precision for a quarter of a day and could be improved by clamping minutes to multiples of five to be less noisy.
  4. If there are 2 or less days left, access "expires in %d hours", this is 48 hours downwards to 6 hours.
  5. If there are 2 and more days left, access "expires in %d days", this is likely to be improved and then shows for 2 days + 1 minute up to 3 days + 0 minutes an "expires in %d days and %d hour%s" and the number of days only for 3 days + n>0 minutes.
• Still considering to not display the expiration information at all, when there are more than X days left, but I find it hard to find the number for X as a sane default. The lowest default expiration period GitHub web console offers is 7 days (which is a week), perhaps X is seven then.
• "never expires" should be always shown, also if a classic token has scopes should always be visible (I think).
• token (...) is likely to be replaced with classic (...) as this is the actual information, oauth in this context implies a token already.

This has grown quite a bit, but I could not find a good middle-ground, e.g. a more safe handling of the header value already requires to parse it for the date and time data, therefore it requires testing, then setting up a unit test etc. and then you find yourself experimenting already with the output format.

I need to run a couple more tests and can then push my branch so that there is some code to see, the implementation is based on the changes here, but instead of asking for a header in the response object, it describes the whole response object in a seam (which was put under test).

[fredden]

I like the idea of displaying a relative time. Perhaps we can show the absolute timestamp when increased verbosity is requested (-vvv/debug).

For another project where I show a relative time, I calculate how many months, weeks, days, hours, minutes (in turn) and display the first one which rounds to more than one (ie, 1.5 or above), then it would display that number with its word. I only display one unit (so only "3 hours" and not "3 hours and 15 minutes"). Eg, 100 days --> 3 months; 50 days --> 2 months; 40 days --> 6 weeks; 37 days --> 5 weeks; 50 hours --> 2 days; 32.5 hours --> 32 hours. I wonder if this approach would be suitable here. I wonder what the benefit of extra precision would be for end users. See also the suggestion above about showing the real timestamp with "verbose."

I think omitting/hiding the expiration time (when the number of days remaining is very large) seems confusing. It may lead people to believe that the token does not expire, rather than suggesting they may like to check back in (for example) a month if the token expires in "5 weeks." (Yes, it wouldn't say "never expires" and would effectively be the same as without this feature / the current behaviour.)

---

I was going to enquire about what this looks like when the token has already expired, but I've tested this locally now. I'm including a screen-shot of this so others can see that this case is already handled outside of the scope of this pull request.

Checking github.com oauth access: The oauth token for github.com seems invalid, run "composer config --global --unset github-oauth.github.com" to remove it

[Ayesh]

Thank you @fredden and @ktomk for the excellent feedback!

• Not using the GitHub header as-is: I agree, we should try to parse the date format to at least validate that it contains a valid date in the expected format.
• Removing braces around the expiration field: I agree here as well, and this seems to be the pattern with other fields too.
• Brilliant idea on showing the token type and scopes too. I will like up the docs and test the endpoints to see if they are consistently sent for both classic tokens and fine-grained tokens.

I will push with these two changes taken care of.

However, I can't say I'm 100% in with the idea of relative times, and would still prefer to show the exact timestamp after validating it:

• CLI tools tend to yield absolutely data by default, and with the least amount of additional processing as possible. Commands like ls, git, free, etc., although used by several times a day, do not emit more "human readable" data by default. git log timestamps are absolute, file sizes of ls and free are bytes and not rounded to KiB/MiB/GiB/etc by default either. I think we should also follow a similar approach, where our CLI outputs are plumbing and porcelain-friendly.
• Given that composer diagnose is likely called on systems that do not function properly, there is a possibility that we might not have the system time to calculate the token validity duration. System time skew might also cause us to show that the token is valid/invalid, although GitHub eventually rejects it.
• The additional complexity in calculating the "human friendly" relative times, and the potential bikeshed there as we shift the focus.

[Seldaek]

Sounds like good improvements to me overall. I have not read all the details sorry too many walls of text here, but I'll let you two hash it out and trust a good solution will come out of it :D

[Ayesh]

Thank you @Seldaek.

I updated the PR with some of the suggestions above (which indeed went pretty extensively I agree :) ).

The summary of changes are that:

• composer diagnose now inspects the github-authentication-token-expiration header.
• For PATs without expiration date (header not present), it shows OK does not expire
• PATs with expiration date shows OK expires on 2024-08-01 01:30:00 +0700 (with the actual date time of course). The header is validated with DateTime::createFromFormat('Y-m-d h:i:s O', $expiration).
• Failing to parse the date results in the existing OK text.
• There datetime value is not further processed, and does not attempt to make it more human friendly for the reasons I listed above.

Thank you.

[ktomk]

Thanks a lot @fredden and @Ayesh, you mentioned the Law of Triviality, and rightly so, this is
an experiment. In regard to simplicity (sorry if my comment comes in at a bad time, was not able to do this earlier):

1. addcslashes() for console output independent to format.
2. strtotime() should suffice for date parsing. /E: in my code you find as well parseFromString()
3. date() for date formatting (gmdate() for UTC).

Headers:

1. x-oauth-scopes - classic token only ¹, empty string is "no scopes"
2. github-authentication-token-expiration - if not set, token "never expires" ²
3. date - standard message header

^{¹ Didn't find anything similar for fine-grained tokens ² Only classic tokens can "never expire" (Github removes such tokens after one year of inactivity)}

Rule of thumb: If there is the scopes header, it's a classic token.

More notes and references are buried in the commit message and code (tests are the docs).

That just for the moment.

And @fredden, you can find a second interval style active based on your feedback if you're interested (feel free to use my repo for commenting, even prolonging), thanks for the inspiration and feedback also for both of you.

---

Token types / Composer support table

Information only, there is no need to inspect the actual token. This is just note-taking for the patterns I've seen to document the work:

Type	Pattern / Lengths	Comment
historic	[40:hexit:]	very old, Composer 1.0.0-alpha6
classic	ghp_[36:tokit:]	birth of the prefix, Composer 1.10.21 / 2.0.12 / 9757, Mar '21
fine‑grained	github_pat_[22:tokit:]_[59:tokit:]	beta, Composer 2.4.4 / 11137, Oct '22

[:hexit:]:= [a-f0-9] ;[:tokit:]:= [A-Za-z0-9]

More: GitHub's token formats

[Ayesh]

Thank you @ktomk - your experiment work looks quite extensive and thorough!

Because this PR was added to the 2.7 milestone, do you think you could open a new PR with the token-info improvements? I'm sure GitLab also has something similar on your end that we could incorporate. I look forward to your new PR, and kindly @-mention me when you do so.

[ktomk]

@Ayesh I like your thinking, however for only the token-info improvements, it should be a three/five/seven liner. Don't get blinded by the experiment that ships with some bulk (first implementation that is).

Let me scratch it, it is basically this part:

        $scopeHeader = $this->getHeader(self::GH_OAUTH_SCOPES_HEADER);

        if (null !== $scopeHeader) {
            $numberOfScopes = count(Preg::split('~,[ \x09]*~', $scopeHeader, 128, PREG_SPLIT_NO_EMPTY));
            if (0 === $numberOfScopes) {
                $buffer = 'classic (no scopes) ';
            } else {
                $buffer = sprintf('classic (%d scope%s) ', $numberOfScopes, 1 === $numberOfScopes ? '' : 's');
            }
        }

With the tiny detail that $scopeHeader MUST BE trim($scopeHeader), the getHeader() method not be used here, take the one from the $response.

[ktomk]

And for the relative time: I like it, but that one has the most complexity and while I have now played and tested with it, this must not ship right now. It's always good to leave something for the next iteration.

[Ayesh]

Thanks again for your comments and the code review. I still would like to keep this particular PR quite minimal, and also to not display the scope information. The way composer uses GitHub API, I don't think the scope information matters all that much, and the reason why I wanted to submit this PR is because it's been about a year since GH added fine-grained, so there are actual PATs that are expiring these days.

I didn't want to deal with the ValueError exceptions either, because it would have polluted the PR with version branching, and because I believe theoretically we will not be dealing with NULL bytes at that point in code. I hope you understand my sentiment behind this.

[ktomk]

The way composer uses GitHub API, I don't think the scope information matters all that much

Let me just comment this: If you see classic (>0 scopes) never expires, it's perhaps good to see during diagnose.

I didn't want to deal with the ValueError exceptions

I can understand your reasoning, from where I come from is that this added code should be silent as it's otherwise throwing while it adds additional information (it's not the main part of this diagnose check).

For this implementation it could be done by switching to strtotime() and that's all (in my experiment I wrapped the call internally in an try-catch'em-all, but I would not suggest that for the implementation here.)

The problem with unintended exceptions is that this brings the composer invocation down. That means, it will degrade non-interactive use. As you commented earlier: "Given that composer diagnose is likely called on systems that do not function properly, ...".

If you say you don't want to put the token info in, I'd say it's a bit sad, but it's ok to do that as expiration is the feature.

Potentially throwing code for expiration info we would otherwise silently drop, not ok as this renders the safe-guard the parsing is here moot. It still requires formatting of the parsed value or otherwise escaping of the header value.

[Ayesh]

Fair, what @ktomk mentioned about not breaking workflows with date formats and new potential exceptions. I really try not to use strtotime because of the "magic nature" of the function, so I wrapped it with a try/catch block. It tries to catch Throwable because Date extension can also throw \Errors in the unlikely case of timelib errors, etc.

[Seldaek]

Hey @Ayesh sorry this kinda got lost.. but would you say it's ready now? If so I'll try to review it tomorrow.

[Ayesh]

Hi @Seldaek - thank you for coming back to this.

The PR is ready :) I also rebased it from the current main branch HEAD.

[Seldaek]

Thanks!