Do not return secret token in the Oauth API #538
Conversation
Signed-off-by: Igor Vinokur <[email protected]>
vinokurig
requested review from
l0rd,
ibuziuk,
nickboldt and
tolusha
as code owners
| throw new UnauthorizedException( | ||
| "OAuth token for user " + subject.getUserId() + " was not found"); | ||
| } catch (IOException | ScmConfigurationPersistenceException | ScmCommunicationException e) { |
There was a problem hiding this comment.
That snippet above was add as a part of #408
Could you check if that functionality still works?
There was a problem hiding this comment.
Yes, I have checked the revoke token functionality and it works fine.
|
/retest |
| Optional<PersonalAccessToken> tokenOptional = | ||
| personalAccessTokenManager.get(subject, provider.getEndpointUrl()); | ||
| if (tokenOptional.isPresent()) { | ||
| PersonalAccessToken accessToken = tokenOptional.get(); | ||
| return newDto(OAuthToken.class).withToken(accessToken.getToken()); | ||
| } |
There was a problem hiding this comment.
@vinokurig why have we got PAT as part of OAuth flow in general and now removing it?
There was a problem hiding this comment.
I believe this was added by a mistake.
There was a problem hiding this comment.
I was able to continue working with Eclipse Che after executing of step 3:
- Che Setup bitbucket-server oauth
Start a workspace from the bitbucket-server repo to initiate the OAuth exchange.
Go to dashboard -> User preferences -> Personal Access Tokens tab and add a new bitbucket-server token. You can use a random string for the token input.
There was no "backend is not available" error [1] observed.
@vinokurig : could you please prepare backport PR to 7.72.x branch for review to make the fix available in DS 3.8.0?
|
[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.9 :: server_3.x/197: Console, Changes, Git Data |
Remove the personalAccessTokenManager.get() call from the OAuth API getToken() method. The OAuth API must not know anything about PAT secrets. It should get tokens only by requesting an SCM provider OAuth API. Fix validating the Bitbucket-Server PAT method by requesting user instead of requesting. This prevents the code execution going to a recursive loop: bitbucketServerApiClient.getPersonalAccessToken() calls oauthApi.getToken() which referred to personalAccessTokenManager.getToken() which validated the token by calling scmPersonalAccessTokenFetcher.getScmUsername() -> bitbucketServerApiClient.getPersonalAccessToken().
|
Build 3.9 :: sync-to-downstream_3.x/4040: Console, Changes, Git Data |
|
Build 3.9 :: push-latest-container-to-quay_3.x/2962: Console, Changes, Git Data |
|
Build 3.9 :: get-sources-rhpkg-container-build_3.x/3894: server : 3.x :: Build 54729933 : quay.io/devspaces/server-rhel8:3.9-8 |
|
Build 3.9 :: server_3.x/197: Upstream sync done; /DS_CI/sync-to-downstream_3.x/4040 triggered |
|
Build 3.9 :: update-digests_3.x/3954: Console, Changes, Git Data |
|
Build 3.9 :: operator-bundle_3.x/1776: Console, Changes, Git Data |
|
Build 3.9 :: sync-to-downstream_3.x/4045: Console, Changes, Git Data |
|
Build 3.9 :: get-sources-rhpkg-container-build_3.x/3898: devspaces-operator-bundle : 3.x :: Failed in : BREW:BUILD/STATUS:UNKNOWN |
|
Build 3.9 :: server_3.x/198: Console, Changes, Git Data |
|
Build 3.9 :: sync-to-downstream_3.x/4070: Console, Changes, Git Data |
|
Build 3.9 :: get-sources-rhpkg-container-build_3.x/3924: server : 3.x :: Failed in : BREW:BUILD/STATUS:UNKNOWN |
What does this PR do?
personalAccessTokenManager.get()call from the OAuth APIgetToken()method. The OAuth API must not know anything about PAT secrets. It should get tokens only by requesting an SCM provider OAuth API.This prevents the code execution going to a recursive loop:
bitbucketServerApiClient.getPersonalAccessToken()callsoauthApi.getToken()which referred topersonalAccessTokenManager.getToken()which validated the token by callingscmPersonalAccessTokenFetcher.getScmUsername()->bitbucketServerApiClient.getPersonalAccessToken().Screenshot/screencast of this PR
What issues does this PR fix or reference?
https://issues.redhat.com/browse/CRW-4351
How to test this PR?
See: the token creation is rejected, but no backend errors appear.
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.