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 OIDC looping issue - too many redriects #1014

Merged
merged 2 commits into from
Jun 24, 2022

Conversation

aoguan1990
Copy link
Contributor

@aoguan1990 aoguan1990 commented Jun 21, 2022

Signed-off-by: Aozixuan Priscilla Guan [email protected]

Description

Customized error handling mechanism based on the error message for OIDC routing

Category

Bug fix

Why these changes are required?

Resolve redirect login looping issues when authentication failures detected.

What is the old behavior before changes and new behavior after changes?

Old Behavior:

Any exceptions caught during the OIDC authentication process causes redirecting login infinitely.

New Behavior:
If error message includes "authentication error": => return 401: unauthorized
Else: redirect to login

Issues Resolved

#990

Testing

unit testing and integration testing

Check List

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@aoguan1990 aoguan1990 requested a review from a team June 21, 2022 00:35
@cliu123
Copy link
Member

cliu123 commented Jun 21, 2022

@aoguan1990 Great contribution! Please sign the commit and add tests for the fix.

aoguan1990 and others added 2 commits June 21, 2022 16:52
Signed-off-by: Aozixuan, Priscilla, Guan <[email protected]>
Signed-off-by: Aozixuan Priscilla Guan <[email protected]>
Signed-off-by: Aozixuan Priscilla Guan <[email protected]>
@aoguan1990
Copy link
Contributor Author

aoguan1990 commented Jun 21, 2022

@aoguan1990 Great contribution! Please sign the commit and add tests for the fix.

@cliu123 Unit test and commit issues are fixed. Please advice when we can resolve the integration test blocker.

@cliu123
Copy link
Member

cliu123 commented Jun 21, 2022

@aoguan1990 Thanks for resoving these issues! Could you please add tests for the fix? Without the fix, the test should fail. With the fix, the test should pass.

@seraphjiang
Copy link
Member

I saw integration test failed on download 2.1 security artifacts. do we have 2.1 artifacts now?

https://ci.opensearch.org/ci/dbc/distribution-build-opensearch/2.1.0/3911/linux/x64/tar/builds/opensearch/plugins/opensearch-security-2.1.0.0.zip

Run wget -O opensearch-security.zip https://ci.opensearch.org/ci/dbc/distribution-build-opensearch/2.1.0/latest/linux/x64/tar/builds/opensearch/plugins/opensearch-security-2.1.0.0.zip
--2022-06-21 21:53:44--  https://ci.opensearch.org/ci/dbc/distribution-build-opensearch/2.1.0/latest/linux/x64/tar/builds/opensearch/plugins/opensearch-security-2.1.0.0.zip
Resolving ci.opensearch.org (ci.opensearch.org)... 52.84.125.21, 52.84.125.109, 52.84.125.1[7](https://github.com/opensearch-project/security-dashboards-plugin/runs/6993741182?check_suite_focus=true#step:3:8), ...
Connecting to ci.opensearch.org (ci.opensearch.org)|52.84.125.21|:443... connected.
HTTP request sent, awaiting response... 302 Moved Temporarily
Location: /ci/dbc/distribution-build-opensearch/2.1.0/3911/linux/x64/tar/builds/opensearch/plugins/opensearch-security-2.1.0.0.zip [following]
--2022-06-21 21:53:44--  https://ci.opensearch.org/ci/dbc/distribution-build-opensearch/2.1.0/3911/linux/x64/tar/builds/opensearch/plugins/opensearch-security-2.1.0.0.zip
Reusing existing connection to ci.opensearch.org:443.
HTTP request sent, awaiting response... 403 Forbidden
2022-06-21 21:53:45 ERROR 403: Forbidden.
Error: Process completed with exit code [8](https://github.com/opensearch-project/security-dashboards-plugin/runs/6993741182?check_suite_focus=true#step:3:9).

@cliu123
Copy link
Member

cliu123 commented Jun 22, 2022

@seraphjiang This is an known issue. 2.1.0 build failed, so the artifact hasn't been available yet. This PR needs to wait for the artifact.
@zelinh @gaiksaya When is the 2.1.0 artifact expected to be available?

@DarshitChanpura
Copy link
Member

DarshitChanpura commented Jun 24, 2022

@opensearch-project/security Can we get a second review for this?

@cliu123 cliu123 merged commit 015dc3f into opensearch-project:main Jun 24, 2022
@cliu123
Copy link
Member

cliu123 commented Jun 24, 2022

@aoguan1990 @seraphjiang Thanks for the contribution!

@peternied
Copy link
Member

@aoguan1990 I know this was merged, but I do not see test modifications in the pull request, could you make another pull request to include them?

@DarshitChanpura DarshitChanpura added the backport 2.x backport to 2.x branch label Jun 27, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 27, 2022
Signed-off-by: Aozixuan Priscilla Guan <[email protected]>
(cherry picked from commit 015dc3f)
@aoguan1990
Copy link
Contributor Author

@aoguan1990 I know this was merged, but I do not see test modifications in the pull request, could you make another pull request to include them?

@peternied Due to the technical challenge, our existing test framework does not include test cases for OIDC authentication. As per discussion with @seraphjiang and @zengyan-amazon, we can revisit the OIDC test case issue later. So closed the issue #990 for now.

DarshitChanpura pushed a commit that referenced this pull request Jun 27, 2022
Signed-off-by: Aozixuan Priscilla Guan <[email protected]>
(cherry picked from commit 015dc3f)

Co-authored-by: Aozixuan Priscilla Guan <[email protected]>
spartan2015 pushed a commit to spartan2015/security-dashboards-plugin that referenced this pull request Aug 8, 2022
Signed-off-by: Aozixuan Priscilla Guan <[email protected]>
Signed-off-by: Vasile Negru <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants