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

[Core] fix timeout redirect location #9534

Merged
merged 1 commit into from
Feb 19, 2025

Conversation

ridz1208
Copy link
Collaborator

@ridz1208 ridz1208 commented Jan 16, 2025

Brief summary of changes

The feature introduced in #9041 seems to be broken on 26. The changes I'm proposing are the hack that i'm temporarily using on CBIG but there might be a better way of doing it.

One cause of the bug might actually be another PR submitted by maxime and merged #9055. not sure if the changes in that PR broke the chnages in #9041

Below are a couple examples of the existing redirect for URLs

http://localhost:8083/user_accounts/edit_user/rida
<a href="/login?redirect=http%3A%2F%2Flocalhost%3A8083%2Fedit_user%2Frida">Try logging in</a>

http://localhost:8083/configuration/
<a href="/login?redirect=http%3A%2F%2Flocalhost%3A8083">Try logging in</a>

Testing instructions (if applicable)

Link(s) to related issue(s)

  • Resolves # (Reference the issue this fixes, if any.)

@ridz1208
Copy link
Collaborator Author

@maximemulder @driusan I'd appreciate a quick look at this one

@ridz1208 ridz1208 added Category: Bug PR or issue that aims to report or fix a bug Area: UI PR or issue related to the user interface Language: PHP PR or issue that update PHP code 26.0.0-bugs Issues that were raised during the release testing for 26.0.0 Project: C-BIG Issue or PR related to the C-BIG project Difficulty: Medium PR or issue that require a moderate effort or expertise to implement, review, or test labels Jan 16, 2025
@ridz1208
Copy link
Collaborator Author

@kongtiaowang not sure why the tests are failing here, is the branch broken ?

@maximemulder
Copy link
Contributor

maximemulder commented Jan 16, 2025

@ridz1208 A GitHub update broke CI during the new year, the fix has been committed to main but not 26, so the branch is indeed broken. @kongtiaowang can you push the fix to 26 as well ?

@maximemulder
Copy link
Contributor

#9055 got merged before #9041, so it did not break #9041 (in fact, it was a prerequisite to #9041).

I must say I am not sure why the current code does not work for you. I am not opposed to a fix but I'd prefer someone else to look into it if possible.

@ridz1208
Copy link
Collaborator Author

@maximemulder I agree someone else should look into it but on your end can you just see, on 26, the redirect works for you? I just want to know if its still working on your environment but not on mine

@kongtiaowang
Copy link
Contributor

kongtiaowang commented Jan 16, 2025

@ridz1208 @maximemulder #9536 will fix the CI failure issue.

@ridz1208
Copy link
Collaborator Author

@kongtiaowang I think this PR was assigned to you for testing.

Best way I found to test it is to login, go to a page with a complex URL, copy the URL, logout, try to go to the URL again (as if you timed out), you get a 403 error, then choose the option to try signing in and see if it redirects you to your original URL

@ridz1208 ridz1208 added this to the 27.0.0 milestone Feb 13, 2025
@kongtiaowang kongtiaowang added the Passed manual tests PR has been successfully tested by at least one peer label Feb 19, 2025
Copy link
Contributor

@kongtiaowang kongtiaowang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@driusan driusan merged commit aee6c04 into aces:26.0-release Feb 19, 2025
32 of 51 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
26.0.0-bugs Issues that were raised during the release testing for 26.0.0 Area: UI PR or issue related to the user interface Category: Bug PR or issue that aims to report or fix a bug Difficulty: Medium PR or issue that require a moderate effort or expertise to implement, review, or test Language: PHP PR or issue that update PHP code Passed manual tests PR has been successfully tested by at least one peer Project: C-BIG Issue or PR related to the C-BIG project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants