Skip to content

feat: add enhanced support for Slack OAuth #10151

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

Closed
wants to merge 1 commit into from
Closed

Conversation

kylecarbs
Copy link
Member

@kylecarbs kylecarbs commented Oct 9, 2023

I'm going to change this to be more abstract. It's too jank as-is right now.

I'm going to allow for any fields to be specified as extra, and then stored in the database as a JSON mapping. They can be accessed from the extra payload later on.

Comment on lines +270 to +276
oidctest.WithExtra(func(email string) map[string]interface{} {
return map[string]interface{}{
"authed_user": map[string]interface{}{
"access_token": "slack-user-token",
},
}
}),
Copy link
Member

Choose a reason for hiding this comment

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

Should we add the existing fields to the params of WithExtra? Maybe just pass the token in and let the function mutate the existing map.

I ask because it would be ideal to pass the actual access token:

oidctest.WithExtra(func(email string, token map[string]interface{}) {
	token["authed_user"] = map[string]interface{}{
		"access_token": token["access_token"],
	}
	// Completely nuke the original access token?
	token["access_token"] = "bogus_unused_token"
}),

This would make it valid to the fake OIDC IDP.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was primarily trying to mimic the usage on the output side, which has the Extra property. So while we could merge them together, I think it's better to make it super consumable to the other end.

Copy link
Member

Choose a reason for hiding this comment

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

The thing is the refresh works because the refresh token is set correctly.

But the access-token is invalidly set to slack-user-token. We only check access-token on /user-info, so maybe it does not matter. But I could see it being a benefit to set the extra value to the correctly generated access_token.

Copy link
Member Author

Choose a reason for hiding this comment

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

The access_token isn't being set. authed_user is which has a property of access_token.

@kylecarbs kylecarbs closed this Oct 9, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Oct 9, 2023
@github-actions github-actions bot deleted the slackprovider branch April 10, 2024 00:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants