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

feat: Update to Slack OAuth V2 #1591

Merged
merged 11 commits into from
Jun 12, 2024
Merged

Conversation

zhawtof
Copy link
Contributor

@zhawtof zhawtof commented May 29, 2024

What kind of change does this PR introduce?

  • Updates the Slack OAuth provider with the new Sign In With Slack V2.
  • Creates a test for Slack, improving test coverage
  • Moves the old Slack provider to slack_legacy. Some users might still rely on this provider after the creation of legacy apps is disallowed on June 4th.

What is the current behavior?

Fixes #1294

Current behavior uses the original Slack OAuth V1 which is sunsetting June 4th according to the changelog

What is the new behavior?

New behavior now leverages the new Sign In With Slack (SIWS) on OAuth V2 for Slack authentication.

Additional context

A ticket should be created for ending support on slack_legacy.

@zhawtof
Copy link
Contributor Author

zhawtof commented Jun 3, 2024

Hi @kangmingtay and @hf! There is an end-of-life for creating Legacy Slack Apps on June 4th.

I'm hoping we can check in by then so that Supabase will still support Slack OAuth.

Please let me know if there are any additional updates you would like to see.

internal/api/external.go Outdated Show resolved Hide resolved
internal/api/provider/slack_legacy.go Outdated Show resolved Hide resolved
@zhawtof zhawtof force-pushed the feat-slack-oauth-v2 branch from 246dcc3 to 36304f2 Compare June 5, 2024 02:15
@zhawtof
Copy link
Contributor Author

zhawtof commented Jun 5, 2024

As requested @kangmingtay I undid the changes to slack (from slack_legacy) and included a net new slack_oidc provider.

Copy link
Member

@kangmingtay kangmingtay left a comment

Choose a reason for hiding this comment

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

thanks for the quick turnaround @zhawtof! i'll test this out later this week and if it's all good, we can merge it in

it would be nice if you can make a PR to update the slack oauth docs too - you can take a look at what we did for the linkedin oauth docs when we had to update the provider to support the new API

internal/api/provider/slack_oidc.go Outdated Show resolved Hide resolved
@zhawtof
Copy link
Contributor Author

zhawtof commented Jun 8, 2024

thanks for the quick turnaround @zhawtof! i'll test this out later this week and if it's all good, we can merge it in

it would be nice if you can make a PR to update the slack oauth docs too - you can take a look at what we did for the linkedin oauth docs when we had to update the provider to support the new API

Updated the docs @kangmingtay. Hopefully, the PR is now good to go.

After this change, I want to reference a new issue missing in Supabase Auth around Slack.

The legacy system allowed users to add additional scopes but with Sign In With Slack, that capability is no longer allowed. Details are here.

It's generally recommended that those scopes be added in subsequent requests. Because Supabase supports users with multiple login providers merging into one common user, an additional provider should be created for OAuth 2.0 that does not follow the OIDC protocol.

This would allow developers to have SIWS, where a user can log in with a single-click experience, and OAuth 2.0 (w/out OIDC), where a user has a 2-step process for approving the additional scopes. That way, all relevant scopes can be configured for the same user in Supabase.

This should be created in a separate ticket if we agree.

@kangmingtay
Copy link
Member

kangmingtay commented Jun 10, 2024

@zhawtof

The legacy system allowed users to add additional scopes but with Sign In With Slack, that capability is no longer allowed.

I'm not too sure i understand this fully - are you referring to this note by slack:

A scope conflict occurs when attempting to combine Sign in with Slack (SIWS) user scopes with non-Sign in with Slack scopes in the same OAuth flow. Each set of scopes must be requested in a separate OAuth flow.

so it seems to me that if you need to request for a token with user scopes and granular bot scopes, you need to make 2 separate oauth requests with each set of scopes?

an additional provider should be created for OAuth 2.0 that does not follow the OIDC protocol. This would allow developers to have SIWS, where a user can log in with a single-click experience, and OAuth 2.0 (w/out OIDC), where a user has a 2-step process for approving the additional scopes. That way, all relevant scopes can be configured for the same user in Supabase.

Would be keen to hear how you plan to achieve this.

@coveralls
Copy link

coveralls commented Jun 10, 2024

Pull Request Test Coverage Report for Build 9452537318

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 30 of 68 (44.12%) changed or added relevant lines in 4 files are covered.
  • 92 unchanged lines in 7 files lost coverage.
  • Overall coverage decreased (-0.05%) to 57.544%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/api/provider/slack.go 0 1 0.0%
