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

Updated manager security. Fixes #1741 #1742

Merged
merged 2 commits into from
Oct 25, 2021
Merged

Conversation

tidyui
Copy link
Member

@tidyui tidyui commented Oct 21, 2021

No description provided.

@tidyui
Copy link
Member Author

tidyui commented Oct 21, 2021

Review requested by @hagaiwech & @tedvanderveen

@@ -104,10 +104,9 @@ public async Task<IActionResult> OnPostAsync(string returnUrl = null)

if (!string.IsNullOrEmpty(returnUrl))
{
return LocalRedirect(returnUrl);
return LocalRedirect($"~/manager/login/auth?returnUrl={ returnUrl }");
Copy link
Member Author

Choose a reason for hiding this comment

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

Custom authentication providers should also redirect to this new route which is responsible for setting up the CSRF request token for the application.

@@ -14,7 +14,28 @@ piranha.utils = {
},
strLength: function (str) {
return str != null ? str.length : 0;
}
},
antiForgery: function () {
Copy link
Member Author

Choose a reason for hiding this comment

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

This method is responsible for extracting the CSRF request token from the current cookies.

}
return "";
},
antiForgeryHeaders: function (isJson) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This method is responsible for setting up API request headers including CSRF anti forgery token sent back to the server. If correct token is not provided in the header actions will return a 400.

@tidyui
Copy link
Member Author

tidyui commented Oct 23, 2021

@tedvanderveen Do you see any conflicts with this in regards to the stuff you've done for external authentication?

@tedvanderveen
Copy link
Contributor

@tidyui I shall pick this up next week!

@tidyui
Copy link
Member Author

tidyui commented Oct 23, 2021

@tedvanderveen Great! The main update here when using an external authentication is that after login the request should be redirected to ~/manager/login/auth instead of ~/manager. This new route sets up the CSRF cookie that the client will need when making API-calls back to the server.

@tidyui
Copy link
Member Author

tidyui commented Oct 25, 2021

Merging this as it's blocking other pull requests and we're on a tight schedule :) If you have any comments regarding the implementation we can always address it in master.

@tidyui tidyui merged commit e42abac into master Oct 25, 2021
@tidyui tidyui deleted the features/manager-security-update branch October 25, 2021 19:47
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

2 participants