Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Support Google OAuth2 and other OIdC providers #147

Merged
merged 27 commits into from
Mar 2, 2021
Merged

Support Google OAuth2 and other OIdC providers #147

merged 27 commits into from
Mar 2, 2021

Conversation

EngHabu
Copy link
Contributor

@EngHabu EngHabu commented Jan 5, 2021 •
edited
Loading

TL;DR

A few changes to make sure OIdC with other providers (besides Okta) also work:

  • Move OAuth scopes to a config. This allows flyte admins to choose which additional scopes to request. E.g. request offline_access in Okta case but not in Google Auth
  • Rely on id_token instead of access_token for authentication
  • Persist/restore id_token to cookies

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Tracking Issue

flyteorg/flyte#657

@EngHabu EngHabu requested a review from katrogan as a code owner January 8, 2021 19:01
katrogan
katrogan previously approved these changes Jan 8, 2021
View reviewed changes
pkg/auth/handlers.go Outdated Show resolved Hide resolved
@EngHabu EngHabu requested a review from katrogan January 11, 2021 17:38
@katrogan
Copy link
Contributor

looks like github.com/lyft/flyteadmin/pkg/auth compilation failed?

@EngHabu EngHabu requested a review from kumare3 as a code owner February 22, 2021 18:19
@EngHabu EngHabu changed the title Move scopes to config Support Google OAuth2 and other OIdC providers Feb 23, 2021
kumare3
kumare3 reviewed Feb 23, 2021
View reviewed changes
flyteadmin_config.yaml Outdated Show resolved Hide resolved
@katrogan
Copy link
Contributor

LGTM but will let @wild-endeavor review & approve

wild-endeavor
wild-endeavor previously approved these changes Feb 24, 2021
View reviewed changes
Copy link
Contributor

@wild-endeavor wild-endeavor left a comment

Choose a reason for hiding this comment

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

reminder to @anandswaminathan that when this goes out you will need to add scopes

@EngHabu
Copy link
Contributor Author

EngHabu commented Feb 24, 2021

I added them as default values: https://github.com/flyteorg/flyteadmin/pull/147/files#diff-a3d824da3c42420cd5cbb0a4a2c0e7b5bfddd819652788a0596d195dc6e31fa5R55-R60

so we don't have to break existing behavior... Is that enough?

EngHabu and others added 12 commits February 26, 2021 16:51
@EngHabu
Move scopes to config
cb3388b 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
missed
2c9d741 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
wip
5b240dc 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
Update handlers.go
28c6814 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@wild-endeavor @EngHabu
go get propeller at v0.5.12 (#146)
99f8279 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@katrogan @EngHabu
Update to using latest flytepropeller v0.5.13 (#148)
ec898af 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
Update propeller to 0.5.14 (#149)
5757466 
* Update propeller

* cleanup

Signed-off-by: Haytham Abuelfutuh <[email protected]>
@katrogan @EngHabu
Filter executions by user (#150)
11692d7 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
Update CI post migration
9a38dee 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
Update codecov link
bf68098 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@tnsetting @EngHabu
Publish raw events (#151)
327494d 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
fix test
1f69a77 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
EngHabu added 10 commits February 26, 2021 16:51
@EngHabu
wip
c200a25 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
Fix token retrieval from cookies
a8f02c4 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
Fix unit tests and lint
167467e 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
Move to const
c141091 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
Revert Auth config
c30754d 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
Revert kube config path
0af1714 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
Use access token when posting to IdP
1084093 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
Ignore refresh token error
503f889 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
Fix tests
954416e 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
Expose openId metadata endpoint and expose scopes in /config endpoint
28ff50d 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
wild-endeavor
wild-endeavor previously approved these changes Feb 27, 2021
View reviewed changes
wild-endeavor
wild-endeavor previously approved these changes Mar 1, 2021
View reviewed changes
@EngHabu EngHabu force-pushed the offline_access branch 2 times, most recently from c8cf001 to 28ff50d Compare March 2, 2021 00:27
EngHabu added 5 commits March 1, 2021 16:30
@EngHabu
merge master
f2e88c8 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
mod tidy
b702934 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
rename
98dc4bc 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
lint
7cab9f2 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu
unit tests
d1a3332 
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu EngHabu merged commit 1948047 into master Mar 2, 2021
eapolinario pushed a commit that referenced this pull request Sep 6, 2023
@EngHabu @wild-endeavor
@katrogan @tnsetting
Support Google OAuth2 and other OIdC providers (#147)
6b0e1e9 
* Move scopes to config

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* missed

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* wip

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Update handlers.go

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* go get propeller at v0.5.12 (#146)

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Update to using latest flytepropeller v0.5.13 (#148)

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Update propeller to 0.5.14 (#149)

* Update propeller

* cleanup

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Filter executions by user (#150)

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Update CI post migration

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Update codecov link

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Publish raw events (#151)

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* fix test

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* wip

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Fix token retrieval from cookies

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Fix unit tests and lint

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Move to const

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Revert Auth config

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Revert kube config path

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Use access token when posting to IdP

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Ignore refresh token error

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Fix tests

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* Expose openId metadata endpoint and expose scopes in /config endpoint

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* mod tidy

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* rename

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* lint

Signed-off-by: Haytham Abuelfutuh <[email protected]>

* unit tests

Signed-off-by: Haytham Abuelfutuh <[email protected]>

Co-authored-by: Yee Hing Tong <[email protected]>
Co-authored-by: Katrina Rogan <[email protected]>
Co-authored-by: tnsetting <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kumare3 kumare3 kumare3 left review comments

@katrogan katrogan katrogan left review comments

@wild-endeavor wild-endeavor wild-endeavor approved these changes

@anandswaminathan anandswaminathan Awaiting requested review from anandswaminathan

@matthewphsmith matthewphsmith Awaiting requested review from matthewphsmith

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

Successfully merging this pull request may close these issues.
5 participants
@EngHabu @katrogan @wild-endeavor @kumare3 @tnsetting