Skip to content
This repository has been archived by the owner on Nov 9, 2017. It is now read-only.

Fix redirect to login page when using single open id provider authentication #579

Closed
wants to merge 3 commits into from
Closed

Fix redirect to login page when using single open id provider authentication #579

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
8a3d899
Fix redirect to login page when using single open id provider authent…
Sep 4, 2014
48da2ea
Code review updates.
Sep 4, 2014
a9ba559
Rename methods and change comments.
Sep 4, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 26 additions & 0 deletions zanata-war/src/main/java/org/zanata/action/LoginAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import org.jboss.seam.annotations.In;
import org.jboss.seam.annotations.Name;
import org.jboss.seam.annotations.Scope;
import org.jboss.seam.security.Identity;
import org.zanata.ApplicationConfiguration;
import org.zanata.security.AuthenticationManager;
import org.zanata.security.AuthenticationType;
Expand All @@ -50,6 +51,9 @@
public class LoginAction implements Serializable {
private static final long serialVersionUID = 1L;

@In
private Identity identity;

@In
private ZanataCredentials credentials;

Expand Down Expand Up @@ -151,4 +155,26 @@ public static OpenIdProviderType getBestSuitedProvider(String openId) {
return OpenIdProviderType.Generic;
}
}

/**
* This method is executed when accessing the login page. Depending on
* current session state, it might indicate to redirect to a different
* location. For example, if the user is already logged in, it will indicate
* to redirect to the dashboard.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the first line should give a reasonable summary, before giving all the details. e.g.

/**
 * Indicates which page the user should be directed to when trying to log in.
 * ...

*
* @return A string indicating where the user should be redirected when
* trying to access the login page.
*/
public String onLoginPageAccessed() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This name makes this look like an event handler, which suggests that it would perform some action, but all it does is return a String. Its purpose is to provide an identifier for which page to redirect to, so why not getLoginPageId?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't like that because Id is such an overloaded term. Also, in the case of open id, it's actually performing the authentication too. How about getLoginPageRedirect?

Copy link
Contributor

Choose a reason for hiding this comment

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

getLoginPageRedirect sounds fine to me.

if (identity.isLoggedIn()) {
return "dashboard";
}
if (applicationConfiguration.isOpenIdAuth()
&& applicationConfiguration.isSingleOpenIdProvider()) {
// go directly to the provider's login page
return genericOpenIdLogin(applicationConfiguration
.getOpenIdProviderUrl());
}
return "login";
}
}
6 changes: 3 additions & 3 deletions zanata-war/src/main/webapp/WEB-INF/pages.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,9 @@
value="no-cache, no-store, max-age=0, must-revalidate" />
<param name="continue" value="#{userRedirect.encodedUrl}" />

<action execute="#{identity.isLoggedIn()}" />
<navigation from-action="#{identity.isLoggedIn()}">
<rule if-outcome="true">
<action execute="#{loginAction.onLoginPageAccessed()}" />
<navigation from-action="#{loginAction.onLoginPageAccessed()}">
<rule if-outcome="dashboard">
<redirect view-id="/dashboard/home.xhtml" />
</rule>
</navigation>
Expand Down
2 changes: 1 addition & 1 deletion zanata-war/src/main/webapp/WEB-INF/template/banner.xhtml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@

<h:form rendered="#{applicationConfigurationAction.singleOpenId}">
<h:commandLink id="openid_single_signin_link"
action="#{loginAction.genericOpenIdLogin(applicationConfiguration.openIdProviderUrl)}"
action="#{loginAction.goToLoginPage()}"
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this was missed when renaming goToLoginPage().

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good catch.

propagation="none" styleClass="l--push-left-half button--primary">
#{messages['jsf.Login']}
</h:commandLink>
Expand Down