Skip to content

OpenAPI: Deprecate oauth/tokens endpoint #10603

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
Merged
Fokko merged 4 commits into from
Jul 12, 2024
Merged

OpenAPI: Deprecate oauth/tokens endpoint #10603

Fokko merged 4 commits into from
Jul 12, 2024
+69 −4

Conversation

@snazy
Copy link
Member

@snazy snazy commented Jun 28, 2024 •
edited
Loading

This PR implements "M1" of this document, see #10537.

dimas-b
dimas-b reviewed Jun 28, 2024
View reviewed changes
@snazy snazy force-pushed the rest-tokens-deprecate branch from 16e4a1e to 2efb0d4 Compare June 28, 2024 19:20
nastra
nastra reviewed Jun 29, 2024
View reviewed changes
nastra
nastra reviewed Jun 29, 2024
View reviewed changes
@snazy snazy force-pushed the rest-tokens-deprecate branch from 2efb0d4 to 38cf158 Compare June 29, 2024 06:54
snazy
snazy commented Jun 29, 2024
View reviewed changes
ajantha-bhat
ajantha-bhat reviewed Jun 30, 2024
View reviewed changes
@snazy snazy force-pushed the rest-tokens-deprecate branch from 38cf158 to 9e100ba Compare July 1, 2024 10:22
@snazy snazy force-pushed the rest-tokens-deprecate branch 2 times, most recently from 48fc819 to cc10a13 Compare July 1, 2024 17:05
jackye1995
jackye1995 reviewed Jul 2, 2024
View reviewed changes
jackye1995
jackye1995 reviewed Jul 2, 2024
View reviewed changes
jackye1995
jackye1995 reviewed Jul 2, 2024
View reviewed changes
nastra
nastra reviewed Jul 2, 2024
View reviewed changes
@snazy snazy force-pushed the rest-tokens-deprecate branch from 4bc026c to b710286 Compare July 2, 2024 16:14
nastra
nastra reviewed Jul 2, 2024
View reviewed changes
@jackye1995 jackye1995 mentioned this pull request Jul 2, 2024
Closed
@snazy snazy force-pushed the rest-tokens-deprecate branch from 196f50c to 66fddb3 Compare July 4, 2024 16:26
danielcweeks
danielcweeks reviewed Jul 8, 2024
View reviewed changes
jackye1995
jackye1995 approved these changes Jul 9, 2024
View reviewed changes
Copy link
Contributor

@jackye1995 jackye1995 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 to me

@c-thiel c-thiel mentioned this pull request Jul 10, 2024
Closed
@snazy snazy force-pushed the rest-tokens-deprecate branch 2 times, most recently from 0a1b94f to 6fc0f69 Compare July 10, 2024 15:56
nastra
nastra reviewed Jul 10, 2024
View reviewed changes
open-api/rest-catalog-open-api.yaml Outdated
to the correct OAuth endpoint.

Deprecated since Iceberg (Java) 1.6.0. The endpoint and related types will be removed from
this spec in Iceberg (Java) 1.7.0.
Copy link
Contributor

@nastra nastra Jul 10, 2024

Choose a reason for hiding this comment

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

what's the implication of removing this (and all the request / response types) from the Spec in 1.7.0 but not actually from the implementation? To me it seems that this should be marked for removal with Iceberg 2.0

Copy link
Member Author

@snazy snazy Jul 10, 2024

Choose a reason for hiding this comment

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

It's to let (new) adopters run into the trap of "blindly" implementing it and accidentally run into security issues

Copy link
Contributor

@dimas-b dimas-b Jul 10, 2024

Choose a reason for hiding this comment

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

Removing the endpoint from the OpenAPI YAML could break auto-generated clients. Custom clients (e.g. the Iceberg java REST client) calling this endpoint in servers that offer backward compatibility will not be affected. AFAIK, PyIceberg also not affected by dropping the endpoint from OpenAPI.

Given the discussion of the negative security aspects of this endpoint (in the dev mail list), I tend to think that removing the endpoint from Open API sooner (1.7.0) is worth the potential hardship for auto-generated clients.

@snazy snazy force-pushed the rest-tokens-deprecate branch from 6fc0f69 to 27dd00f Compare July 10, 2024 18:22
@ajantha-bhat ajantha-bhat added this to the Iceberg 1.6.0 milestone Jul 11, 2024
@ajantha-bhat
Copy link
Member

ajantha-bhat commented Jul 11, 2024

I have added 1.6.0 milestone for this now because we still have a day or two for the release cut and we have mentioned that we are deprecating it in 1.6.0 in the PR changes.

@nastra nastra changed the title Deprecate oauth/tokens endpoint OpenAPI: Deprecate oauth/tokens endpoint Jul 11, 2024
@snazy
Copy link
Member Author

snazy commented Jul 11, 2024

I've updated the PR to mention "2.0".

The CI failures look unrelated, but I don't have the power to rerun those.

@jackye1995
Copy link
Contributor

jackye1995 commented Jul 11, 2024

Can you try rebase to see if it fixes the CI?

@snazy snazy force-pushed the rest-tokens-deprecate branch from 2b566d1 to a1e3c73 Compare July 11, 2024 15:55
danielcweeks
danielcweeks reviewed Jul 11, 2024
View reviewed changes
@snazy
Copy link
Member Author

snazy commented Jul 11, 2024

Can you try rebase to see if it fixes the CI?

CI looking good

@Fokko
Copy link
Contributor

Fokko commented Jul 12, 2024

The vote has been passed: https://lists.apache.org/thread/o4qmrm5jx50mk1mqws0t9f1z2op4gvvm

@Fokko Fokko merged commit 63af974 into apache:main Jul 12, 2024
@Fokko
Copy link
Contributor

Fokko commented Jul 12, 2024

Thanks everyone, moving this forward for the 1.6.0 release 👍

jasonf20 pushed a commit to jasonf20/iceberg that referenced this pull request Aug 4, 2024
@snazy @jasonf20
OpenAPI: Deprecate oauth/tokens endpoint (apache#10603)
61cbea6 
* Deprecate `oauth/tokens` endpoint

This PR implements "M1" of [this
document](https://docs.google.com/document/d/1Xi5MRk8WdBWFC3N_eSmVcrLhk3yu5nJ9x_wC0ec6kVQ/), see apache#10537.

* update wording in spec

* 2

* left-over
@agrawalreetika agrawalreetika mentioned this pull request Sep 29, 2024
Merged
6 tasks
@snazy snazy deleted the rest-tokens-deprecate branch October 2, 2024 19:16
zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024
@snazy @zachdisc
OpenAPI: Deprecate oauth/tokens endpoint (apache#10603)
2ab4833 
* Deprecate `oauth/tokens` endpoint

This PR implements "M1" of [this
document](https://docs.google.com/document/d/1Xi5MRk8WdBWFC3N_eSmVcrLhk3yu5nJ9x_wC0ec6kVQ/), see apache#10537.

* update wording in spec

* 2

* left-over
@dimas-b dimas-b mentioned this pull request Feb 19, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danielcweeks danielcweeks danielcweeks left review comments

@jackye1995 jackye1995 jackye1995 approved these changes

@nastra nastra nastra approved these changes

@Fokko Fokko Fokko approved these changes

+3 more reviewers

@jbonofre jbonofre jbonofre left review comments

@dimas-b dimas-b dimas-b approved these changes

@ajantha-bhat ajantha-bhat ajantha-bhat approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

build core OPENAPI

Projects

None yet

Milestone

Iceberg 1.6.0

Development

Successfully merging this pull request may close these issues.

8 participants

@snazy @ajantha-bhat @jackye1995 @Fokko @jbonofre @nastra @danielcweeks @dimas-b