Skip to content

Commit

Permalink
Improvement to the is-real-login-error or just-needing-to-reauthentic…
Browse files Browse the repository at this point in the history 
…ate logic for #109.
  • Loading branch information
@groboclown
groboclown committed Mar 24, 2016
1 parent a23c9e5 commit 284813f
Show file tree
Hide file tree
Showing 6 changed files with 107 additions and 92 deletions.
15 changes: 15 additions & 0 deletions CHANGES.md
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,20 @@
# IDEA Community VCS Integration for Perforce

## ::v0.7.11::

### Overview

* Bug fixes.

### Details

* Bug fixes.
* Aleviated some of the issues surrounding the plugin
constantly reporting that the password is invalid.
(#109)



## ::v0.7.10::

### Overview
Expand Down
19 changes: 7 additions & 12 deletions plugin/META-INF/plugin.xml
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
<idea-plugin version="2">
<name>Perforce IDEA Community Integration</name>
<id>PerforceIC</id>
<version>0.7.10</version>
<version>0.7.11</version>
<idea-version since-build="IC-135.1286"/>
<category>VCS Integration</category>
<change-notes><![CDATA[
<ol>
<li><em>0.7.11</em>
<ol>
<li>Aleviated some of the issues surrounding the plugin
constantly reporting that the password is invalid.</li>
</ol>
</li>
<li><em>0.7.9 &amp; 0.7.10</em>
<ol>
<li>Android Studio and other non-JRE 8 IDEs can
Expand All @@ -14,17 +20,6 @@
compatibility libraries.</li>
</ol>
</li>
<li><em>0.7.8</em>
<ol>
<li>Initial project VCS setup no longer
causes "OK" and "Apply"
buttons to not react.</li>
<li>Initial project loading in Android Studio 1.5.1 with a master password
store no longer causes the project to deadlock on startup.</li>
<li>Better handling of false authentication failures
reported by the server.</li>
</ol>
</li>
</ol>
]]></change-notes>
<description><![CDATA[
Expand Down
56 changes: 0 additions & 56 deletions plugin/src/net/groboclown/idea/p4ic/server/VcsExceptionUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,9 @@
package net.groboclown.idea.p4ic.server;

import com.intellij.openapi.diagnostic.Logger;
import com.intellij.openapi.vcs.VcsConnectionProblem;
import com.perforce.p4java.exception.AccessException;
import com.perforce.p4java.exception.ConnectionException;
import com.perforce.p4java.exception.P4JavaError;
import com.perforce.p4java.exception.P4JavaException;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

import java.io.IOException;
import java.net.URISyntaxException;
import java.util.concurrent.CancellationException;
import java.util.concurrent.TimeoutException;

Expand All @@ -48,55 +41,6 @@ public static void alwaysThrown(@NotNull Throwable t) {
}
}

public static boolean isPasswordWrongMessage(@Nullable String message) {
if (message == null) {
return false;
}
return message.contains("Perforce password (P4PASSWD) invalid or unset.");
}


public static boolean isLoginWrongMessage(@Nullable String message) {
if (message == null) {
return false;
}
return (message.contains("Access for user '") &&
message.contains("' has not been enabled by 'p4 protect'") ||
message.contains("Unable to resolve Perforce server host name '"));
}


public static boolean isDisconnectErrorMessage(@Nullable String message) {
return (message != null && ("Disconnected from Perforce server".equals(message) ||
"Not currently connected to a Perforce server".equals(message) ||
message.contains("Unable to connect to Perforce server at ") ||
message.endsWith("Read timed out") ||
isPasswordWrongMessage(message) ||
isLoginWrongMessage(message)));
}


public static boolean isDisconnectError(@Nullable Throwable t) {
if (t == null) {
return false;
}
if (t instanceof VcsConnectionProblem || t instanceof ConnectionException ||
t instanceof AccessException || t instanceof URISyntaxException) {
return true;
}

if (t.getCause() != null && t.getCause() != t) {
Throwable cause = t.getCause();
if (t instanceof P4JavaException || t instanceof P4JavaError) {
if (cause instanceof IOException) {
return true;
}
}
}

return isDisconnectErrorMessage(t.getMessage());
}


/**
* Does the exception indicate a task cancellation?
Expand Down
21 changes: 18 additions & 3 deletions plugin/src/net/groboclown/idea/p4ic/server/connection/ClientPasswordConnectionHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import com.perforce.p4java.PropertyDefs;
import com.perforce.p4java.exception.AccessException;
import com.perforce.p4java.exception.P4JavaException;
import com.perforce.p4java.option.server.LoginOptions;
import com.perforce.p4java.server.IOptionsServer;
import net.groboclown.idea.p4ic.config.ServerConfig;
import net.groboclown.idea.p4ic.server.ConfigurationProblem;
Expand Down Expand Up @@ -53,6 +52,19 @@ public Properties getConnectionProperties(@NotNull ServerConfig config, @Nullabl
ret.setProperty(PropertyDefs.CLIENT_NAME_KEY, clientName);
}

// We will explicitly log in when ready.
// This key is set to null to mean false. Note that any other
// value means true (even the text "false").
ret.remove(PropertyDefs.AUTO_LOGIN_KEY);

// Turning this flag on means that the server connection
// will attempt to get the client object without having
// logged in first, so we want it turned off.
// This key is set to null to mean false. Note that any other
// value means true (even the text "false").
ret.remove(PropertyDefs.AUTO_CONNECT_KEY);


// This property key doesn't actually seem to do anything.
// A real login is still required.
// ret.setProperty(PropertyDefs.PASSWORD_KEY, new String(password));
Expand Down Expand Up @@ -109,14 +121,17 @@ private AccessException authenticate(@Nullable Project project, @NotNull IOption
// If the password is blank, then there's no need for the
// user to log in; in fact, that wil raise an error by Perforce
try {
server.login(password, new LoginOptions(false, true));
// server.login(password, new LoginOptions(false, true));
server.login(password);
LOG.debug("No issue logging in with stored password");
return null;
} catch (AccessException ex) {
// This seems to happen on a rare occasion on the
// first attempt at logging in. Don't automatically
// forget the password.
LOG.debug("login failed", ex);
if (LOG.isDebugEnabled()) {
LOG.debug("login failed", ex);
}
if (ex.getServerMessage().hasMessageFragment("'login' not necessary")) {
// ignore login and keep going
PasswordManager.getInstance().forgetPassword(project, config);
Expand Down
80 changes: 61 additions & 19 deletions plugin/src/net/groboclown/idea/p4ic/v2/server/connection/AuthenticatedServer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ class AuthenticatedServer {
private static final Logger LOG = Logger.getInstance(AuthenticatedServer.class);
private static final Logger P4LOG = Logger.getInstance("p4");
private static final Lock CONNECT_LOCK = new ReentrantLock();
public static final int MAX_AUTHENTICATION_RETRIES = 3;
public static final int MAX_AUTHENTICATION_RETRIES = 2;

private static final AtomicInteger serverCount = new AtomicInteger(0);
private static final AtomicInteger activeConnectionCount = new AtomicInteger(0);
Expand All @@ -62,8 +62,10 @@ class AuthenticatedServer {
@NotNull
private IOptionsServer server;

private boolean forcedAuthentication = false;
private P4JavaException authenticationException = null;
private boolean hasPassedAuthentication = false;
private boolean hasValidatedAuthentication = false;
private boolean isInvalidLogin = false;

// metrics for debugging
private int loginFailedCount = 0;
Expand All @@ -83,6 +85,9 @@ class AuthenticatedServer {
this.tempDir = tempDir;
this.server = reconnect(project, clientName, connectionHandler,
config, tempDir, serverInstance);

// Note: at this point, the

connectedCount++;
}

Expand All @@ -102,7 +107,16 @@ void disconnect() throws ConnectionException, AccessException {


@NotNull
IOptionsServer getServer() {
IOptionsServer getServer() throws P4JavaException {
if (isInvalidLogin) {
if (authenticationException != null) {
throw new P4JavaException(authenticationException);
}
throw new P4JavaException("invalid login credentials");
}
if (! hasValidatedAuthentication) {
authenticate();
}
return server;
}

Expand All @@ -116,24 +130,45 @@ boolean authenticate() throws P4JavaException {
if (! server.isConnected() || (project != null && project.isDisposed())) {
return false;
}
if (! forcedAuthentication) {
if (LOG.isDebugEnabled()) {
LOG.debug("Attempting to force an authentication for " + this);
}

if (isInvalidLogin) {
LOG.info("Previous login attempts failed. Assuming authentication is invalid.");
return false;
}

if (! hasPassedAuthentication) {
// We have not attempted to authenticate this connection.
isInvalidLogin = true;
hasValidatedAuthentication = false;
forceAuthenticate();

// If the forced authentication fails (throws an
// exception), then the connection is marked as
// invalid, and we will never attempt to
// re-login again.

isInvalidLogin = false;
hasPassedAuthentication = true;
}
if (! testLogin()) {

if (! validateLogin()) {
loginFailedCount++;
if (! hasPassedAuthentication) {

// the forced authentication has passed, but the
// validation failed.

if (! hasValidatedAuthentication) {
// we have never been authenticated by the server,
// and this situation means that we still aren't
// even after a forced authentication. So, we'll
// report this connection as being unauthenticated.
if (LOG.isDebugEnabled()) {
LOG.debug("Assuming actual authentication issue");
}
forcedAuthentication = false;
return false;

// TODO see if this assumption that the authentication is invalid
// is too quick to judge.
// return false;
}

// we have had a valid connection before, so we'll
Expand All @@ -152,7 +187,7 @@ boolean authenticate() throws P4JavaException {
serverInstance);
connectedCount++;
forceAuthenticate();
if (testLogin()) {
if (validateLogin()) {
LOG.info("Authorization successful after " + i +
" unauthorized connections (but a valid one was seen earlier) for " +
this);
Expand All @@ -167,7 +202,7 @@ boolean authenticate() throws P4JavaException {
this);
return false;
}
hasPassedAuthentication = true;
hasValidatedAuthentication = true;
return true;
}

Expand All @@ -181,10 +216,13 @@ private void forceAuthenticate() throws P4JavaException {
try {
connectionHandler.forcedAuthentication(project, server, config, AlertManager.getInstance());
forcedAuthenticationCount++;
forcedAuthentication = true;
} finally {
CONNECT_LOCK.unlock();
}
} catch (P4JavaException e) {
// capture the exception for future use
authenticationException = e;
throw e;
} catch (InterruptedException e) {
throw new P4JavaException(e);
}
Expand All @@ -199,7 +237,7 @@ protected void finalize() throws Throwable {
}


private boolean testLogin()
private boolean validateLogin()
throws ConnectionException, RequestException, AccessException {
try {
// Perform an operation that should succeed, and only fail if the
Expand Down Expand Up @@ -314,6 +352,7 @@ public LogTraceLevel getTraceLevel() {
// an invalid password to cause the server connection
// to never be returned.


LOG.debug("calling defaultAuthentication on " + connectionHandler.getClass().getSimpleName());
connectionHandler.defaultAuthentication(project, server, config);

Expand Down Expand Up @@ -344,10 +383,13 @@ public int hashCode() {
@Override
public String toString() {
return "Server" + serverInstance +
" (lf#: " + loginFailedCount +
", c#: " + connectedCount +
", d#: " + disconnectedCount +
", fa#: " + forcedAuthenticationCount +
" (loginFailed# " + loginFailedCount +
", connected# " + connectedCount +
", disconnected# " + disconnectedCount +
", forcedLogin# " + forcedAuthenticationCount +
", invalidLogin? " + isInvalidLogin +
", passedLogin? " + hasPassedAuthentication +
", validatedLogin? " + hasValidatedAuthentication +
")";
}
}
8 changes: 6 additions & 2 deletions plugin/src/net/groboclown/idea/p4ic/v2/server/connection/ClientExec.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,9 @@ static boolean checkIfOnline(@NotNull Project project, @NotNull ServerConfig con
} catch (IOException e) {
exception = e;
} catch (AccessException e) {
needsAuthentication = true;
if (isPasswordProblem(e)) {
needsAuthentication = true;
}
exception = e;
} catch (P4JavaException e) {
exception = e;
Expand Down Expand Up @@ -393,7 +395,9 @@ private <T> T p4RunFor(@NotNull Project project, @NotNull P4Runner<T> runner, in
}
}

private <T> T p4RunWithSkippedPasswordCheck(@NotNull Project project, @NotNull P4Runner<T> runner, int retryCount)

// TODO experimental synchronized here.
private synchronized <T> T p4RunWithSkippedPasswordCheck(@NotNull Project project, @NotNull P4Runner<T> runner, int retryCount)
throws VcsException, CancellationException, P4JavaException {
// Must check offline status
if (connectedController.isWorkingOffline()) {
Expand Down

0 comments on commit 284813f

Please sign in to comment.