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: Allow use of relative URLs in config #1754

Merged
merged 6 commits into from Oct 29, 2021

Conversation

chlasch
Copy link
Contributor

@chlasch chlasch commented Sep 15, 2021

This PR reverts to previous behavior and allows relative uri-references for self-service URLS. This functionality matches the current documentation and addresses what seems like a bug that was introduced.

Our specific use case is an on-prem application that does not know it's hostname at deployment, and needs to be accessible by arbitrary DNS names or IP hostnames.

Related issue(s)

#1446

Checklist

Further Comments

I don't believe a documentation change is needed, as this should be working already according to the config examples and schema. eg. /Dashboard

@CLAassistant
Copy link

CLAassistant commented Sep 15, 2021

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Sep 15, 2021

Codecov Report

Merging #1754 (b24e9a3) into master (02c9e26) will decrease coverage by 1.19%.
The diff coverage is 88.00%.

❗ Current head b24e9a3 differs from pull request most recent head 2d1d017. Consider uploading reports for the commit 2d1d017 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1754      +/-   ##
==========================================
- Coverage   75.13%   73.93%   -1.20%     
==========================================
  Files         292      260      -32     
  Lines       15125    12715    -2410     
==========================================
- Hits        11364     9401    -1963     
+ Misses       2950     2685     -265     
+ Partials      811      629     -182     
Impacted Files Coverage Δ
driver/config/config.go 81.53% <78.57%> (-0.16%) ⬇️
internal/testhelpers/selfservice.go 100.00% <100.00%> (ø)
text/message_system.go 0.00% <0.00%> (-100.00%) ⬇️
cmd/migrate/sql.go 57.14% <0.00%> (-42.86%) ⬇️
text/message_recovery.go 63.04% <0.00%> (-36.96%) ⬇️
text/message_validation.go 64.70% <0.00%> (-35.30%) ⬇️
cmd/cliclient/migrate.go 0.00% <0.00%> (-34.55%) ⬇️
ui/node/attributes.go 27.58% <0.00%> (-33.29%) ⬇️
text/message_verification.go 67.56% <0.00%> (-32.44%) ⬇️
text/message_login.go 71.69% <0.00%> (-28.31%) ⬇️
... and 117 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 034806f...2d1d017. Read the comment docs.

Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

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

To prevent regression of this bug (again 😅), it would be good to add a case to one of the handlers that ensures it actually does a relative redirect when configured to do so.
Ideally even in all flow handlers like https://github.com/ory/kratos/blob/43c315022f05206f1ab0db82b3c98b3fd333921d/selfservice/flow/login/handler_test.go
https://github.com/ory/kratos/blob/43c315022f05206f1ab0db82b3c98b3fd333921d/selfservice/flow/registration/handler_test.go
and the others.

driver/config/config_test.go Outdated Show resolved Hide resolved
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Awesome, thank you for your contribution! This looks pretty good and I have some ideas how to improve it further :)

driver/config/config.go Show resolved Hide resolved
@chlasch
Copy link
Contributor Author

chlasch commented Sep 16, 2021

To prevent regression of this bug (again 😅), it would be good to add a case to one of the handlers that ensures it actually does a relative redirect when configured to do so.
Ideally even in all flow handlers like https://github.com/ory/kratos/blob/43c315022f05206f1ab0db82b3c98b3fd333921d/selfservice/flow/login/handler_test.go
https://github.com/ory/kratos/blob/43c315022f05206f1ab0db82b3c98b3fd333921d/selfservice/flow/registration/handler_test.go
and the others.

I added an explicit redirect check to the 3 handler tests that already had TestInitFLow functions (login, recovery, registration). I opted for a testhelper function to check the redirect, instead of duplicating the test function in each handler_test, but I'm not sure if this is how you want it done.

Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

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

Thanks a lot, this looks very good now 🎉

@@ -197,3 +197,17 @@ func SelfServiceMakeHookRequest(t *testing.T, ts *httptest.Server, suffix string
require.NoError(t, err)
return res, string(body)
}

func GetSelfServiceRedirectLocation(t *testing.T, url string) string {
Copy link
Member

Choose a reason for hiding this comment

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

Perfect 👍

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Great work! :)

Could you add tests for the remaining (verification, settings, error) flows also please? :)

That ensures that this never unintentionally breaks - so it’s like an insurance for you :)

@chlasch
Copy link
Contributor Author

chlasch commented Sep 17, 2021

Great work! :)

Could you add tests for the remaining (verification, settings, error) flows also please? :)

That ensures that this never unintentionally breaks - so it’s like an insurance for you :)

I can give it a shot, but since those flows don't have the TestInitFlow functions, I got a little stuck in understanding where it should fall within the test.

@aeneasr
Copy link
Member

aeneasr commented Sep 18, 2021

Oh I see! If you have difficulties figuring it out feel free to ask here :)

@aeneasr
Copy link
Member

aeneasr commented Oct 12, 2021

While the PR is being worked on I will mark it as a draft. That declutters our review backlog :)

Once you're done with your changes and would like someone to review them, mark the PR as ready and request a review from one of the maintainers.

Thank you!

@aeneasr aeneasr marked this pull request as draft October 12, 2021 05:57
Separate functions for absolute and relative URLS
Add distinct test cases for relative urls
Add tests for error, verification, and settings flows
@chlasch chlasch marked this pull request as ready for review October 25, 2021 17:52
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Great job, thank you! :)

@aeneasr
Copy link
Member

aeneasr commented Oct 27, 2021

Re-running tests

@aeneasr aeneasr merged commit 5f73bb0 into ory:master Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants