Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
ThomasK33 wants to merge1 commit intothomask33/07-12-feat_oauth2_add_client_credentials_grant_type_and_user_ownership
base:thomask33/07-12-feat_oauth2_add_client_credentials_grant_type_and_user_ownership
Choose a base branch
Loading
fromthomask33/07-14-feat_oauth2_add_frontend_ui_for_client_credentials_applications

Conversation

ThomasK33
Copy link
Member

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:

  • Added new fields to OAuth2ProviderApp model:created_at,grant_types, anduser_id
  • Addedcreated_at field to OAuth2ProviderAppSecret model
  • Added revocation endpoint to OAuth2AppEndpoints
  • Created new UI components for managing client credentials applications:
    • ClientCredentialsAppForm for creating/editing applications
    • ClientCredentialsAppRow for displaying applications in a table
  • Added new routes for client credentials applications:
    • /settings/oauth2-provider/new for creating new applications
    • /settings/oauth2-provider/:appId for managing existing applications
  • Redesigned OAuth2 applications settings page to separate owned applications from authorized applications
  • Enhanced secret creation dialog with code examples for API usage
  • Updated API documentation to reflect new fields and endpoints

This 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.

@ThomasK33Graphite App
Copy link
MemberAuthor

ThomasK33 commentedJul 14, 2025
edited
Loading

@ThomasK33ThomasK33force-pushed thethomask33/07-12-feat_oauth2_add_client_credentials_grant_type_and_user_ownership branch fromc654948 toeac2681CompareJuly 14, 2025 16:22
@ThomasK33ThomasK33force-pushed thethomask33/07-14-feat_oauth2_add_frontend_ui_for_client_credentials_applications branch from8c29819 to168176bCompareJuly 14, 2025 16:22
@ThomasK33ThomasK33force-pushed thethomask33/07-12-feat_oauth2_add_client_credentials_grant_type_and_user_ownership branch fromeac2681 toac0d1f6CompareJuly 14, 2025 17:18
@ThomasK33ThomasK33force-pushed thethomask33/07-14-feat_oauth2_add_frontend_ui_for_client_credentials_applications branch 2 times, most recently from4fcf5b1 to65b1054CompareJuly 14, 2025 17:46
@ThomasK33ThomasK33 marked this pull request as ready for reviewJuly 14, 2025 18:09
@BrunoQuaresma
Copy link
Collaborator

I'm doing the review, but I'm really missing a video demoing the feature, or at least screenshots.

ThomasK33 reacted with thumbs up emoji

@ThomasK33Graphite App
Copy link
MemberAuthor

I'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 reacted with thumbs up emoji

Copy link
Collaborator

@BrunoQuaresmaBrunoQuaresma left a comment
edited
Loading

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?

@@ -1726,11 +1726,18 @@ class ApiMethods {
getOAuth2ProviderApps = async (
filter?: TypesGen.OAuth2ProviderAppFilter,
Copy link
Collaborator

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;};

ThomasK33 reacted with thumbs up emoji
@@ -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 }) => {
Copy link
Collaborator

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),};};

ThomasK33 reacted with thumbs up emoji
@@ -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;
Copy link
Collaborator

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?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

yes, that's possible

BrunoQuaresma reacted with thumbs up emoji
Copy link
Collaborator

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?

Copy link
MemberAuthor

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.

BrunoQuaresma reacted with thumbs up emoji
Copy link
Collaborator

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.

Copy link
Collaborator

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 theonSubmit 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.

ThomasK33 reacted with thumbs up emoji
event.preventDefault();
const formData = new FormData(event.target as HTMLFormElement);

onSubmit({
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know using the WEB API is nice, but it has some gaps and to make the code consistent lets useFormik andYup for validation.

You can easily find examples on how to use both in the codebase.

ThomasK33 reacted with thumbs up emoji
const [showCodeExample, setShowCodeExample] = useState(false);

// Fetch users and find the owner by user_id
const usersQuery = useQuery({
Copy link
Collaborator

Choose a reason for hiding this comment

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

⚠️ Blocker

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.

ThomasK33 reacted with thumbs up emoji
hideCancel
{fullNewSecret && app && (
<Dialog
css={{
Copy link
Collaborator

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.

ThomasK33 reacted with thumbs up emoji
Copy link
MemberAuthor

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.

Copy link
Collaborator

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={{
Copy link
Collaborator

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.

ThomasK33 reacted with thumbs up emoji
}
/>
<div css={{ marginTop: 24 }}>
<Link
Copy link
Collaborator

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.

ThomasK33 reacted with thumbs up emoji
@@ -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}>
Copy link
Collaborator

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 🤔

ThomasK33 reacted with thumbs up emoji
Copy link
MemberAuthor

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".

BrunoQuaresma reacted with eyes emoji
Copy link
Collaborator

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.

Valid: true,
})
if err != nil {
httpapi.InternalServerError(rw, err)
Copy link
Contributor

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.

ThomasK33 reacted with thumbs up emoji
Copy link
Contributor

@dannykoppingdannykoppingJul 16, 2025
edited
Loading

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.

@ThomasK33ThomasK33force-pushed thethomask33/07-14-feat_oauth2_add_frontend_ui_for_client_credentials_applications branch from65b1054 tof044533CompareJuly 15, 2025 17:27
@ThomasK33ThomasK33force-pushed thethomask33/07-12-feat_oauth2_add_client_credentials_grant_type_and_user_ownership branch fromac0d1f6 toef3c66eCompareJuly 15, 2025 17:27
@ThomasK33ThomasK33force-pushed thethomask33/07-14-feat_oauth2_add_frontend_ui_for_client_credentials_applications branch 2 times, most recently from5f71ebd toc84c4beCompareJuly 17, 2025 13:30
@ThomasK33
Copy link
MemberAuthor

ThomasK33 commentedJul 17, 2025
edited
Loading

@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>
@ThomasK33ThomasK33force-pushed thethomask33/07-12-feat_oauth2_add_client_credentials_grant_type_and_user_ownership branch fromef3c66e to3f1495cCompareJuly 17, 2025 14:38
@ThomasK33ThomasK33force-pushed thethomask33/07-14-feat_oauth2_add_frontend_ui_for_client_credentials_applications branch fromc84c4be to40d7fd1CompareJuly 17, 2025 14:38
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@dannykoppingdannykoppingdannykopping approved these changes

@aslilacaslilacAwaiting requested review from aslilacaslilac is a code owner

@BrunoQuaresmaBrunoQuaresmaAwaiting requested review from BrunoQuaresma

Assignees

@ThomasK33ThomasK33

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@ThomasK33@BrunoQuaresma@dannykopping

[8]ページ先頭

©2009-2025 Movatter.jp