internal/api/provider/slack_oidc.go 27 64 42.19%
Files with Coverage Reduction New Missed Lines %
internal/api/mail.go 7 60.65%
internal/models/cleanup.go 8 88.37%
internal/observability/request-tracing.go 9 81.98%
internal/api/provider/spotify.go 10 0.0%
internal/crypto/password.go 14 78.29%
internal/api/middleware.go 16 80.81%
internal/observability/metrics.go 28 4.96%
Totals Coverage Status
Change from base Build 9369008036: -0.05%
Covered Lines: 8539
Relevant Lines: 14839

💛 - Coveralls

@zhawtof
Copy link
Contributor Author

zhawtof commented Jun 11, 2024

Thanks for cleaning up my tests @kangmingtay.

I'll also have to look at the OAuth2 flow without OIDC in a few weeks. We'll likely be a first consumer after I get SIWS working in my own Supabase app.

On that note, I am wondering what the expected timeline for merging and releasing this in an upcoming Supabase might be. I appreciate the support, and hopefully, this helps some Slack devs out there.

@kangmingtay
Copy link
Member

@zhawtof it will probably be some time next week or after - merging the PR doesn't reflect that the version will be deployed on the platform immediately

for this PR specifically, we also need some time to make changes on the UI + backend to support the new configs added, as well as roll out any comms necessary to let folks know that we've added a new slack provider and plan to deprecate the old one.

@kangmingtay kangmingtay merged commit bb99251 into supabase:master Jun 12, 2024
1 check passed
J0 pushed a commit that referenced this pull request Jun 14, 2024
🤖 I have created a release *beep* *boop*
---


##
[2.154.0](v2.153.0...v2.154.0)
(2024-06-12)


### Features

