- Notifications
You must be signed in to change notification settings - Fork947
feat: add client credentials OAuth2 applications for API access#18846
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
base:thomask33/07-12-feat_oauth2_add_client_credentials_grant_type_and_user_ownership
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ThomasK33 commentedJul 14, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stackon Graphite.
This stack of pull requests is managed byGraphite. Learn more aboutstacking. |
c654948
toeac2681
Compare8c29819
to168176b
Compareeac2681
toac0d1f6
Compare4fcf5b1
to65b1054
CompareI'm doing the review, but I'm really missing a video demoing the feature, or at least screenshots. |
I’ll add some screenshots tomorrow |
BrunoQuaresma left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks for working on the FE 🙏 👏
Since there are a good amount of changes, and I want to be sure I'm going to review them properly, and not overwhelming you, I think it would make sense to run a few review rounds.
For the first round, I pointed one blocker, and a few major improvements we can do. After we work on that, I will run another review and see if there is any minor left.
Sounds good?
site/src/api/api.ts Outdated
@@ -1726,11 +1726,18 @@ class ApiMethods { | |||
getOAuth2ProviderApps = async ( | |||
filter?: TypesGen.OAuth2ProviderAppFilter, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think we can simplify this function by just making the filter not optional and adding a default value as{}
so we could use it directly in theURLSearchParams
without having to do many ifs.
getOAuth2ProviderApps=async(filter:TypesGen.OAuth2ProviderAppFilter={},):Promise<TypesGen.OAuth2ProviderApp[]>=>{constparams=newURLSearchParams(filter);constresp=awaitthis.axios.get("/api/v2/oauth2-provider/apps",{ params},);returnresp.data;};
site/src/api/queries/oauth2.ts Outdated
@@ -21,10 +21,10 @@ export const getGitHubDeviceFlowCallback = (code: string, state: string) => { | |||
}; | |||
}; | |||
export const getApps = (userId?: string) => { | |||
export const getApps = (filter?:{ user_id?:string; owner_id?: string }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Another function we can improve.
exportconstgetApps=(filter:OAuth2ProviderAppFilter={})=>{return{queryKey:[...appsKey,filter],queryFn:()=>API.getOAuth2ProviderApps(filter),};};
@@ -1606,17 +1606,22 @@ export interface OAuth2ProviderApp { | |||
readonly name: string; | |||
readonly redirect_uris: readonly string[]; | |||
readonly icon: string; | |||
readonly created_at: string; | |||
readonly grant_types: readonly string[]; | |||
readonly user_id?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is it possible to have anOAuth2ProviderApp
without an user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
yes, that's possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What is the use case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Different flows/grant types. Client credential flows have to be owned/tied to a user, while authorization code flow must not be tied to a user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do you think would make sense to left a comment in this attribute, in the codersdk type? I think this is something I would easily forget in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My overall feeling is this component is doing way more than a form should do. I think we can improve by:
- Remove props that are not going to change and use the value directly
- Execute the mutation in the form, and only call the
onSubmit
oronSubmitSuccess
if the parent needs that
By applying these, I think we can significantly reduce the complexity of this component. This may require a few changes, so probably will need a second review round.
event.preventDefault(); | ||
const formData = new FormData(event.target as HTMLFormElement); | ||
onSubmit({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
const [showCodeExample, setShowCodeExample] = useState(false); | ||
// Fetch users and find the owner by user_id | ||
const usersQuery = useQuery({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The BE supports filtering a user by username or email, if that is not available in the app, I think we can do.
- Add the username or user email to the app response
- Add an endpoint into the backend to fetch user data by ID
I don't think we should move forward with the current solution.
hideCancel | ||
{fullNewSecret && app && ( | ||
<Dialog | ||
css={{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should avoid adding a new dialog style only for this case. Did you take a look in the other dialogs? Wondering what you want to achieve here that is not achievable by using the existent dialogs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I can refactor that into a component if you'd like, but none of the other dialogs allow resizing since the max width is always hardcoded.
When opening the code example in the dialog, I want it to expand both horizontally and vertically so that the word wrap on the curl command in the code example doesn't apply, and users can see the command they are about to copy and paste without having first to copy it and then inspect it in another editor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok, lets keep it as it is - in terms of design. I will give it a try and see what we can do to better support this use case in our design system.
OAuth2 client secret | ||
</h3> | ||
<div | ||
css={{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We are not using inline CSS styles anymore. Give a try toTailwindCSS class names. You can see many of them in the code base.
} | ||
/> | ||
<div css={{ marginTop: 24 }}> | ||
<Link |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks like it should be a button instead of a link. Links should be used for navigation.
@@ -61,7 +61,7 @@ export const Sidebar: FC<SidebarProps> = ({ user }) => { | |||
<SidebarNavItem href="ssh-keys" icon={FingerprintIcon}> | |||
SSH Keys | |||
</SidebarNavItem> | |||
<SidebarNavItem href="tokens" icon={KeyIcon}> | |||
<SidebarNavItem href="tokens" icon={KeyIcon} end={false}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Wondering why only these two have end set to false 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
None of the other items has nested menus. Only the tokens and OAuth2 app section can navigate to another route, thus making the nav item "unselected".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I will give this a try and see how it behaves.
Uh oh!
There was an error while loading.Please reload this page.
coderd/oauth2provider/apps.go Outdated
Valid: true, | ||
}) | ||
if err != nil { | ||
httpapi.InternalServerError(rw, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's also add a log here; we probably don't want to leak the error details back in the HTTP response, so let's just say something generic; admins can refer to the logs for details.
dannykoppingJul 16, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You're still returning the error directly; we probably don't want to do that; ditto for the other cases.
65b1054
tof044533
Compareac0d1f6
toef3c66e
Compare5f71ebd
toc84c4be
CompareThomasK33 commentedJul 17, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@BrunoQuaresma, sorry it took me a while, but here's a video showing off the client_credentials UI, including the expanding dialog: coder_client_credentials_ui.mp4 |
- Add ClientCredentialsAppForm and ClientCredentialsAppRow components- Update API schemas to include created_at, grant_types, and user_id fields- Add dedicated pages for creating and managing client credentials apps- Update sidebar navigation and routing for OAuth2 client credentials- Enhance OAuth2AppPageView with user ownership information displayChange-Id: I3271c7fb995d7225dd6cc830066fa2c8cb29720aSigned-off-by: Thomas Kosiewski <tk@coder.com>
ef3c66e
to3f1495c
Comparec84c4be
to40d7fd1
Compare
Add OAuth2 Client Credentials Grant Type Support
This PR adds support for OAuth2 client credentials grant type, enabling server-to-server API access. Key changes include:
created_at
,grant_types
, anduser_id
created_at
field to OAuth2ProviderAppSecret modelThis implementation provides a more secure alternative to long-lived API tokens by allowing users to create OAuth2 applications with client credentials grant type for server-to-server authentication.