-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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 safari oidc login #11884
Merged
Merged
UI/fix safari oidc login #11884
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
690239e
use window.postMessage instead of localStorage on oidc callback
hashishaw f5d8193
WIP // skipped tests passing
hashishaw eb61de1
Clean up auth-jwt logic, add origin check
hashishaw f30d263
WIP // clean up tests, most are passing
hashishaw 4e79fb8
Fix last test
hashishaw 9c6e783
Add settled so we don't miss a bad handler call
hashishaw f2ad947
Cleanup
hashishaw e869446
Disable login button if isLoading
hashishaw a71de46
Address comments
hashishaw 2876883
Add changelog
hashishaw File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
```release-note:bug | ||
ui: fix oidc login with Safari | ||
``` |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,12 @@ import { ERROR_WINDOW_CLOSED, ERROR_MISSING_PARAMS, ERROR_JWT_LOGIN } from 'vaul | |
|
||
const component = create(form); | ||
const windows = []; | ||
const buildMessage = opts => ({ | ||
isTrusted: true, | ||
origin: 'https://my-vault.com', | ||
data: {}, | ||
...opts, | ||
}); | ||
const fakeWindow = EmberObject.extend(Evented, { | ||
init() { | ||
this._super(...arguments); | ||
|
@@ -29,12 +35,7 @@ const fakeWindow = EmberObject.extend(Evented, { | |
width: 500, | ||
}; | ||
}), | ||
localStorage: computed(function() { | ||
return { | ||
getItem: sinon.stub(), | ||
removeItem: sinon.stub(), | ||
}; | ||
}), | ||
origin: 'https://my-vault.com', | ||
closed: false, | ||
}); | ||
|
||
|
@@ -70,7 +71,6 @@ const renderIt = async (context, path = 'jwt') => { | |
context.set('handler', sinon.spy(handler)); | ||
context.set('roleName', ''); | ||
context.set('selectedAuthPath', path); | ||
|
||
await render(hbs` | ||
<AuthJwt | ||
@window={{window}} | ||
|
@@ -135,7 +135,7 @@ module('Integration | Component | auth jwt', function(hooks) { | |
assert.equal(component.yieldContent, 'Hello!', 'yields properly'); | ||
}); | ||
|
||
test('jwt: it renders', async function(assert) { | ||
test('jwt: it renders and makes auth_url requests', async function(assert) { | ||
await renderIt(this); | ||
await settled(); | ||
assert.ok(component.jwtPresent, 'renders jwt field'); | ||
|
@@ -203,56 +203,88 @@ module('Integration | Component | auth jwt', function(hooks) { | |
assert.equal(this.error, ERROR_WINDOW_CLOSED, 'calls onError with error string'); | ||
}); | ||
|
||
test('oidc: storage event fires without state key', async function(assert) { | ||
test('oidc: shows error when message posted with state key, wrong params', async function(assert) { | ||
await renderIt(this); | ||
this.set('selectedAuthPath', 'foo'); | ||
await component.role('test'); | ||
component.login(); | ||
await waitUntil(() => { | ||
return this.openSpy.calledOnce; | ||
}); | ||
this.window.localStorage.getItem.returns(null); | ||
this.window.trigger('storage', { storageArea: this.window.localStorage }); | ||
this.window.trigger('message', buildMessage({ data: { state: 'state', foo: 'bar' } })); | ||
run.cancelTimers(); | ||
assert.ok(this.window.localStorage.getItem.calledOnce, 'calls getItem'); | ||
assert.notOk(this.window.localStorage.removeItem.called, 'never calls removeItem'); | ||
assert.equal(this.error, ERROR_MISSING_PARAMS, 'calls onError with params missing error'); | ||
}); | ||
|
||
test('oidc: storage event fires with state key, wrong params', async function(assert) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed this test because we don't fire en event |
||
test('oidc: storage event fires with state key, correct params', async function(assert) { | ||
await renderIt(this); | ||
this.set('selectedAuthPath', 'foo'); | ||
await component.role('test'); | ||
component.login(); | ||
await waitUntil(() => { | ||
return this.openSpy.calledOnce; | ||
}); | ||
this.window.localStorage.getItem.returns(JSON.stringify({})); | ||
this.window.trigger('storage', { storageArea: this.window.localStorage }); | ||
this.window.trigger( | ||
'message', | ||
buildMessage({ | ||
data: { | ||
path: 'foo', | ||
state: 'state', | ||
code: 'code', | ||
}, | ||
}) | ||
); | ||
await settled(); | ||
assert.equal(this.selectedAuth, 'token', 'calls onSelectedAuth with token'); | ||
assert.equal(this.token, 'token', 'calls onToken with token'); | ||
assert.ok(this.handler.calledOnce, 'calls the onSubmit handler'); | ||
}); | ||
|
||
test('oidc: fails silently when event origin does not match window origin', async function(assert) { | ||
await renderIt(this); | ||
this.set('selectedAuthPath', 'foo'); | ||
await component.role('test'); | ||
component.login(); | ||
await waitUntil(() => { | ||
return this.openSpy.calledOnce; | ||
}); | ||
this.window.trigger( | ||
'message', | ||
buildMessage({ | ||
origin: 'http://hackerz.com', | ||
data: { | ||
path: 'foo', | ||
state: 'state', | ||
code: 'code', | ||
}, | ||
}) | ||
); | ||
run.cancelTimers(); | ||
assert.ok(this.window.localStorage.getItem.calledOnce, 'calls getItem'); | ||
assert.ok(this.window.localStorage.removeItem.calledOnce, 'calls removeItem'); | ||
assert.equal(this.error, ERROR_MISSING_PARAMS, 'calls onError with params missing error'); | ||
await settled(); | ||
assert.notOk(this.handler.called, 'should not call the submit handler'); | ||
}); | ||
|
||
test('oidc: storage event fires with state key, correct params', async function(assert) { | ||
test('oidc: fails silently when event is not trusted', async function(assert) { | ||
await renderIt(this); | ||
this.set('selectedAuthPath', 'foo'); | ||
await component.role('test'); | ||
component.login(); | ||
await waitUntil(() => { | ||
return this.openSpy.calledOnce; | ||
}); | ||
this.window.localStorage.getItem.returns( | ||
JSON.stringify({ | ||
path: 'foo', | ||
state: 'state', | ||
code: 'code', | ||
this.window.trigger( | ||
'message', | ||
buildMessage({ | ||
isTrusted: false, | ||
data: { | ||
path: 'foo', | ||
state: 'state', | ||
code: 'code', | ||
}, | ||
}) | ||
); | ||
this.window.trigger('storage', { storageArea: this.window.localStorage }); | ||
run.cancelTimers(); | ||
await settled(); | ||
assert.equal(this.selectedAuth, 'token', 'calls onSelectedAuth with token'); | ||
assert.equal(this.token, 'token', 'calls onToken with token'); | ||
assert.ok(this.handler.calledOnce, 'calls the onSubmit handler'); | ||
assert.notOk(this.handler.called, 'should not call the submit handler'); | ||
}); | ||
}); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this because once the popup is closed, while things are processing but before authenticate runs, the button is clickable again and it was confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smart