* add max length check for email
([#1508](#1508))
([f9c13c0](f9c13c0))
* add support for Slack OAuth V2
([#1591](#1591))
([bb99251](bb99251))
* encrypt sensitive columns
([#1593](#1593))
([e4a4758](e4a4758))
* upgrade otel to v1.26
([#1585](#1585))
([cdd13ad](cdd13ad))
* use largest avatar from spotify instead
([#1210](#1210))
([4f9994b](4f9994b)),
closes [#1209](#1209)


### Bug Fixes

* define search path in auth functions
([#1616](#1616))
([357bda2](357bda2))
* enable rls & update grants for auth tables
([#1617](#1617))
([28967aa](28967aa))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@zhawtof zhawtof deleted the feat-slack-oauth-v2 branch June 16, 2024 16:56
uxodb pushed a commit to uxodb/auth that referenced this pull request Nov 13, 2024
## What kind of change does this PR introduce?

- Updates the Slack OAuth provider with the new Sign In With Slack V2.
- Creates a test for Slack, improving test coverage
- Moves the old Slack provider to slack_legacy. Some users might still
rely on this provider after the creation of legacy apps is disallowed on
June 4th.

## What is the current behavior?

Fixes supabase#1294

Current behavior uses the original Slack OAuth V1 which is sunsetting
June 4th according to [the
changelog](https://api.slack.com/changelog/2024-04-discontinuing-new-creation-of-classic-slack-apps-and-custom-bots)

## What is the new behavior?

New behavior now leverages the new [Sign In With
Slack](https://api.slack.com/authentication/sign-in-with-slack) (SIWS)
on OAuth V2 for Slack authentication.

## Additional context

A ticket should be created for ending support on slack_legacy.

---------

Co-authored-by: Kang Ming <kang.ming1996@gmail.com>
uxodb pushed a commit to uxodb/auth that referenced this pull request Nov 13, 2024
🤖 I have created a release *beep* *boop*
---


##
[2.154.0](supabase/auth@v2.153.0...v2.154.0)
(2024-06-12)


### Features

* add max length check for email
([supabase#1508](supabase#1508))
([f9c13c0](supabase@f9c13c0))
* add support for Slack OAuth V2
([supabase#1591](supabase#1591))
([bb99251](supabase@bb99251))
* encrypt sensitive columns
([supabase#1593](supabase#1593))
([e4a4758](supabase@e4a4758))
* upgrade otel to v1.26
([supabase#1585](supabase#1585))
([cdd13ad](supabase@cdd13ad))
* use largest avatar from spotify instead
([supabase#1210](supabase#1210))
([4f9994b](supabase@4f9994b)),
closes [supabase#1209](supabase#1209)


### Bug Fixes

* define search path in auth functions
([supabase#1616](supabase#1616))
([357bda2](supabase@357bda2))
* enable rls & update grants for auth tables
([supabase#1617](supabase#1617))
([28967aa](supabase@28967aa))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 13, 2024
## What kind of change does this PR introduce?

- Updates the Slack OAuth provider with the new Sign In With Slack V2.
- Creates a test for Slack, improving test coverage
- Moves the old Slack provider to slack_legacy. Some users might still
rely on this provider after the creation of legacy apps is disallowed on
June 4th.

## What is the current behavior?

Fixes supabase#1294

Current behavior uses the original Slack OAuth V1 which is sunsetting
June 4th according to [the
changelog](https://api.slack.com/changelog/2024-04-discontinuing-new-creation-of-classic-slack-apps-and-custom-bots)

## What is the new behavior?

New behavior now leverages the new [Sign In With
Slack](https://api.slack.com/authentication/sign-in-with-slack) (SIWS)
on OAuth V2 for Slack authentication.

## Additional context

A ticket should be created for ending support on slack_legacy.

---------

Co-authored-by: Kang Ming <kang.ming1996@gmail.com>
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 13, 2024
🤖 I have created a release *beep* *boop*
---


##
[2.154.0](supabase/auth@v2.153.0...v2.154.0)
(2024-06-12)


### Features

* add max length check for email
([supabase#1508](supabase#1508))
([f9c13c0](supabase@f9c13c0))
* add support for Slack OAuth V2
([supabase#1591](supabase#1591))
([bb99251](supabase@bb99251))
* encrypt sensitive columns
([supabase#1593](supabase#1593))
([e4a4758](supabase@e4a4758))
* upgrade otel to v1.26
([supabase#1585](supabase#1585))
([cdd13ad](supabase@cdd13ad))
* use largest avatar from spotify instead
([supabase#1210](supabase#1210))
([4f9994b](supabase@4f9994b)),
closes [supabase#1209](supabase#1209)


### Bug Fixes

* define search path in auth functions
([supabase#1616](supabase#1616))
([357bda2](supabase@357bda2))
* enable rls & update grants for auth tables
([supabase#1617](supabase#1617))
([28967aa](supabase@28967aa))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 15, 2024
## What kind of change does this PR introduce?

- Updates the Slack OAuth provider with the new Sign In With Slack V2.
- Creates a test for Slack, improving test coverage
- Moves the old Slack provider to slack_legacy. Some users might still
rely on this provider after the creation of legacy apps is disallowed on
June 4th.

## What is the current behavior?

Fixes supabase#1294

Current behavior uses the original Slack OAuth V1 which is sunsetting
June 4th according to [the
changelog](https://api.slack.com/changelog/2024-04-discontinuing-new-creation-of-classic-slack-apps-and-custom-bots)

## What is the new behavior?

New behavior now leverages the new [Sign In With
Slack](https://api.slack.com/authentication/sign-in-with-slack) (SIWS)
on OAuth V2 for Slack authentication.

## Additional context

A ticket should be created for ending support on slack_legacy.

---------

Co-authored-by: Kang Ming <kang.ming1996@gmail.com>
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 15, 2024
🤖 I have created a release *beep* *boop*
---


##
[2.154.0](supabase/auth@v2.153.0...v2.154.0)
(2024-06-12)


### Features

* add max length check for email
([supabase#1508](supabase#1508))
([f9c13c0](supabase@f9c13c0))
* add support for Slack OAuth V2
([supabase#1591](supabase#1591))
([bb99251](supabase@bb99251))
* encrypt sensitive columns
([supabase#1593](supabase#1593))
([e4a4758](supabase@e4a4758))
* upgrade otel to v1.26
([supabase#1585](supabase#1585))
([cdd13ad](supabase@cdd13ad))
* use largest avatar from spotify instead
([supabase#1210](supabase#1210))
([4f9994b](supabase@4f9994b)),
closes [supabase#1209](supabase#1209)


### Bug Fixes

* define search path in auth functions
([supabase#1616](supabase#1616))
([357bda2](supabase@357bda2))
* enable rls & update grants for auth tables
([supabase#1617](supabase#1617))
([28967aa](supabase@28967aa))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Getting "500: Unable to exchange external code" when using signInWithOAuth for Slack
3 participants