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

Encode the authentication reject error to build a proper callback url #568

Merged
merged 7 commits into from
Sep 27, 2023

Conversation

vinokurig
Copy link
Contributor

@vinokurig vinokurig commented Sep 21, 2023

What does this PR do?

Encode the &error_code=access_denied query param for the callback url in order to fix the bug when the authentication request appears again if it was rejected.

DO NOT MERGE until eclipse-che/che-dashboard#932 is merged

Screenshot/screencast of this PR

What issues does this PR fix or reference?

eclipse-che/che#22499
fixes eclipse-che/che#22539

How to test this PR?

  1. Deploy che with the PR image quay.io/eclipse/che-server:pr-568
  2. Setup the GitHub oauth flow.
  3. Create a workspace from a public GitHub repository.
  4. When the authentication page appears, reject the request.

See: the workspace start proceeds without fetching the token.

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

@tolusha
Copy link
Contributor

tolusha commented Sep 21, 2023

Please find the error below:
screencast-bpconcjcammlapcogcnnelfmaeghhagj-2023 09 21-13_06_52

@vinokurig
Copy link
Contributor Author

vinokurig commented Sep 21, 2023

@tolusha

Please find the error below

This is another bug. You can reproduce it even on dogfooding:

  1. Start a workspace from a GitHub repository, accept the authorisation as you can not reject it because of this bug.
  2. Stop the workspace.
  3. Go to dashboard -> user preferences -> git services and revoke the authorisation.
  4. Start the workspace and reject the authorisation.

I don't think we should do something with this because according to the issue we will not ask to authorise on workspace start, as the authorisation can be either accepted or rejected once on workspace create step.

The PR does not contain this part as the approach should be clarified with @ibuziuk

@tolusha
Copy link
Contributor

tolusha commented Sep 21, 2023

/test v12-bitbucket-no-pat-oauth-flow

@vinokurig
Copy link
Contributor Author

@tolusha opened a separate issue: eclipse-che/che#22539

@openshift-ci openshift-ci bot removed the lgtm label Sep 25, 2023
@openshift-ci
Copy link

openshift-ci bot commented Sep 25, 2023

@vinokurig: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/v12-git-no-pat-oauth-flow acfb9df link true /test v12-git-no-pat-oauth-flow

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@artaleks9
Copy link
Contributor

Verified on Che deployed with

quay.io/eclipse/che-server:pr-568
quay.io/eclipse/che-dashboard:pr-932

The functionality works as expected.

@openshift-ci
Copy link

openshift-ci bot commented Sep 27, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: artaleks9, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@vinokurig vinokurig merged commit b2ff110 into main Sep 27, 2023
22 of 23 checks passed
@vinokurig vinokurig deleted the che-22499 branch September 27, 2023 11:20
@devstudio-release
Copy link

Build 3.10 :: server_3.x/250: Console, Changes, Git Data

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.10 :: server_3.x/250: SUCCESS

Upstream sync done; /DS_CI/sync-to-downstream_3.x/4811 triggered

vinokurig added a commit that referenced this pull request Sep 27, 2023
…#568)

Encode the &error_code=access_denied query param for the callback url in order to fix the bug when the authentication request appears again if it was rejected.
@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.10 :: sync-to-downstream_3.x/4812: SUCCESS

Build container: devspaces-operator-bundle synced; /DS_CI/get-sources-rhpkg-container-build_3.x/4606 triggered; /job/DS_CI/job/dsc_3.x triggered;

@devstudio-release
Copy link

Build 3.10 :: operator-bundle_3.x/2104: SUCCESS

Upstream sync done; /DS_CI/sync-to-downstream_3.x/4812 triggered

@devstudio-release
Copy link

Build 3.10 :: copyIIBsToQuay/1952: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.10 :: dsc_3.x/1423: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.10 :: update-digests_3.x/4458: SUCCESS

Detected new images: rebuild operator-bundle
* server; /DS_CI/operator-bundle_3.x/2104 triggered

@devstudio-release
Copy link

Build 3.10 :: dsc_3.x/1423: SUCCESS

3.10.0 CI

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