Fix gitlab oauth token validation request #674
Conversation
|
/retest |
1 similar comment
|
/retest |
|
@vinokurig : hello,
Did you mean |
Right, When I start creating a che-server pull request I don't know the id before it is opened. This time I forgot to update the description after the pr creation. Updated the description. |
| @@ -93,7 +94,8 @@ public OAuthToken getToken(String userId) throws IOException { | |||
| if (token == null | |||
| || token.getToken() == null | |||
| || token.getToken().isEmpty() | |||
| || getJson(gitlabUserEndpoint, token.getToken(), GitLabUser.class) == null) { | |||
| || isNullOrEmpty( | |||
There was a problem hiding this comment.
@vinokurig could you clarify the principle difference and how it fixes gitlab validation? can we add a unit test for that ?
There was a problem hiding this comment.
could you clarify the principle difference and how it fixes gitlab validation?
Currently, in the main branch we check that the GitLabUser object is not null. If we request the GitLab user with an invalid token, we will not get null, but a GitLabUser object with empty (null) fields. To fix that I changed the check to verify if the userId field is not null.
can we add a unit test for that ?
Done
| @@ -93,7 +94,8 @@ public OAuthToken getToken(String userId) throws IOException { | |||
| if (token == null | |||
| || token.getToken() == null | |||
| || token.getToken().isEmpty() | |||
| || getJson(gitlabUserEndpoint, token.getToken(), GitLabUser.class) == null) { | |||
| || isNullOrEmpty( | |||
| getJson(gitlabUserEndpoint, token.getToken(), GitLabUser.class).getId())) { | |||
| return null; | |||
There was a problem hiding this comment.
returning null is a bad practice in general
There was a problem hiding this comment.
Agree, added a TODO for refactoring to return Optional.
There was a problem hiding this comment.
Test scenario from description with additional steps has tested successfully using quay.io/eclipse/che-server:pr-674 image using private repo https://gitlab-gitlab-system.apps.git.crw-qe.com/admin-user/private-repo.git with Gitlab Server on OCP 4.14.
Remark: Git Services tab showed state "unauthorized", not "unauthorised".
I would suggest using the same vocabulary in the test scenario as used in Eclipse Che UI.
Test scenario with additional steps:
- Deploy che with the PR image: quay.io/eclipse/che-server:pr-674
- Configure Gitlab oauth.
- Start a workspace from a Gitlab repository with a devfile https://gitlab-gitlab-system.apps.git.crw-qe.com/admin-user/private-repo.git.
3.a See page to authorize OAuth on GitLab server during the workspace startup. - Go to dashboard -> User Preferences -> Git Services tab, see: the Gitlab provider is in the authorized state.
- Revoke the Gitlab authorization.
- See: the authorization state of the Gitlab provider has changed to
unauthorized. - Refresh dashboard page
- See: the authorization state of the Gitlab provider is
unauthorized. - Start a new workspace from a Gitlab repository with a devfile https://gitlab-gitlab-system.apps.git.crw-qe.com/admin-user/private-repo.git.
- Go to VS Code Editor terminal and make sure
git config --list | grep user.namereturnsTestUser. - See page to authorize OAuth on GitLab server during the workspace startup.
- Start a new workspace from a Gitlab repository with a devfile https://gitlab-gitlab-system.apps.git.crw-qe.com/admin-user/private-repo.git.
- See NO page to authorize OAuth on GitLab server during the workspace startup.
Test screencast: screencast-bpconcjcammlapcogcnnelfmaeghhagj-2024.04.03-11_59_12.webm
vinokurig
force-pushed
the
che-22836_gitlab
branch
from
851692e
to
4bdb2a3
Compare
vinokurig
force-pushed
the
che-22836_gitlab
branch
from
4bdb2a3
to
d86f795
Compare
|
/retest |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dmytro-ndp, ibuziuk, tolusha, vinokurig The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Build 3.13 :: server_3.x/332: Console, Changes, Git Data |
|
Build 3.13 :: sync-to-downstream_3.x/6404: Console, Changes, Git Data |
|
Build 3.13 :: push-latest-container-to-quay_3.x/4507: Console, Changes, Git Data |
|
Build 3.13 :: get-sources-rhpkg-container-build_3.x/6366: server : 3.x :: Build 60253287 : quay.io/devspaces/server-rhel8:3.13-9 |
|
Build 3.13 :: server_3.x/332: Upstream sync done; /DS_CI/sync-to-downstream_3.x/6404 triggered |
|
Build 3.13 :: update-digests_3.x/6247: Console, Changes, Git Data |
|
Build 3.13 :: operator-bundle_3.x/2732: Console, Changes, Git Data |
|
Build 3.13 :: sync-to-downstream_3.x/6407: Console, Changes, Git Data |
|
Build 3.13 :: push-latest-container-to-quay_3.x/4511: Console, Changes, Git Data |
|
Build 3.13 :: copyIIBsToQuay/2600: Console, Changes, Git Data |
|
Build 3.13 :: sync-to-downstream_3.x/6407: Build container: devspaces-operator-bundle synced; /DS_CI/get-sources-rhpkg-container-build_3.x/6369 triggered; /job/DS_CI/job/dsc_3.x triggered; |
|
Build 3.13 :: operator-bundle_3.x/2732: Upstream sync done; /DS_CI/sync-to-downstream_3.x/6407 triggered |
|
Build 3.13 :: update-digests_3.x/6247: Detected new images: rebuild operator-bundle |
|
Build 3.13 :: dsc_3.x/1883: Console, Changes, Git Data |
|
Build 3.13 :: dsc_3.x/1883: 3.13.0-CI |
What does this PR do?
Rework the gitlab oauth token validation check to verify if the test response has data in the returned object.
Screenshot/screencast of this PR
What issues does this PR fix or reference?
eclipse-che/che#22836
How to test this PR?
quay.io/eclipse/che-server:pr-674authorisedstate.See: the authorisation state of the Gitlab provider has changed to
unauthorised.PR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or referenceandHow to test this PRcompletedReviewers
Reviewers, please comment how you tested the PR when approving it.