NotificationsYou must be signed in to change notification settings
Fork927
Star10.1k

feat: add oauth2 codes and tokens to database#11779

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

code-asher wants to merge8 commits intomainfromasher/oauth2-exchange-db

Closed

feat: add oauth2 codes and tokens to database#11779

code-asher wants to merge8 commits intomainfromasher/oauth2-exchange-db

Conversation

Copy link

Member

code-asher commentedJan 23, 2024•
edited
Loading

Split out to hopefully make reviewing and merging easier. Works in tandom with#11778.

Blocks:

Add OAuth2 provider codes and tokens to database

d54e56a

github-actionsbot assignedcode-asher

Jan 23, 2024

This was referencedJan 23, 2024

feat: add oauth2 token exchange#11778

Merged

feat: enable oauth2 token exchange#11609

Closed

code-asher changed the title~~Add OAuth2 provider codes and tokens to database~~feat: add oauth2 codes and tokens to database

Jan 23, 2024

code-asher mentioned this pull request

Jan 23, 2024

feat: add user settings page for revoking OAuth2 apps#11780

Closed

code-asher marked this pull request as ready for review

January 23, 2024 19:23

code-asher requested review fromjohnstcn andEmyrk

January 23, 2024 19:23

Copy link

MemberAuthor

code-asher commentedJan 23, 2024

@johnstcn just FYI I requested you along with@Emyrk since we co-wrote some of this and seems like it makes sense to get another pair of eyes.

Check indexes in dbmem

7b118ac

Might have no match, and a -1 will panic.

johnstcn reviewed

Jan 24, 2024

View reviewed changes

Copy link

Member

johnstcn left a comment

Choose a reason for hiding this comment

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

In the interest of avoiding later rework, would it be possible to add test coverage for the newly added methods in this PR?

Emyrk added2 commits

January 24, 2024 09:41

minor dbauthz changes

b0df965

use model struct over rbac.ResourceXX

cc366f6

Emyrk reviewed

Jan 24, 2024

View reviewed changes

coderd/database/dbmem/dbmem.go

Comment on lines +1250 to +1251

		q.oauth2ProviderAppCodes[index] = q.oauth2ProviderAppCodes[len(q.oauth2ProviderAppCodes)-1]
		q.oauth2ProviderAppCodes = q.oauth2ProviderAppCodes[:len(q.oauth2ProviderAppCodes)-1]

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 about this is that it does change the slice order. If we care about order from a test assertion perspective, this will fail that assumption.

Testsshould be written such that if the order is required, they should specify in the query though, so we can leave this.

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Good point! I think we will probably not have to worry about it since we do not rely on ordering of authentication codes (there is no method to get multiple codes right now either) but seems worth keeping in mind.

I lazily copied these lines from other implementations in this file but I think it might be better to add aremove and change all of these:

func remove[T any](slice []T, i int) []T {    return append(slice[:i], slice[i+1:]...)}

Copy link

MemberAuthor

code-asherJan 24, 2024

Choose a reason for hiding this comment

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

The way I have been doing it (for apps and secrets which do need ordering) is to sort them before returning in theirGet* calls so the underlying slice can be in any order.

Copy link

MemberAuthor

code-asherJan 24, 2024

Choose a reason for hiding this comment

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

Ooo 1.21 has aDelete for slices apparently.

coderd/database/migrations/000186_oauth2_provider_codes.up.sql OutdatedShow resolvedHide resolved

coderd/database/queries/oauth2.sql OutdatedShow resolvedHide resolved

Copy link

MemberAuthor

code-asher commentedJan 24, 2024•
edited
Loading

In the interest of avoiding later rework, would it be possible to add test coverage for the newly added methods in this PR?

Good question, do the dbauthz tests look insufficient? They test all the methods and that they return the right thing, I think (I might have the wrong impression on some of the things it does though.)

There are also the coderd tests added in the next PR (#11778) that end up hitting these methods, maybe I made the wrong call to split the PR here. I can hold off merging this until both are good to merge.