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.

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

Support for Neo4j 4.4 SSO, including discovery and refresh tokens #1581

Merged
merged 30 commits into from
Nov 2, 2021

Conversation

OskarDamkjaer
Copy link
Contributor

No description provided.

if (SSOProviders) {
return Rx.Observable.fromPromise(
(async function() {
//TODO can throw?
Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, if the handleRefreshingToken throws, we should bail out of the reconnection attempts, and go straight to the connection lost state. TBD is whether we should specifically report this back to the user somehow, possible with custom error text?

requestedUseDb: action.requestedUseDb,
host: `${protocol ? `${protocol}//` : ''}${host}`,
supportsMultiDb: !!action.requestedUseDb,
encrypted: action.encrypted,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a bit fuzzy on while all these discovered properties are needed, and whether they're actually used anywhere, but I guess it's fine for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this turned into a bit of a rabbit hole trying to understand where they're all used. I'm somewhat certain that the encrypted key is not used here and that the restApi is used for the :http command. But didn't have enough time to dig into it fully

expect(discoveryData?.urlMissing).toEqual(false)
})

test('finds and priotises sso providers from session storage/connect form when all discovery sources are present, but doesnt merge when hosts differ', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should probably add the new discoveryConnection source to this test?

@OskarDamkjaer OskarDamkjaer force-pushed the connect-discovery branch 2 times, most recently from a304c09 to 403e452 Compare November 2, 2021 15:15
Copy link
Collaborator

@jharris4 jharris4 left a comment

Choose a reason for hiding this comment

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

This is working really well in testing so far, so I'm going to merge it so we can have a release candidate.

We may want to address some of the minor remaining review comments later though...

@jharris4 jharris4 merged commit 3a50d72 into neo4j:master Nov 2, 2021
@OskarDamkjaer OskarDamkjaer changed the title Update SSO to use 4.4 discovery and refresh tokens Support for Neo4j 4.4 SSO, including discovery and refresh tokens Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants