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

Jump to bottom

DRIVERS-2836 OIDC Spec Cleanup #1551

Merged
merged 31 commits into from
Apr 22, 2024
Merged

DRIVERS-2836 OIDC Spec Cleanup #1551

merged 31 commits into from
Apr 22, 2024

Conversation

blink1073
Copy link
Member

Please complete the following before merging:

  • Update changelog.
  • Make sure there are generated JSON files from the YAML test files.
  • Test changes in at least one language driver.
  • Test these changes against all server versions and topologies (including standalone, replica set, sharded
    clusters, and serverless).

@blink1073 blink1073 requested a review from a team as a code owner March 19, 2024 12:51
@blink1073 blink1073 requested review from JamesKovacs and removed request for a team March 19, 2024 12:51
@blink1073
Copy link
Member Author

mongodb/mongo-python-driver#1556

@blink1073 blink1073 marked this pull request as draft March 19, 2024 16:52
@blink1073
Copy link
Member Author

I'm planning to add one additional prose test suggested by @pmeredit, "in human flow, if there is no cached access token but there is a refresh token, that the refresh token is passed to the callback."

@blink1073 blink1073 requested review from pmeredit and removed request for JamesKovacs March 20, 2024 01:32
pmeredit
pmeredit approved these changes Mar 21, 2024
View reviewed changes
Copy link
Contributor

@pmeredit pmeredit left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM. It might be out of scope for this ticket, but for human_4_3 should also say "create a default client with a callback that returns the test_user1 access token and a bad refresh token"

@blink1073 blink1073 marked this pull request as ready for review March 21, 2024 17:19
katcharov
katcharov requested changes Mar 22, 2024
View reviewed changes
Copy link
Contributor

@katcharov katcharov left a comment

Choose a reason for hiding this comment

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

Just some minor clarifications, and questions about where best to address some remaining issues.

source/auth/auth.md Outdated Show resolved Hide resolved
source/auth/auth.md Outdated Show resolved Hide resolved
source/auth/auth.md Outdated Show resolved Hide resolved
source/auth/tests/mongodb-oidc.md Outdated Show resolved Hide resolved
source/auth/tests/mongodb-oidc.md Outdated Show resolved Hide resolved
@blink1073 @katcharov
Update source/auth/auth.md
6ed2c94 
Co-authored-by: Maxim Katcharov <maxim.katcharov@mongodb.com>
@blink1073 blink1073 changed the title DRIVERS-2836 Only invalidate cached OIDC access tokens if the server returns error 18 DRIVERS-2836 OIDC Spec Cleanup Apr 3, 2024
@blink1073 blink1073 marked this pull request as draft April 3, 2024 15:52
@blink1073 blink1073 marked this pull request as ready for review April 9, 2024 15:43
katcharov
katcharov reviewed Apr 9, 2024
View reviewed changes
source/auth/auth.md Show resolved Hide resolved
source/auth/auth.md Outdated Show resolved Hide resolved
source/auth/auth.md Outdated Show resolved Hide resolved
source/auth/auth.md Show resolved Hide resolved
source/auth/auth.md Outdated Show resolved Hide resolved
blink1073 and others added 5 commits April 10, 2024 07:32
@blink1073 @katcharov
Update source/auth/auth.md
5967353 
Co-authored-by: Maxim Katcharov <maxim.katcharov@mongodb.com>
@blink1073 @katcharov
Update source/auth/auth.md
3242bea 
Co-authored-by: Maxim Katcharov <maxim.katcharov@mongodb.com>
@blink1073 @katcharov
Update source/auth/auth.md
ddd4054 
Co-authored-by: Maxim Katcharov <maxim.katcharov@mongodb.com>
@blink1073 @katcharov
Update source/auth/auth.md
c263ad2 
Co-authored-by: Maxim Katcharov <maxim.katcharov@mongodb.com>
@blink1073
address review
119fce1
katcharov
katcharov approved these changes Apr 10, 2024
View reviewed changes
Copy link
Contributor

@katcharov katcharov left a comment

Choose a reason for hiding this comment

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

LGTM!

pmeredit
pmeredit approved these changes Apr 10, 2024
View reviewed changes
Copy link
Contributor

@pmeredit pmeredit left a comment

Choose a reason for hiding this comment

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

LGTM other than the one question and a typo :)

source/auth/auth.md
@@ -1480,7 +1479,7 @@ An example human callback API might look like:
```typescript
interface IdpInfo {
issuer: string;
clientId: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the intended behavior when clientId is None? The code I have written on client side assumes the clientId will always exist.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be None if the user used a Human Callback and the IdP is configured on the server with supportsHumanFlows:False. They shouldn't really be doing that, but it is allowed. See https://jira.mongodb.org/browse/DRIVERS-2773 for details.

source/auth/tests/mongodb-oidc.md Outdated Show resolved Hide resolved
@blink1073 @pmeredit
Update source/auth/tests/mongodb-oidc.md
0f6f3bc 
Co-authored-by: Patrick Meredith <pmeredit@protonmail.com>
@blink1073 blink1073 requested a review from pmeredit April 11, 2024 01:23
pmeredit
pmeredit approved these changes Apr 14, 2024
View reviewed changes
Copy link
Contributor

@pmeredit pmeredit left a comment

Choose a reason for hiding this comment

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

LGTM!

katcharov
katcharov approved these changes Apr 17, 2024
View reviewed changes
Copy link
Contributor

@katcharov katcharov left a comment

Choose a reason for hiding this comment

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

LGTM!

katcharov
katcharov approved these changes Apr 22, 2024
View reviewed changes
Copy link
Contributor

@katcharov katcharov left a comment

Choose a reason for hiding this comment

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

LGTM, with the 2 suggestions

source/auth/tests/mongodb-oidc.md Outdated Show resolved Hide resolved
source/auth/tests/mongodb-oidc.md Outdated Show resolved Hide resolved
blink1073 and others added 4 commits April 22, 2024 14:35
@blink1073 @katcharov
Update source/auth/tests/mongodb-oidc.md
8cef9de 
Co-authored-by: Maxim Katcharov <maxim.katcharov@mongodb.com>
@blink1073 @katcharov
Update source/auth/tests/mongodb-oidc.md
8471762 
Co-authored-by: Maxim Katcharov <maxim.katcharov@mongodb.com>
@blink1073
Merge branch 'master' into DRIVERS-2836
1716f23
@blink1073
lint
531753e
@blink1073 blink1073 merged commit e62b205 into mongodb:master Apr 22, 2024
3 checks passed
@blink1073 blink1073 deleted the DRIVERS-2836 branch April 22, 2024 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@katcharov katcharov katcharov approved these changes

@pmeredit pmeredit pmeredit approved these changes

@durran durran Awaiting requested review from durran

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@blink1073 @katcharov @pmeredit