Merge "Fix race condition when linking to specific line"
diff --git a/java/com/google/gerrit/httpd/ProjectOAuthFilter.java b/java/com/google/gerrit/httpd/ProjectOAuthFilter.java
index 9d58d09..8645f9e 100644
--- a/java/com/google/gerrit/httpd/ProjectOAuthFilter.java
+++ b/java/com/google/gerrit/httpd/ProjectOAuthFilter.java
@@ -36,6 +36,7 @@
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.account.AuthRequest;
import com.google.gerrit.server.account.AuthResult;
+import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -155,7 +156,6 @@
return true;
}
}
- authRequest = authRequestFactory.createForExternalUser(authInfo.username);
Optional<AccountState> who =
accountCache.getByUsername(authInfo.username).filter(a -> a.account().isActive());
if (!who.isPresent()) {
@@ -167,6 +167,16 @@
}
Account account = who.get().account();
+ Optional<ExternalId> extId =
+ who.get().externalIds().stream()
+ .filter(e -> e.key().scheme().equals(authInfo.exportName))
+ .findAny();
+ if (extId.isPresent()) {
+ authRequest = authRequestFactory.create(extId.get().key());
+ authRequest.setUserName(authInfo.username);
+ } else {
+ authRequest = authRequestFactory.createForExternalUser(authInfo.username);
+ }
authRequest.setEmailAddress(account.preferredEmail());
authRequest.setDisplayName(account.fullName());
authRequest.setPassword(authInfo.tokenOrSecret);
diff --git a/java/com/google/gerrit/server/account/AuthRequest.java b/java/com/google/gerrit/server/account/AuthRequest.java
index 03c6631..bb7ee97 100644
--- a/java/com/google/gerrit/server/account/AuthRequest.java
+++ b/java/com/google/gerrit/server/account/AuthRequest.java
@@ -25,6 +25,7 @@
import com.google.gerrit.server.account.externalids.ExternalIdKeyFactory;
import com.google.inject.Inject;
import com.google.inject.Singleton;
+import java.util.Objects;
import java.util.Optional;
/**
@@ -215,4 +216,37 @@
public void setActive(Boolean isActive) {
this.active = isActive;
}
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(
+ active,
+ authPlugin,
+ authProvider,
+ authProvidesAccountActiveStatus,
+ displayName,
+ emailAddress,
+ externalId,
+ password,
+ skipAuthentication,
+ userName);
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj) return true;
+ if (obj == null) return false;
+ if (!(obj instanceof AuthRequest)) return false;
+ AuthRequest other = (AuthRequest) obj;
+ return active == other.active
+ && Objects.equals(authPlugin, other.authPlugin)
+ && Objects.equals(authProvider, other.authProvider)
+ && authProvidesAccountActiveStatus == other.authProvidesAccountActiveStatus
+ && Objects.equals(displayName, other.displayName)
+ && Objects.equals(emailAddress, other.emailAddress)
+ && Objects.equals(externalId, other.externalId)
+ && Objects.equals(password, other.password)
+ && skipAuthentication == other.skipAuthentication
+ && Objects.equals(userName, other.userName);
+ }
}
diff --git a/javatests/com/google/gerrit/httpd/ProjectOAuthFilterTest.java b/javatests/com/google/gerrit/httpd/ProjectOAuthFilterTest.java
index 3ce35de..7f8e2f1 100644
--- a/javatests/com/google/gerrit/httpd/ProjectOAuthFilterTest.java
+++ b/javatests/com/google/gerrit/httpd/ProjectOAuthFilterTest.java
@@ -37,7 +37,9 @@
import com.google.gerrit.server.account.AuthRequest;
import com.google.gerrit.server.account.AuthResult;
import com.google.gerrit.server.account.externalids.ExternalId;
+import com.google.gerrit.server.account.externalids.ExternalIdFactory;
import com.google.gerrit.server.account.externalids.ExternalIdKeyFactory;
+import com.google.gerrit.server.account.externalids.storage.notedb.ExternalIdFactoryNoteDbImpl;
import com.google.gerrit.server.config.AuthConfig;
import com.google.gerrit.util.http.testutil.FakeHttpServletRequest;
import com.google.gerrit.util.http.testutil.FakeHttpServletResponse;
@@ -72,11 +74,7 @@
String.format("%s:%s", AUTH_USER, OAUTH_TOKEN).getBytes(StandardCharsets.UTF_8));
private static final OAuthUserInfo OAUTH_USER_INFO =
new OAuthUserInfo(
- String.format("oauth-test:%s", AUTH_USER),
- AUTH_USER,
- "johndoe@example.com",
- "John Doe",
- null);
+ String.format("test:%s", AUTH_USER), AUTH_USER, "johndoe@example.com", "John Doe", null);
@Mock private DynamicItem<WebSession> webSessionItem;
@@ -100,6 +98,7 @@
private FakeHttpServletRequest req;
private HttpServletResponse res;
private AuthResult authSuccessful;
+ private ExternalIdFactory extIdFactory;
private ExternalIdKeyFactory extIdKeyFactory;
private AuthRequest.Factory authRequestFactory;
private Config gerritConfig = new Config();
@@ -110,6 +109,7 @@
res = new FakeHttpServletResponse();
extIdKeyFactory = new ExternalIdKeyFactory(new ExternalIdKeyFactory.ConfigImpl(authConfig));
+ extIdFactory = new ExternalIdFactoryNoteDbImpl(extIdKeyFactory, authConfig);
authRequestFactory = new AuthRequest.Factory(extIdKeyFactory);
authSuccessful =
@@ -288,14 +288,22 @@
oAuthFilter.init(null);
oAuthFilter.doFilter(req, res, chain);
- verify(accountManager).authenticate(any());
+ AuthRequest expected =
+ authRequestFactory.create(
+ ExternalIdKeyFactory.parse(OAUTH_USER_INFO.getExternalId(), false));
+ expected.setUserName(OAUTH_USER_INFO.getUserName());
+ expected.setPassword(OAUTH_TOKEN);
+ expected.setAuthPlugin("oauth");
+ expected.setAuthProvider("test");
+
+ verify(accountManager).authenticate(eq(expected));
verify(chain, never()).doFilter(any(), any());
assertThat(res.getStatus()).isEqualTo(HttpServletResponse.SC_UNAUTHORIZED);
}
private void initAccount() throws Exception {
- initAccount(ImmutableSet.of());
+ initAccount(ImmutableSet.of(extIdFactory.create("test", AUTH_USER, AUTH_ACCOUNT_ID)));
}
private void initAccount(Collection<ExternalId> extIds) throws Exception {
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
index d77b420..e4ddfd1 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts
@@ -114,6 +114,7 @@
import {pluginLoaderToken} from '../../shared/gr-js-api-interface/gr-plugin-loader';
import {modalStyles} from '../../../styles/gr-modal-styles';
import {subscribe} from '../../lit/subscription-controller';
+import {chatModelToken} from '../../../models/chat/chat-model';
import {userModelToken} from '../../../models/user/user-model';
import {ParsedChangeInfo} from '../../../types/types';
import {configModelToken} from '../../../models/config/config-model';
@@ -500,6 +501,10 @@
@state() threadsWithUnappliedSuggestions?: CommentThread[];
+ @state() chatCapabilitiesLoaded = false;
+
+ private aiChatLoadingCleanup?: () => void;
+
private readonly restApiService = getAppContext().restApiService;
private readonly reporting = getAppContext().reportingService;
@@ -508,6 +513,8 @@
private readonly getPluginLoader = resolve(this, pluginLoaderToken);
+ private readonly getChatModel = resolve(this, chatModelToken);
+
private readonly getUserModel = resolve(this, userModelToken);
private readonly getConfigModel = resolve(this, configModelToken);
@@ -524,6 +531,11 @@
super();
subscribe(
this,
+ () => this.getChatModel().capabilitiesLoaded$,
+ x => (this.chatCapabilitiesLoaded = x)
+ );
+ subscribe(
+ this,
() => this.getChangeModel().latestPatchNum$,
x => (this.latestPatchNum = x)
);
@@ -912,6 +924,15 @@
this.menuActions = this.computeMenuActions();
}
+ override updated(changedProperties: PropertyValues) {
+ super.updated(changedProperties);
+
+ if (this.chatCapabilitiesLoaded && this.aiChatLoadingCleanup) {
+ this.aiChatLoadingCleanup();
+ this.aiChatLoadingCleanup = undefined;
+ }
+ }
+
reload() {
if (!this.changeNum || !this.latestPatchNum || !this.change) {
return Promise.resolve();
@@ -1517,6 +1538,15 @@
break;
}
case AI_CHAT_ACTION.__key: {
+ if (!this.chatCapabilitiesLoaded) {
+ try {
+ this.aiChatLoadingCleanup =
+ this.setLoadingOnButtonWithKey(AI_CHAT_ACTION);
+ } catch (e) {
+ // This is expected if the button is not found in the DOM.
+ }
+ return;
+ }
fire(this, 'ai-chat', {});
break;
}
diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
index d7b1997..d858d84 100644
--- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions_test.ts
@@ -64,6 +64,7 @@
ChangeModel,
changeModelToken,
} from '../../../models/change/change-model';
+import {ChatModel, chatModelToken} from '../../../models/chat/chat-model';
import {GrAutogrowTextarea} from '../../shared/gr-autogrow-textarea/gr-autogrow-textarea';
// TODO(dhruvsri): remove use of _populateRevertMessage as it's private
@@ -72,6 +73,7 @@
let navigateResetStub: SinonStubbedMember<
ChangeModel['navigateToChangeResetReload']
>;
+ let chatModel: ChatModel;
suite('basic tests', () => {
setup(async () => {
@@ -140,6 +142,7 @@
testResolver(changeModelToken),
'navigateToChangeResetReload'
);
+ chatModel = testResolver(chatModelToken);
await element.updateComplete;
await element.reload();
@@ -345,6 +348,36 @@
element.revisionActions = {};
assert.isFalse(await isLoading());
});
+
+ test('chatCapabilitiesLoaded', async () => {
+ stubFlags('isEnabled').returns(true);
+ element.aiPluginsRegistered = true;
+ chatModel.updateState({
+ models: undefined,
+ actions: undefined,
+ modelsLoadingError: undefined,
+ actionsLoadingError: undefined,
+ });
+ element.requestUpdate();
+ await element.updateComplete;
+ const chatButton = queryAndAssert(
+ element,
+ 'gr-button[data-action-key="chat"]'
+ );
+ assert.isFalse(chatButton.hasAttribute('loading'));
+
+ (chatButton as HTMLElement).click();
+ await element.updateComplete;
+ assert.isTrue(chatButton.hasAttribute('loading'));
+
+ chatModel.updateState({
+ models: {models: [], default_model_id: 'foo'},
+ actions: {actions: [], default_action_id: 'bar'},
+ });
+ element.requestUpdate();
+ await element.updateComplete;
+ assert.isFalse(chatButton.hasAttribute('loading'));
+ });
});
test('show-revision-actions event should fire', async () => {
diff --git a/polygerrit-ui/app/elements/chat-panel/splash-page.ts b/polygerrit-ui/app/elements/chat-panel/splash-page.ts
index ee3eaa8..f7cf759 100644
--- a/polygerrit-ui/app/elements/chat-panel/splash-page.ts
+++ b/polygerrit-ui/app/elements/chat-panel/splash-page.ts
@@ -41,6 +41,8 @@
@state() documentationUrl?: string;
+ @state() capabilitiesLoaded = false;
+
@property({type: Boolean}) isChangePrivate = false;
private readonly getChatModel = resolve(this, chatModelToken);
@@ -76,6 +78,11 @@
);
subscribe(
this,
+ () => this.getChatModel().capabilitiesLoaded$,
+ x => (this.capabilitiesLoaded = x)
+ );
+ subscribe(
+ this,
() => this.getUserModel().account$,
x => (this.account = x)
);
@@ -226,6 +233,7 @@
}
private renderContent() {
+ if (!this.capabilitiesLoaded) return '';
if (this.isChangePrivate) {
return html`
<div class="background-request-container">
diff --git a/polygerrit-ui/app/models/chat/chat-model.ts b/polygerrit-ui/app/models/chat/chat-model.ts
index 1630862..21528d6 100644
--- a/polygerrit-ui/app/models/chat/chat-model.ts
+++ b/polygerrit-ui/app/models/chat/chat-model.ts
@@ -315,6 +315,14 @@
chatState => chatState.errorMessage
);
+ readonly capabilitiesLoaded$: Observable<boolean> = select(
+ this.state$,
+ state =>
+ !!state.modelsLoadingError ||
+ !!state.actionsLoadingError ||
+ (!!state.models && !!state.actions)
+ );
+
readonly userInput$: Observable<string> = select(
this.state$,
chatState => chatState.draftUserMessage.content
@@ -342,7 +350,8 @@
});
this.pluginsModel.aiCodeReviewPlugins$.subscribe(
- plugins => (this.plugin = plugins[0].provider)
+ plugins =>
+ (this.plugin = plugins.length > 0 ? plugins[0].provider : undefined)
);
this.filesModel.files$.subscribe(files => (this.files = files ?? []));
this.changeModel.change$.subscribe(change => {
