Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

allow users list their own access tokens #24

Merged
merged 6 commits into from Nov 27, 2020
Merged

allow users list their own access tokens #24

merged 6 commits into from Nov 27, 2020

Conversation

stlaz
Copy link
Member

@stlaz stlaz commented Oct 21, 2020 •
edited

Adds an endpoint that mirrors the oauthaccesstokens that only belong to the requesting user. The new endpoint allows these operations:

  • get
  • list
  • watch
  • delete

cc @deads2k @sttts

enhancement ref: openshift/enhancements#507

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 21, 2020
@stlaz stlaz force-pushed the personal_tokens branch 2 times, most recently from d1c33f8 to 1e2c154 Compare October 22, 2020 08:50
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 23, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 23, 2020
@stlaz stlaz force-pushed the personal_tokens branch 2 times, most recently from ab1916b to 89cfe78 Compare October 26, 2020 12:36
sttts
sttts reviewed Oct 29, 2020
View reviewed changes
pkg/oauth/printers/internalversion/printer.go
created,
expires,
personalAccessToken.RedirectURI,
strings.Join(personalAccessToken.Scopes, ","),
Copy link
Contributor

Choose a reason for hiding this comment

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

@Anandnatraj these are the columns with oc.

@stlaz stlaz force-pushed the personal_tokens branch 6 times, most recently from eac485d to 5445619 Compare October 30, 2020 13:50
@stlaz
Copy link
Member Author

stlaz commented Oct 30, 2020

/retest
I wonder why it's a baremetal operator that failed on e2e-aws

@stlaz stlaz changed the title [prototype] allow users list their own access tokens allow users list their own access tokens Nov 2, 2020
@stlaz
Copy link
Member Author

stlaz commented Nov 2, 2020

Addressed all comments. The PR now depends on openshift/api#774 (aside from the enhancement merging)

@stlaz stlaz mentioned this pull request Nov 2, 2020
Merged
sttts
sttts reviewed Nov 23, 2020
View reviewed changes
test/e2e/useroauthaccesstokens_test.go
name: "set label selector to get own tokens and of others",
userToken: mirkaTokenString,
userName: "mirka",
labelSelector: parseLabelSelectorOrDie("randomLabel notin (mirka,franta)"),
Copy link
Contributor

Choose a reason for hiding this comment

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

would add the simple case too: userName == somebody else.

Copy link
Member Author

@stlaz stlaz Nov 24, 2020 •
edited

Choose a reason for hiding this comment

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

there's a fieldSelector like this, but I'll add a label to one of the basic tokens above and will attempt to list it, too

p0lyn0mial
p0lyn0mial reviewed Nov 24, 2020
View reviewed changes
pkg/oauth/apiserver/registry/useroauthaccesstokens/delegate/delegate.go Outdated Show resolved Hide resolved
pkg/oauth/apiserver/registry/useroauthaccesstokens/delegate/delegate.go

tokenListGenericObj, err := r.accessTokenStorage.List(ctx, sanitizedListOpts)
if err != nil {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

should we mask the err ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should be fine with the error as is

pkg/oauth/apiserver/registry/useroauthaccesstokens/delegate/delegate.go

tokenList, ok := tokenListGenericObj.(*oauthapi.OAuthAccessTokenList)
if !ok {
return nil, errors.NewInternalError(fmt.Errorf("failed to convert generic accesstoken LIST result to its typed version"))
Copy link
Contributor

Choose a reason for hiding this comment

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

could we be more specific and log the underlying type for tokenListGenericObj and OAuthAccessTokenList ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This error gets propagated to the user, does it not? I think it might be confusing when they see a mention of a different type from what they requested

Copy link
Contributor

Choose a reason for hiding this comment

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

When the conversion can fail? Can it only happen when the wiring is wrong?

In that case, I would prefer to see a very clear and specific error so that I can debug it.

pkg/oauth/apiserver/registry/useroauthaccesstokens/delegate/delegate.go Show resolved Hide resolved
pkg/oauth/apiserver/registry/useroauthaccesstokens/delegate/delegate.go Outdated Show resolved Hide resolved
pkg/oauth/apiserver/registry/useroauthaccesstokens/delegate/proxywatcher.go Show resolved Hide resolved
pkg/oauth/apiserver/registry/useroauthaccesstokens/delegate/proxywatcher.go Outdated Show resolved Hide resolved
pkg/oauth/apiserver/registry/useroauthaccesstokens/delegate/proxywatcher.go Outdated Show resolved Hide resolved
pkg/oauth/apiserver/registry/useroauthaccesstokens/delegate/delegate.go Show resolved Hide resolved
@stlaz
Copy link
Member Author

stlaz commented Nov 25, 2020

/retest

@sttts
Copy link
Contributor

sttts commented Nov 25, 2020

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 25, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stlaz, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 25, 2020
@stlaz
Copy link
Member Author

stlaz commented Nov 25, 2020 •
edited

The component tests are currently failing because openshift/cluster-authentication-operator#381 hasn't merged yet

edit: and it won't because we test our default RBAC policies... (I did not realize that)

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@stlaz
Copy link
Member Author

stlaz commented Nov 25, 2020

/hold
wait for rbac deps to merge

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 25, 2020
@sttts
Copy link
Contributor

sttts commented Nov 26, 2020

/retest

@stlaz
Copy link
Member Author

stlaz commented Nov 26, 2020

we still need openshift/cluster-authentication-operator#381 for the tests to pass :/

@stlaz
Copy link
Member Author

stlaz commented Nov 27, 2020

/hold cancel
the operator PR merged. We'll probably need a few retests before the RBAC is actually there

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 27, 2020
@stlaz
Copy link
Member Author

stlaz commented Nov 27, 2020

/retest

@openshift-merge-robot openshift-merge-robot merged commit 7a3ac7c into openshift:master Nov 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

@deads2k deads2k deads2k left review comments

@p0lyn0mial p0lyn0mial p0lyn0mial left review comments

@ericavonb ericavonb Awaiting requested review from ericavonb

Assignees

@sttts sttts

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@stlaz @sttts @openshift-ci-robot @openshift-bot @deads2k @p0lyn0mial @openshift-merge-robot