zaquestion / lab

Lab wraps Git or Hub, making it simple to clone, fork, and interact with repositories on GitLab
https://zaquestion.github.io/lab
Creative Commons Zero v1.0 Universal
1.1k stars 103 forks source link

cmd/root.go: Add config verification #855

Closed prarit closed 1 year ago

prarit commented 1 year ago

From GitLab:

"The ability to create personal access tokens without expiry was deprecated in GitLab 15.4 and removed in GitLab 16.0. In GitLab 16.0 and later, existing personal access tokens without an expiry date are automatically given an expiry date of 365 days later than the current date. The automatic adding of an expiry date occurs on GitLab.com during the 16.0 milestone. The automatic adding of an expiry date occurs on self-managed instances when they are upgraded to GitLab 16.0. This change is a breaking change."

Frequent users of GitLab are being caught by this 1-year expiration, and are seeing general command errors, for example,

[prarit@prarit centos-stream-9]$ lab mr show 123 2023/05/20 08:34:51 ERROR: mr_show.go:45: GET https://gitlab.com/api/v4/projects/redhat/centos-stream/src/kernel/centos-stream-9/merge_requests/123: 401 {message: 401 Unauthorized}

Add a simple 'cheap' test to get the user's information from the GitLab instance. On failure, output a better message.

Additional fix: Update the existing config setup failure message as well.

claytonrcarter commented 1 year ago

Would this add'l API call run on every invocation of lab? We're already bound by the API request/response time, so I am concerned that this will add add'l latency every time we run lab. (I just checked and the API responds to https://gitlab.com/api/v4/users?username=:username in anywhere from 200-500ms.)

Are you open to any alternative approaches? These are all obviously more complicated, but perhaps that may be worth it?

prarit commented 1 year ago

I'm not accusing @claytonrcarter of reading my mind, but there's a huge chance he did. And that is why I'm wrapping my head with aluminum foil right now.

Would this add'l API call run on every invocation of lab? We're already bound by the API request/response time, so I am concerned that this will add add'l latency every time we run lab. (I just checked and the API responds to https://gitlab.com/api/v4/users?username=:username in anywhere from 200-500ms.)

I couldn't figure out a better way to do it. See my responses below.

Are you open to any alternative approaches? These are all obviously more complicated, but perhaps that may be worth it?

Heh :) I'm always open to other approaches. FWIW, I push code, assume it is wrong, and wait for someone to tell me to do it a better way.

* Is there any way to trap errors from the "real" API calls and print this message only if the response is a 401? (I think this would be ideal, but may involve tracking down too many API call sites?)

I didn't see a convenient way to do this. 'lab mr show' vs 'lab issue edit' return errors from the golang gojira library. IOW, you're correct (and again I'm accusing you of reading my mind) there are too many API call sites to do this.

* Add a lottery mechanism so that this is only called on every nth request? (Sort of defeats the point, I guess.)

Yeah (again ... reading my mind :)) I thought about this approach as well. It isn't a good user experience if they end up with n-1 401 failures and then a loud 'real' failure at n.

* Add support for an (optional) `token_expires` config and `LAB_CORE_TOKEN_EXPIRES` env to bypass this check until after that expiration date? (Allow users to opt out of constant checking, but adds extra config.)

I was writing another snarky reply about you reading my mind and then I had an idea on what you said above.

I wonder if the solution here is to timestamp the token in lab.toml? So perhaps we ask someone to enter their gitlab PAT, and then ask them when it expires. Then we can warn a number of weeks out that the token is going to expire? Is that a bad experience?

* Use the [PAT](https://docs.gitlab.com/ee/api/personal_access_tokens.html#using-a-request-header) ([go-gitlab](https://pkg.go.dev/github.com/xanzy/go-gitlab#PersonalAccessTokensService.GetSinglePersonalAccessToken)) endpoint to query info on the current PAT and record it's expiration date for use in previous item.

I thought about this too. But I didn't know if that would be better/worse than the user ID lookup? It's still an API call.

* Similar to previous item, but use the PAT endpoint to query info on the current PAT, then cache the result somewhere (`/tmp/lab_<PAT ID>.txt`) to indicate that we shouldn't check again for a week or so.

Again, this would lead to a situation where the token could expire and not inform the user.

Let me look into the idea above of asking for a timestamp on expiry. I wonder if anyone uses lab with a short-lived token?

prarit commented 1 year ago

Another option: look up the expiry of the PAT one time. Copy the expiry date and then start warning a week or so out from expiry?

zampierilucas commented 1 year ago

Another option: look up the expiry of the PAT one time. Copy the expiry date and then start warning a week or so out from expiry?

Or just check once a day on the first lab command that the user runs, From the [docs] (https://docs.gitlab.com/ee/user/profile/personal_access_tokens.html#create-a-personal-access-token) "The token expires on that date at midnight UTC.", so probably can get away to check once a day.

prarit commented 1 year ago

I've modified the code to check the token once a day by fetching the user from the GitLab instance. If that fails the user will see, for example,

[prarit@prarit centos-stream-9]$ ~/Other/github/lab/lab issue show 123
2023/05/22 14:30:22 config.go:225: GET https://gitlab.com/api/v4/user: 401 {message: 401 Unauthorized}
Error: User authentication failed.  This is likely due to a misconfigured or expired Personal Access Token.  Verify the token or token_load config settings before attempting to authenticate.  A new token can be created at https://gitlab.com/-/profile/personal_access_tokens
zampierilucas commented 1 year ago

LGTM

prarit commented 1 year ago

The check failures in this MR also happen on an MR with no 'net' changes. I will investigate further to find out why the tests are failing.

claytonrcarter commented 1 year ago

I love the idea of querying the token expiry date and then saving it and warning about pending expiration, but I think that the proposed method of checking only 1x/day is a good solution too. 👍 Do you have any idea if test runs will also be able to "cache" that, or is there something about how they run (resetting the repo?) that will need to re-query multiple times?

As for the failing tests, the I opened #857 w/ a potential fix. (At least for the failures on this PR and my other one.)

BTW I've put away my 🔮 ... for now

prarit commented 1 year ago

Hey @claytonrcarter and @zampierilucas

Here's the situation. I'd like to add the expiry checking to be automatic, ie) do a lookup on the token and then determine if we should warn the user. However, this requires an update of the go mod packages associated with lab. That in turn requries some updates be made to various chunks of code (aside: I've done this already and will submit an MR).

Here's what I propose: This MR's solution is half done. I'm going to merge this change, then take a look at @claytonrcarter #857 and hopefully merge that. Then I'll look at their other MR, and also post the modules update MR ... and then finally do the second half of this MR :)