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.

Sign up for GitHub

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

Jump to bottom

adjust routers in openid module #7528

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

adjust routers in openid module #7528

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
31243c3
adjust routers in openid module
infofromca Jan 22, 2017
39951db
adjust routers in openid module-good formate
infofromca Jan 22, 2017
17bd093
Merge branch 'dev' into openidRoute
BenedekFarkas Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/Orchard.Web/Modules/Orchard.OpenId/Controllers/AccountController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ public AccountController(
public ILogger Logger { get; set; }
public Localizer T { get; set; }

[AlwaysAccessible]
[HttpGet]
public ActionResult LogOn() {
if (Request.IsAuthenticated) {
Expand All @@ -68,11 +69,12 @@ public ActionResult LogOn(string userNameOrEmail, string password, string return
}

var membershipSettings = _membershipService.GetSettings();

if (user != null &&
membershipSettings.EnableCustomPasswordPolicy &&
membershipSettings.EnablePasswordExpiration &&
_membershipService.PasswordIsExpired(user, membershipSettings.PasswordExpirationTimeInDays)) {
return RedirectToAction("ChangeExpiredPassword", new { username = user.UserName });
return RedirectToAction("ChangeExpiredPassword", "Account", new { Area = "Orchard.Users", username = user.UserName });
Copy link
Member

Choose a reason for hiding this comment

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

In the same PR you change this to the Users module, but you also create routes for the OpenId module, on the same Controller/Action names, is that on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will redirect action to ChangeExpiredPassword in Orchard.Users because there is no ChangeExpiredPassword in opened module.

The purpose of the routes will redirect other action.(and also consistent to original code)

For this specific ChangeExpiredPassword action, considerate both of the above, it will redirect the ChangeExpiredPassword in Orchard.Users module with my test.

Copy link
Contributor

Choose a reason for hiding this comment

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

But if the user is logged in with one of the OpenId providers, we can't change his/her password, because we don't own it!
So I think we need to implement custom logic to detect whether the user is logged in using a local account and then redirect to the change password action.

}

_authenticationService.SignIn(user, rememberMe);
Expand Down
54 changes: 53 additions & 1 deletion src/Orchard.Web/Modules/Orchard.OpenId/Routes/OpenId.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
using Orchard.Mvc.Routes;
using Orchard.OpenId.Services;

namespace Orchard.Azure.Authentication {
namespace Orchard.OpenId.Routes {
public class OpenIdRoutes : IRouteProvider {
private readonly IEnumerable<IOpenIdProvider> _openIdProviders;

Expand Down Expand Up @@ -43,6 +43,57 @@ public IEnumerable<RouteDescriptor> GetRoutes() {
},
new RouteDescriptor {
Priority = 10,
Route = new Route(
"Users/Account/LogOn",
Copy link
Member

Choose a reason for hiding this comment

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

Is the goal to replace the one from Users ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Contributor

Choose a reason for hiding this comment

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

No need to replace the routes, once the user sets the settings correctly of one of the enabled OpenId features the app will register the routes automatically after the first restart.
This was intended, no need to override the routes always, only override when the replacement is valid.
I think a more proper solution will be notifying the user that an app restart is required as @sebastienros earlier suggested.

new RouteValueDictionary {
{"area", "Orchard.OpenId"},
{"controller", "Account"},
{"action","LogOn" }
},
new RouteValueDictionary(),
new RouteValueDictionary {
{"area", "Orchard.OpenId"},
{"controller", "Account"},
{"action","LogOn" }
},
new MvcRouteHandler())
},
new RouteDescriptor {
Priority = 10,
Route = new Route(
"Users/Account/LogOff",
new RouteValueDictionary {
{"area", "Orchard.OpenId"},
{"controller", "Account"},
{"action","LogOff" }
},
new RouteValueDictionary(),
new RouteValueDictionary {
{"area", "Orchard.OpenId"},
{"controller", "Account"},
{"action","LogOff" }
},
new MvcRouteHandler())
},
new RouteDescriptor {
Priority = 10,
Route = new Route(
"Users/Account/AccessDenied",
new RouteValueDictionary {
{"area", "Orchard.OpenId"},
{"controller", "Account"},
{"action","AccessDenied" }
},
new RouteValueDictionary(),
new RouteValueDictionary {
{"area", "Orchard.OpenId"},
{"controller", "Account"},
{"action","AccessDenied" }
},
new MvcRouteHandler())
},
new RouteDescriptor {
Priority = -20,
Route = new Route(
"Users/Account/{action}",
new RouteValueDictionary {
Expand All @@ -56,6 +107,7 @@ public IEnumerable<RouteDescriptor> GetRoutes() {
},
new MvcRouteHandler())
},

new RouteDescriptor {
Priority = 10,
Route = new Route(
Expand Down