feat(oauth): expose clientId in OAuthAuthenticatorDescriptor - #1035
feat(oauth): expose clientId in OAuthAuthenticatorDescriptor#1035olexii4 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: olexii4 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi! I'm che-ai-assistant — I help with your pull requests. Available commands:
|
|
/che-ai-assistant ok-pr-review Task completed. |
|
/retest |
089d0e1 to
e9f6223
Compare
tolusha
left a comment
tolusha
left a comment
There was a problem hiding this comment.
Test Coverage
The PR adds new API surface in three files but includes no test additions or updates. The existing shouldGetRegisteredAuthenticators test in EmbeddedOAuthAPITest only checks the result-set size, not the descriptor contents. Consider adding tests for:
OAuthAuthenticator.getClientId()returns the value passed toconfigure()EmbeddedOAuthAPI.getRegisteredAuthenticators()setsclientIdfor OAuth2 providers andnullfor OAuth1 providers- DTO serialization round-trips the
clientIdfield correctly
|
Docker image build succeeded: quay.io/eclipse/che-server:pr-1035 kubectl patch commandkubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/cheServer/deployment", "value": {containers: [{image: "quay.io/eclipse/che-server:pr-1035", name: che}]}}]" |
e9f6223 to
00d1ce2
Compare
Add clientId field to OAuthAuthenticatorDescriptor DTO so clients can initiate OAuth flows that require the client ID directly (e.g. GitHub Device Authorization Flow / RFC 8628) without needing access to the raw Kubernetes Secret. - OAuthAuthenticator: store clientId passed to configure() and expose it via getClientId() - EmbeddedOAuthAPI: populate clientId in the descriptor returned by GET /api/oauth - OAuthAuthenticatorDescriptor: new clientId getter/setter/builder Signed-off-by: Oleksii Orel <oorel@redhat.com>
00d1ce2 to
cb5afc8
Compare
|
Docker image build succeeded: quay.io/eclipse/che-server:pr-1035 kubectl patch commandkubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/cheServer/deployment", "value": {containers: [{image: "quay.io/eclipse/che-server:pr-1035", name: che}]}}]" |
|
/retest |
|
/che-ai-assistant ok-pr-review |
|
@olexii4: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
What does this PR do?
Adds
clientIdto theOAuthAuthenticatorDescriptorDTO returned byGET /api/oauth, so clients can initiate OAuth flows that require the client ID directly — specifically the GitHub Device Authorization Flow (RFC 8628).Changes
OAuthAuthenticatorDescriptor— newclientIdfield (getter / setter / builder)OAuthAuthenticator— stores theclientIdpassed toconfigure()and exposes it viagetClientId()EmbeddedOAuthAPI— populatesclientIdin the descriptor returned byGET /api/oauthWhy
The Che Dashboard's Device Auth Tokens tab (che-dashboard#1633) needs to call GitHub's device flow API directly, which requires the OAuth App client ID. Currently there is no way for the dashboard to obtain this value without:
With this change, the dashboard calls
GET /api/oauthwith its service account token — the same call it already makes for the Git Services tab — and receives the client ID in the response. No additional RBAC, env vars, or volume mounts needed.Backward compatibility
The new
clientIdfield is optional in the DTO. Existing consumers ofGET /api/oauththat don't useclientIdare unaffected. OAuth1 providers (whereauthenticator.getClientId()would returnnull) simply omit the field.What issues does this PR fix or reference?
references https://redhat.atlassian.net/browse/CRW-11582