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

UI: Fix OIDC login in fullscreen #19071

Merged
merged 3 commits into from
Feb 8, 2023
Merged

UI: Fix OIDC login in fullscreen #19071

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ff121c3
Remove later runloop from auth-jwt
hashishaw Feb 8, 2023
7f28b9b
Add changelog
hashishaw Feb 8, 2023
cec1ff7
Update auth-jwt tests
hashishaw Feb 8, 2023
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
34 changes: 17 additions & 17 deletions ui/app/components/auth-jwt.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import Ember from 'ember';
import { inject as service } from '@ember/service';
// ARG NOTE: Once you remove outer-html after glimmerizing you can remove the outer-html component
import Component from './outer-html';
import { later } from '@ember/runloop';
import { task, timeout, waitForEvent } from 'ember-concurrency';
import { computed } from '@ember/object';
import { waitFor } from '@ember/test-waiters';
Expand Down Expand Up @@ -76,6 +75,17 @@ export default Component.extend({
})
).restartable(),

cancelLogin(oidcWindow, errorMessage) {
this.closeWindow(oidcWindow);
this.handleOIDCError(errorMessage);
},

closeWindow(oidcWindow) {
this.watchPopup.cancelAll();
this.watchCurrent.cancelAll();
oidcWindow.close();
},

handleOIDCError(err) {
this.onLoading(false);
this.prepareForOIDC.cancelAll();
Expand All @@ -94,10 +104,7 @@ export default Component.extend({
// ensure that postMessage event is from expected source
while (true) {
const event = yield waitForEvent(thisWindow, 'message');
if (event.origin !== thisWindow.origin || !event.isTrusted) {
return this.handleOIDCError();
}
if (event.data.source === 'oidc-callback') {
if (event.data.source === 'oidc-callback' && event.isTrusted && event.origin === thisWindow.origin) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I'm in here, pulled in the change from ##18521

return this.exchangeOIDC.perform(event.data, oidcWindow);
}
// continue to wait for the correct message
Expand All @@ -119,12 +126,6 @@ export default Component.extend({
oidcWindow.close();
}),

closeWindow(oidcWindow) {
this.watchPopup.cancelAll();
this.watchCurrent.cancelAll();
oidcWindow.close();
},

exchangeOIDC: task(function* (oidcState, oidcWindow) {
if (oidcState === null || oidcState === undefined) {
return;
Expand All @@ -145,12 +146,8 @@ export default Component.extend({
}
}

// defer closing of the window, but continue executing the task
later(() => {
this.closeWindow(oidcWindow);
}, WAIT_TIME);
Comment on lines -149 to -151
Copy link
Contributor

@zofskeez zofskeez Feb 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm just wondering why this pause was needed in the first place and if there might be a case where removing it will cause a regression? Or was the timeout the problem in the first place?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, if we don't need it then the WAIT_TIME variable can also be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The WAIT_TIME variable is used other places 👍

My suspicion is that we wanted to close the window no matter the outcome of the next request, so wrapping this in a later loop prevented duplicative code. I tested that the window closes if the adapter method errors out and tried to handle all cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha thanks!

if (!path || !state || !code) {
return this.handleOIDCError(ERROR_MISSING_PARAMS);
return this.cancelLogin(oidcWindow, ERROR_MISSING_PARAMS);
}
const adapter = this.store.adapterFor('auth-method');
this.onNamespace(namespace);
Expand All @@ -159,8 +156,11 @@ export default Component.extend({
// and submit auth form
try {
resp = yield adapter.exchangeOIDC(path, state, code);
this.closeWindow(oidcWindow);
} catch (e) {
return this.handleOIDCError(e);
// If there was an error on Vault's end, close the popup
// and show the error on the login screen
return this.cancelLogin(oidcWindow, e);
}
yield this.onSubmit(null, null, resp.auth.client_token);
}),
Expand Down