Skip to content

Add docs for refresh-token support to OAuth2#13009

Merged
Praveen2112 merged 1 commit intotrinodb:masterfrom
Praveen2112:praveen/docs/refresh_token
Jul 14, 2022
Merged

Add docs for refresh-token support to OAuth2#13009
Praveen2112 merged 1 commit intotrinodb:masterfrom
Praveen2112:praveen/docs/refresh_token

Conversation

@Praveen2112
Copy link
Copy Markdown
Member

@Praveen2112 Praveen2112 commented Jun 28, 2022

Description

Is this change a fix, improvement, new feature, refactoring, or other?

Documentation for #12664

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Docs only.

How would you describe this change to a non-technical end user or system administrator?

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
(x) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Jun 28, 2022
@github-actions github-actions bot added the docs label Jun 28, 2022
mosabua
mosabua requested changes Jul 4, 2022
View reviewed changes
@Praveen2112 Praveen2112 force-pushed the praveen/docs/refresh_token branch 2 times, most recently from 1a23d5c to 3b33bc5 Compare July 7, 2022 14:58
@Praveen2112
Copy link
Copy Markdown
Member Author

Praveen2112 commented Jul 7, 2022

@mosabua Did some additional changes to extract refresh token flow under a separate section

s2lomon
s2lomon reviewed Jul 7, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@s2lomon s2lomon left a comment

Choose a reason for hiding this comment

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

% comments

docs/src/main/sphinx/security/oauth2.rst Outdated
Copy link
Copy Markdown
Member

@s2lomon s2lomon Jul 7, 2022

Choose a reason for hiding this comment

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

I would rather go with something like: additional scopes for offline access might be required, depending on your IdP configuration

docs/src/main/sphinx/security/oauth2.rst Outdated
Copy link
Copy Markdown
Member

@s2lomon s2lomon Jul 7, 2022

Choose a reason for hiding this comment

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

openid,offline_access [or offline] // openid is the default scope and when someone will remove it, idps will switch to OAuth2 flows, rather than openid (so potentially no things like groups handling etc.)

@Praveen2112 Praveen2112 force-pushed the praveen/docs/refresh_token branch from 3b33bc5 to b9624be Compare July 7, 2022 15:14
@Praveen2112
Copy link
Copy Markdown
Member Author

Praveen2112 commented Jul 7, 2022

@s2lomon AC

mosabua
mosabua requested changes Jul 7, 2022
View reviewed changes
@Praveen2112 Praveen2112 force-pushed the praveen/docs/refresh_token branch from b9624be to 569bb3b Compare July 7, 2022 17:11
@Praveen2112
Copy link
Copy Markdown
Member Author

Praveen2112 commented Jul 7, 2022

@mosabua Thanks for the review. AC

@Praveen2112 Praveen2112 requested a review from mosabua July 7, 2022 17:12
s2lomon
s2lomon approved these changes Jul 8, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@s2lomon s2lomon left a comment

Choose a reason for hiding this comment

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

lgtm % small typo

@Praveen2112 Praveen2112 force-pushed the praveen/docs/refresh_token branch from 569bb3b to 09755e5 Compare July 8, 2022 13:16
lukasz-walkiewicz
lukasz-walkiewicz reviewed Jul 8, 2022
View reviewed changes
docs/src/main/sphinx/security/oauth2.rst Outdated
Copy link
Copy Markdown
Member

@lukasz-walkiewicz lukasz-walkiewicz Jul 8, 2022

Choose a reason for hiding this comment

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

These 3 properties have default values as well so it's probably worth documenting it.

Copy link
Copy Markdown
Member Author

@Praveen2112 Praveen2112 Jul 12, 2022

Choose a reason for hiding this comment

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

Added them

@Praveen2112 Praveen2112 force-pushed the praveen/docs/refresh_token branch from 09755e5 to 554bc8c Compare July 11, 2022 10:45
lukasz-walkiewicz
lukasz-walkiewicz reviewed Jul 12, 2022
View reviewed changes
docs/src/main/sphinx/security/oauth2.rst Outdated
Copy link
Copy Markdown
Member

@lukasz-walkiewicz lukasz-walkiewicz Jul 12, 2022

Choose a reason for hiding this comment

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

By default it's generated during startup.

Copy link
Copy Markdown
Member Author

@Praveen2112 Praveen2112 Jul 13, 2022

Choose a reason for hiding this comment

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

Added them

@Praveen2112 Praveen2112 force-pushed the praveen/docs/refresh_token branch from 554bc8c to 35bf4ed Compare July 13, 2022 07:42
@Praveen2112 Praveen2112 requested review from mosabua and removed request for mosabua July 13, 2022 07:43
mosabua
mosabua approved these changes Jul 14, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Looks good now. Lets get this shipped.

@Praveen2112 Praveen2112 merged commit 9b92233 into trinodb:master Jul 14, 2022
@github-actions github-actions bot added this to the 390 milestone Jul 14, 2022
@colebow colebow mentioned this pull request Jul 14, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mosabua mosabua mosabua approved these changes

+2 more reviewers

@lukasz-walkiewicz lukasz-walkiewicz lukasz-walkiewicz left review comments

@s2lomon s2lomon s2lomon approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed docs

Milestone

390

Development

Successfully merging this pull request may close these issues.

4 participants

@Praveen2112 @mosabua @s2lomon @lukasz-walkiewicz