Conversation
- Add a full self-service My Apps area for authenticated users to create, view, edit, and delete their own OAuth applications - Introduce an approval lifecycle for OAuth clients with pending, active, and inactive states - Require admin approval for user-created clients while keeping admin-created clients immediately active - Add admin approve and reject actions with audit logging and UI controls - Enforce ownership and scope restrictions for user-managed clients at the service layer - Prevent deletion of active clients and require admin rejection before removal - Add status filtering and pending counts to the admin clients list and navbar badge - Refactor grant type handling into a shared helper and align client status persistence - Centralize shared client form, search, pagination, and status badge templates - Add comprehensive tests covering user client workflows, approval, and validation rules Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a self-service /apps area so authenticated users can manage their own OAuth applications while introducing an admin approval lifecycle (pending → active / inactive) for user-created clients, plus admin UI/actions for approving/rejecting.
Changes:
- Introduces client approval Status (
pending|active|inactive) and admin approve/reject actions with audit events. - Adds user-facing My Apps pages (list/create/detail/edit/delete + secret regeneration) with scope/grant restrictions.
- Refactors shared templates (form fields, status badge, search, pagination) and adds admin list status filtering + pending-count badge.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/templates/user_app_form.templ | User create/edit app form using shared client fields component |
| internal/templates/user_app_detail.templ | User app detail page with status messaging and actions |
| internal/templates/user_app_created.templ | Post-create (and secret regeneration) “show secret once” page |
| internal/templates/static/css/pages/admin-clients.css | Adds button styles for approve/reject controls |
| internal/templates/props.go | Adds navbar pending-count + admin status filter + user-app props structs |
| internal/templates/navbar_component.templ | Adds “My Apps” link + admin pending badge rendering |
| internal/templates/my_apps.templ | User “My Apps” list with search + pagination + status badge |
| internal/templates/client_shared.templ | New shared status badge, search bar, pagination, and shared form fields |
| internal/templates/admin_clients.templ | Adds status tabs + inline approve/reject + uses shared components |
| internal/templates/admin_client_form.templ | Refactors admin form to use shared common fields |
| internal/templates/admin_client_detail.templ | Adds status badges + pending approval action buttons |
| internal/store/sqlite.go | Adds status filtering, per-user listing, and pending count query support |
| internal/store/pagination.go | Extends pagination params with optional StatusFilter |
| internal/services/client_user_test.go | Adds tests for user workflows, approval, ownership, scopes, deletion rules |
| internal/services/client.go | Implements status lifecycle, user update/delete, approve/reject, pending count |
| internal/models/oauth_application.go | Adds Status field + status constants |
| internal/models/audit_log.go | Adds audit event constants for approve/reject |
| internal/handlers/user_client.go | New /apps handler: list/create/view/edit/delete/regenerate secret |
| internal/handlers/client.go | Adds admin status filter handling + approve/reject endpoints + navbar badge |
| internal/bootstrap/router.go | Adds /apps routes and admin approve/reject routes |
| internal/bootstrap/handlers.go | Wires new UserClientHandler into handler set |
Comments suppressed due to low confidence (8)
internal/templates/admin_clients.templ:36
- The status filter isn’t preserved when submitting the search form:
ClientsSearchBar("/admin/clients", ...)will drop the current?status=...selection, so searching within the Pending/Active/Inactive views resets to "All". Consider passing the current status in the form action (or adding a hiddenstatusinput) so filters and search compose correctly.
<!-- Status Filter Tabs -->
<div style="display:flex;gap:var(--space-2);margin-bottom:var(--space-4);border-bottom:1px solid var(--color-border);padding-bottom:var(--space-3);">
@AdminClientsStatusTab("/admin/clients", "All", props.StatusFilter == "")
@AdminClientsStatusTab("/admin/clients?status=pending", "Pending", props.StatusFilter == models.ClientStatusPending)
@AdminClientsStatusTab("/admin/clients?status=active", "Active", props.StatusFilter == models.ClientStatusActive)
@AdminClientsStatusTab("/admin/clients?status=inactive", "Inactive", props.StatusFilter == models.ClientStatusInactive)
</div>
@ClientsSearchBar("/admin/clients", props.Search, props.PageSize)
internal/templates/admin_clients.templ:86
- Pagination links also ignore the active status filter:
ClientsListPagination(..., "/admin/clients", ...)always uses the unfiltered base URL, so clicking Next/Previous while on?status=pendingdrops the filter. Pass a baseURL that includes the currentStatusFilter(or extend the pagination helper to accept additional query params).
@ClientsListPagination(props.Pagination, props.PageSize, props.Search, "/admin/clients", "total clients")
internal/handlers/client.go:55
statusFilter := c.Query("status")is passed straight through toparams.StatusFilter. If an unknown value is provided, the UI ends up with no tab selected and the list can become confusingly empty. Consider validating against the supported set ("", pending/active/inactive) and defaulting to "" when invalid.
search := c.Query("search")
statusFilter := c.Query("status")
// Create pagination params (with optional status filter)
params := store.NewPaginationParams(page, pageSize, search)
params.StatusFilter = statusFilter
internal/templates/admin_client_detail.templ:89
- This template hard-codes status strings ("pending" / "inactive") even though
models.ClientStatus*constants exist and are used elsewhere. Importinternal/modelsand compare against those constants to avoid drift and typos when statuses evolve.
switch props.Client.Status {
case "pending":
<span class="status-badge" style="background:rgba(245,158,11,0.1);color:#D97706;border:1px solid rgba(245,158,11,0.3);">Pending approval</span>
case "inactive":
<span class="status-badge status-inactive">Rejected</span>
}
internal/templates/admin_client_detail.templ:103
- The pending-actions section uses a string literal comparison (
props.Client.Status == "pending"). Prefermodels.ClientStatusPending(and import models) so the condition stays consistent with the rest of the codebase.
<!-- Pending Approval Actions -->
if props.Client.Status == "pending" {
<div style="margin-bottom:var(--space-4);padding:var(--space-3);background:rgba(245,158,11,0.1);border:1px solid rgba(245,158,11,0.3);border-radius:var(--radius-md);">
internal/handlers/user_client.go:107
- The allowed-scope list is duplicated here (switch on "email/profile/openid/offline_access") and again in the service (
allowedUserScopes+validateUserScopes). This duplication is likely to drift over time; consider centralizing the allowlist (e.g., expose a helper from the service layer or a shared package) and using it both for validation and for generating the user-facing error message.
// Validate scopes before calling service to give a user-friendly error
if req.Scopes != "" {
for scope := range strings.FieldsSeq(req.Scopes) {
switch scope {
case "email", "profile", "openid", "offline_access":
// ok
default:
renderUserAppForm(
c,
userModel,
nil,
"/apps",
false,
"Invalid scope: "+scope+". Allowed scopes are: email, profile, openid, offline_access",
)
return
}
}
}
internal/templates/client_shared.templ:65
buildClientPaginationURLconcatenates thesearchquery param without URL-encoding. Searches containing spaces,&,?, etc. will produce broken links and allow query-parameter injection (e.g. appending&status=pending). Build the URL vianet/url(or at leasturl.QueryEscape(search)) so the generated pagination links are well-formed and preserve the intended search value.
func buildClientPaginationURL(baseURL string, page, pageSize int, search string) string {
url := fmt.Sprintf("%s?page=%d&page_size=%d", baseURL, page, pageSize)
if search != "" {
url += "&search=" + search
}
return url
internal/templates/user_app_detail.templ:99
- The
Regenerate Secretaction is implemented as a simple link that issues a GET request to/apps/:id/regenerate-secret, butCSRFMiddlewareonly validates CSRF tokens on POST/PUT/DELETE/PATCH methods. This means a malicious site can cause an authenticated user who owns a confidential app to silently rotate its client secret by embedding this URL (e.g., in an<img>tag), changing a security-critical credential without the user’s intent. To mitigate this, make the secret rotation endpoint POST-only and invoke it via a form that includes thecsrf_token(or otherwise enforce CSRF validation for this operation).
if props.Client.ClientType == "confidential" {
<a
href={ templ.URL("/apps/" + props.Client.ClientID + "/regenerate-secret") }
class="admin-action-btn secondary"
onclick="return confirm('Are you sure? This will invalidate the current secret.');"
>
🔄 Regenerate Secret
</a>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
161
to
195
| // Default scopes | ||
| scopes := strings.TrimSpace(req.Scopes) | ||
| if scopes == "" { | ||
| scopes = "email profile" | ||
| } | ||
|
|
||
| // Default client type | ||
| clientType := req.ClientType | ||
| if clientType != ClientTypePublic { | ||
| clientType = ClientTypeConfidential | ||
| } | ||
|
|
||
| // Client credentials flow is only available for confidential clients | ||
| enableClientCredentials := req.EnableClientCredentialsFlow && | ||
| clientType == ClientTypeConfidential | ||
|
|
||
| // If neither flow is explicitly enabled, default to device flow | ||
| enableDevice := req.EnableDeviceFlow | ||
| enableAuthCode := req.EnableAuthCodeFlow | ||
| if !enableDevice && !enableAuthCode && !enableClientCredentials { | ||
| enableDevice = true | ||
| } | ||
|
|
||
| // Derive GrantTypes string from the enabled flows | ||
| var grants []string | ||
| if enableDevice { | ||
| grants = append(grants, "device_code") | ||
| } | ||
| if enableAuthCode { | ||
| grants = append(grants, "authorization_code") | ||
| } | ||
| if enableClientCredentials { | ||
| grants = append(grants, "client_credentials") | ||
| grantTypes := buildGrantTypes(enableDevice, enableAuthCode, enableClientCredentials) | ||
|
|
||
| // Determine approval status based on creator role. | ||
| // Admin-created clients are immediately active; user-created clients require approval. | ||
| clientStatus := models.ClientStatusPending | ||
| isActive := false | ||
| if req.IsAdminCreated { | ||
| clientStatus = models.ClientStatusActive | ||
| isActive = true | ||
| } | ||
| grantTypes := strings.Join(grants, " ") | ||
|
|
There was a problem hiding this comment.
CreateClient sets Status=pending for non-admin-created clients but does not enforce the user scope allowlist at the service layer. A caller that bypasses the HTML handler could create a pending client with arbitrary scopes (contradicting the stated scope restrictions). Consider calling validateUserScopes(scopes) when !req.IsAdminCreated and returning ErrInvalidScopeForUser on violation.
- Add a middleware that injects the pending client count into the request context for admin users. - Apply the pending client count middleware to all authenticated route groups so the navbar badge is available site-wide. - Refactor navbar construction to read the pending client count from the request context instead of querying per handler. - Update all handlers to pass the gin context into the navbar builder to support context-aware data. - Remove duplicated pending client count queries from individual client handlers. Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix #73