Rework the Bitbucket Server oauth token validation #673
Conversation
Signed-off-by: ivinokur <ivinokur@redhat.com>
|
/retest |
2 similar comments
|
/retest |
|
/retest |
...auth-bitbucket/src/main/java/org/eclipse/che/security/oauth/BitbucketOAuthAuthenticator.java
Show resolved
Hide resolved
| bitbucketServerApiClient.getPersonalAccessToken( | ||
| personalAccessToken.getScmTokenId(), personalAccessToken.getToken()); |
There was a problem hiding this comment.
It is hard to understand, why to retrieve personal access token we have to pass personal access token.
There was a problem hiding this comment.
Right, the PersonalAccessToken interface has a bit misleading name. In this case we implement an oauth token, but not a PAT.
There was a problem hiding this comment.
The Bitbucket Server oauth works in a bit different way: it uses the oauth token to create a PAT and then uses the PAT for all other operations.
There was a problem hiding this comment.
@tolusha Do you think we need to rename the PersonalAccessToken interface to AccessToken in order to avoid such confusion?
There was a problem hiding this comment.
Added a javadoc with a TODO.
|
/retest |
There was a problem hiding this comment.
I have followed test scenario from PR description and, at the end, after revoking of BitBucket Server OAuth 2 application and refreshing 'User Preferences' Dashboard page the 'Authorization' indicator was turned off, and provider's authorization state was Unauthorized.
Test screencast:
screencast-bpconcjcammlapcogcnnelfmaeghhagj-2024.04.05-21_49_42.webm
PR seems to be fixing the target issue eclipse-che/che#22836 regarding BitBucket Server, as expected.
P.S. @vinokurig: so that test scenario in description mentioned BitBucket Server OAuth 2 only, the issue eclipse-che/che#22836 could be still actual to OAuth 1, coudn't it?
If so, I would suggest changing PR title to "Rework the Bitbucket Server oauth 2 token validation" to make the fact obvious.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dmytro-ndp, 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 |
The pull request is relevant for both oauth1 and oauth2 standarts. |
|
Build 3.14 :: server_3.x/333: Console, Changes, Git Data |
|
@vinokurig : thanks for the answer. |
|
Build 3.14 :: sync-to-downstream_3.x/6443: Console, Changes, Git Data |
|
Build 3.14 :: push-latest-container-to-quay_3.x/4528: Console, Changes, Git Data |
|
Build 3.14 :: get-sources-rhpkg-container-build_3.x/6403: server : 3.x :: Build 60319971 : quay.io/devspaces/server-rhel8:3.14-1 |
|
Build 3.14 :: server_3.x/333: Upstream sync done; /DS_CI/sync-to-downstream_3.x/6443 triggered |
|
Build 3.14 :: update-digests_3.x/6287: Console, Changes, Git Data |
|
Build 3.14 :: operator-bundle_3.x/2764: Console, Changes, Git Data |
|
Build 3.14 :: sync-to-downstream_3.x/6445: Console, Changes, Git Data |
|
Build 3.14 :: get-sources-rhpkg-container-build_3.x/6405: devspaces-operator-bundle : 3.x :: Failed in 60320346 : BREW:BUILD/STATUS:UNKNOWN |
What does this PR do?
/rest/api/1.0/application-propertiesrequest is irrelevant as it does not require a token.getPersonalAccessToken()API request in order to avoid circulargetToken()request.Screenshot/screencast of this PR
What issues does this PR fix or reference?
fixes eclipse-che/che#22836
How to test this PR?
quay.io/eclipse/che-server:pr-673Authorisedstate.See: The provider's authorisation state is
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.