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

Fix gitlab v15 token refresh #596

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

KSauter
Copy link

@KSauter KSauter commented Jun 20, 2022

No description provided.

@codecov
Copy link

codecov bot commented Jun 20, 2022

Codecov Report

Merging #596 (32c0d24) into master (50fe808) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master     #596   +/-   ##
=========================================
  Coverage     99.16%   99.16%           
- Complexity     1910     1912    +2     
=========================================
  Files           301      301           
  Lines          6072     6078    +6     
=========================================
+ Hits           6021     6027    +6     
  Misses           51       51           
Impacted Files Coverage Δ
src/Controller/OAuth/GitLabController.php 100.00% <100.00%> (ø)
src/Entity/User/OAuthToken.php 100.00% <100.00%> (ø)
src/Service/User/UserOAuthTokenRefresher.php 100.00% <100.00%> (ø)
...rvice/User/UserOAuthTokenRefresher/AccessToken.php 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50fe808...32c0d24. Read the comment docs.

@KSauter
Copy link
Author

KSauter commented Jun 20, 2022

⚠️ This will not fix already broken entries, or expired tokens which have no expire date in the database.

For this, an additional migrate task would be required.

@KSauter KSauter mentioned this pull request Jun 20, 2022
@Fahl-Design
Copy link
Contributor

interesting find and fix, is creating/register a new user from "oauth" still possible?
I will check it tomorrow

Copy link
Contributor

@Fahl-Design Fahl-Design left a comment

Choose a reason for hiding this comment

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

looks good to me, but please fix the redirect_route

@@ -11,7 +11,7 @@ knpu_oauth2_client:
type: gitlab
client_id: '%env(OAUTH_GITLAB_CLIENT_ID)%'
client_secret: '%env(OAUTH_GITLAB_CLIENT_SECRET)%'
redirect_route: register_gitlab_check
redirect_route: package_gitlab_check
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it still possible to register a user with oauth flow? (route: register_gitlab_check (\Buddy\Repman\Controller\OAuth\GitLabController::registerCheck) will create/login users \Buddy\Repman\Controller\OAuth\OAuthController::createAndAuthenticateUser

edit:
this breaks the initial redirect to create the user

Copy link
Author

Choose a reason for hiding this comment

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

To keep the redirect_url the same in the initial request for the oauth token which is stored and for the token refresh, i changed the config here.

I changed the route for registration to the register_gitlab_check in d22367b

Copy link
Contributor

@Fahl-Design Fahl-Design left a comment

Choose a reason for hiding this comment

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

👍

@belprixx
Copy link

belprixx commented Jul 5, 2022

🆙 please

@slappyslap
Copy link
Contributor

slappyslap commented Jul 8, 2022

Hi,
This PR works great, we just need to delete the record for gitlab in "user_oauth_token" table in the database

@babbassp
Copy link

babbassp commented Jul 28, 2022

+1. Updated code with these changes and I'm able to synchronize packages again. Thanks @KSauter and @Fahl-Design for addressing this!

[edit]
Spoke too soon. Still gives a bad request...

@temp
Copy link

temp commented Aug 2, 2022

Stumbled upon this PR after I got errors in repman after upgrading to gitlab 15.1.
Applied the changes, emtpied the user_oauth_token table, and it worked like a charm.
However, today it is back to errors on updates, with a different error:

Error: An error occurred while refreshing the access token: Bad Request

Anyone else experiencing this?

@temp
Copy link

temp commented Aug 2, 2022

The error from gitlab is:

{"error":"invalid_grant","error_description":"The provided authorization grant is invalid, expired, revoked, does not match the redirection URI used in the authorization request, or was issued to another client."}

@xvilo
Copy link
Contributor

xvilo commented Aug 31, 2022

We've just upgraded our GitLab to 15.x as well and are also seeing these issues. Yesterday I was able to fix it by adding an expires timestamp (might want to set a date by default in the next version). But are also now seeing the bad request error.

@temp / @KSauter / @babbassp do we need more work on this PR to get everything working? Or can we see if this one can be merged already, and create a new PR for other issues regarding the token refreshing for GitLab?

@temp
Copy link

temp commented Aug 31, 2022

@xvilo like I stated in this thread (or another one? don't know), the PR works, but for us it still requires manual intervention.
Need to clear the user-token table, call the "add repo" page, afterwards I can synchronize packages for a few minutes.
So it seems to me that something is still missing...

@KSauter
Copy link
Author

KSauter commented Aug 31, 2022

@temp are you sure that you are running the newest version of this MR? It works for me since Jun without any manual steps.

@xvilo if you set the expire date before you applied this patch, the refreshed tokens in your databare are broken. Please test if applying this patch and deleting the tokens from the DB will help. You have to go to the "app package" => "Gitlab" process to generate a new token after you updated everyting.

@temp
Copy link

temp commented Aug 31, 2022

@KSauter doh. I really had an error in the changed files. Will report later if it works now.
But one thing is still weird, I deleted everything from user_oauth_token table, afterwards logged in again, and I wasn't able to sync, because of "Missing OAuth Token". Had to jump to add package and select gitlab, afterwards the sync worked.
Shouldn't the OAuth token be fetched automatically?

@xvilo
Copy link
Contributor

xvilo commented Sep 2, 2022

So, to add, on 31 Aug 2022 we started using this branch (custom deployed fork on Kubernetes). Which, after removing the OAuth tokens from DB worked fine again.

HOWEVER, I'm currently looking at it (around 2 days later) and seeing these again:
Screenshot 2022-09-02 at 15 02 07

I suspect this PR is not fully ready yet, so this might confirm @temp's suspicions

@slappyslap
Copy link
Contributor

slappyslap commented Oct 31, 2022

Hey, when i reboot all the virtual machine of repman (Installation with ansible) the error with token will apear an another time, and i need to clear the user_oauth_token tabke again and retry the authentification with gitlab and it work again

@f3l1x
Copy link

f3l1x commented Jan 4, 2023

Confirmed @slappyslap comment.

  1. DELETE FROM "public"."user_oauth_token" WHERE "id" = 'yourtokenid';
  2. Go to organization -> add package -> gitlab and authenticate again.
  3. Works.

@mv-ics
Copy link

mv-ics commented Feb 3, 2023

@f3l1x Thx for the workaround. For people like me, running this in docker, there is a simple way to run the query using the console:

bin/console doctrine:query:sql "<QUERY HERE>"

@mikk150
Copy link
Contributor

mikk150 commented May 4, 2023

Seems to work fine for 55 days

@xuandung38
Copy link

Any update ?

1 similar comment
@prey87
Copy link

prey87 commented Jun 28, 2023

Any update ?

@slappyslap
Copy link
Contributor

I still have a problem, when i reboot the VM (ansible install), i need to clear the token and reconnect to gitlab every reboot, but i'm not able to reproduce this locally on my mac, I'm going to investigate with clone of the VM

@xvilo
Copy link
Contributor

xvilo commented Jul 12, 2023

Hi folks, how do we get this merged? /cc @akondas

@moay
Copy link

moay commented Jul 28, 2023

This bug is awfully annoying. We keep running into this on an near to every day basis. Is there any way we can speed this up here?

Does this PR work with the current code base? If so, why can't we merge it?

@xvilo
Copy link
Contributor

xvilo commented Jul 28, 2023

AFAIK @akondas is the only one with write access, and he seems to have stopped responding on any issue or PR.

We can create a fork and apply the patches ourselves, but this will only help with self-hosted instances of Repman

@konanado
Copy link

Nice project, but we are using Packeton with GitLab synchronization and merge request review feature. But it would be nice to have this features here too.

@slappyslap
Copy link
Contributor

slappyslap commented Jul 28, 2023 via email

@xvilo
Copy link
Contributor

xvilo commented Jul 29, 2023

It seems Packaton is not easy to scale horizontally, as there is no option to save package dists to S3, they need to be on disk. So doing this, requiring something like NFS shares or ReadWriteMany PVC volumes on Kubernetes. In case you don't need to scale that way, it's not such of a big issue, but for us it might be

@xvilo
Copy link
Contributor

xvilo commented Sep 25, 2023

It's really weird, it did work for quite a while. But since a week or two (no update on GitLab's side) it's not working anymore and we manually have to create a new token. I'm a bit confused by this TBH

@mv-ics
Copy link

mv-ics commented Jan 29, 2024

Just to leave this here: Packeton is really easy to setup. We have a setup where we run the docker version behind Caddy as a reverse proxy. Took me round about 2 hours to figure everything out + get it running. Sad to see Repman go, but it just got too annoying to revive the token every 2 days.

@Sevyls
Copy link

Sevyls commented Apr 9, 2024

any update?

@xvilo
Copy link
Contributor

xvilo commented Nov 27, 2024

I'd like to point out that:

  • This is still an issue
  • The patch still doesn't fully fixes it

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.