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(http): allow tokens to exceed 20 chars #207

Merged
merged 1 commit into from
Aug 2, 2021

Conversation

max-wittig
Copy link
Contributor

GitLab allows users of an instance to specify a token prefix since 13.8, which is where this current assumption breaks down

See: https://docs.gitlab.com/ee/user/admin_area/settings/account_and_limit_settings.html#personal-access-token-prefix

@max-wittig max-wittig force-pushed the fix/token-prefix-assumption branch 3 times, most recently from e6d3fd4 to f1f7436 Compare February 5, 2021 09:43
@max-wittig
Copy link
Contributor Author

@mapret Thanks for the initial review! Pipeline should be green now. Would you mind to take another look?

@max-wittig
Copy link
Contributor Author

@mapret Care to review again? This library is currently not working for people on our instance

@mapret
Copy link

mapret commented Feb 18, 2021

@max-wittig Looks good to me, but I am not a maintainer of this project, I am just the one who opened the related issue on our internal GitLab 😉.

@max-wittig
Copy link
Contributor Author

@jetersen @nmklotas Would you mind to take a look?

Copy link
Collaborator

@jetersen jetersen left a comment

Choose a reason for hiding this comment

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

I think it would be better to distinguish between bearer token and PAT via two properties.
That way the switch can go away and there will be no need to throw exception.

FYI you can write the same if statements as a switch case, so there was no need to change the entire signature.

@max-wittig
Copy link
Contributor Author

@jetersen thanks for the review. What do you mean by properties? Attributes that the user specifies? I'm not really into C# and this was ment as a bit of a quick fix so I'm not too familiar with all the details of the language.

@wwwjon
Copy link

wwwjon commented Mar 24, 2021

I came across this pull request because I encountered the same issue and I'm very interested that this issues is fixed as soon as possible. So I would like to help, but unfortunately I don't understand either exactly how @jetersen imagined it with properties.

@jetersen Could you explain this in more detail?

GitLab allows users of an instance to specify a token prefix since 13.8, which is where this current assumption breaks down

See: https://docs.gitlab.com/ee/user/admin_area/settings/account_and_limit_settings.html#personal-access-token-prefix
@max-wittig
Copy link
Contributor Author

@jetersen Using switch again now. Could you please take another look?

ap0llo added a commit to ap0llo/changelog that referenced this pull request Jul 3, 2021
Allow GitLab access tokens longer than 20 characters to be used (when custom prefix is configured on a GitLab instance)

Pull-Request: #247
See-Also: https://docs.gitlab.com/ee/user/admin_area/settings/account_and_limit_settings.html#personal-access-token-prefix
See-Also: nmklotas/GitLabApiClient#207
See-Also: nmklotas/GitLabApiClient#204
@fgreinacher
Copy link

fgreinacher commented Aug 2, 2021

@jetersen @nmklotas Mind having another look at this? We're also running in the underlying issue 😀

@jetersen jetersen added the bug label Aug 2, 2021
@jetersen jetersen merged commit 2ca5ea6 into nmklotas:master Aug 2, 2021
@jetersen
Copy link
Collaborator

jetersen commented Aug 2, 2021

released in 1.8.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants