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.

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

new/auth: allow users to manage their tokens #507

Merged
merged 1 commit into from
Nov 9, 2020

Conversation

stlaz
Copy link
Contributor

@stlaz stlaz commented Oct 22, 2020

An enhancement mostly about listing/deletion of personal access tokens

/cc @deads2k @sttts @marun @vareti

@stlaz stlaz force-pushed the usertokens branch 2 times, most recently from 544a84b to 1d87885 Compare October 27, 2020 13:56

## Motivation

It is quite easy for users to retrieve tokens but it is also not so hard
Copy link
Contributor

Choose a reason for hiding this comment

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

s/retrieve/create/ ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

retrieve seems more appropriate to me given that they cannot create their tokens


### Non-Goals

- enable handling sessions of services that use the OpenShift access tokens
Copy link
Contributor

Choose a reason for hiding this comment

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

what would "handle" mean here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pretty much anything regarding a session, we are not dealing with sessions in this enhancement at all

Copy link
Contributor

Choose a reason for hiding this comment

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

  • Changing the granularity of tokens to sessions: tokens can be used by many sessions. The oauth system is not aware of sessions and won't be with this enhancement.


#### Story 3
A user is able to perform actions from Story 1 and Story 2 in both the web
console and the CLI clients.
Copy link
Contributor

Choose a reason for hiding this comment

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

@jhadvig fyi, there is work coming up for the console. Please sync with @Anandnatraj about the timing. We might merge the API already with 4.7.

the default token name length, it is quite improbable). The clusterrolebinding
`system:oauth-token-deleters` will therefore get deprecated as described in
[Graduation Criteria - Removing a Deprecated Feature](#graduation-criteria---removing-a-deprecated-feature).

Copy link
Contributor

Choose a reason for hiding this comment

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

add section about table view in oc, i.e. which columns will we expose by default? @Anandnatraj this will be something for you to review.

Copy link
Contributor Author

@stlaz stlaz Oct 30, 2020

Choose a reason for hiding this comment

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

added - see below.

@Anandnatraj please review so that we only keep the fields necessary

Choose a reason for hiding this comment

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

@stlaz I cannot see the table. Can you slack me a screenshot pls?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Anandnatraj the list 5 lines below are the columns of the table view of oc get useroauthaccesstokens.

command line:

- metadata.name (`NAME`)
- userName (`USERNAME`)
Copy link
Contributor

Choose a reason for hiding this comment

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

is user name really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, probably not

- metadata.name (`NAME`)
- userName (`USERNAME`)
- clientName (`CLIENT NAME`)
- metadata.creationTimestamp (`CREATED`)
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this a default field shown at the very end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we're defining our own table printer, I don't think it would be printed if we don't specifically add it

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe that is different in CRDs. I remotely remember something like that.

### Test Plan

New e2e tests:
1. using the new endpoint, a user can get/list/watch/delete all of the tokens that were issued to them
2. a user cannot get/list/watch/delete tokens for any other user
3. user cannot create, update, patch tokens, neither his own, nor of other users.
4. non-sha256 tokens are not returned by any of the endpoints.
Copy link
Contributor

Choose a reason for hiding this comment

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

any of the new endpoints

@sttts
Copy link
Contributor

sttts commented Nov 9, 2020

/approve
LGTM

Title must be fixed

@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 9, 2020
@sttts
Copy link
Contributor

sttts commented Nov 9, 2020

/lgtm

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

sttts commented Nov 9, 2020

/lgtm

@openshift-ci-robot
Copy link

[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 lgtm Indicates that a PR is ready to be merged. label Nov 9, 2020
@openshift-merge-robot openshift-merge-robot merged commit 0b141ce into openshift:master Nov 9, 2020
command line:

- metadata.name (`NAME`)
- clientName (`CLIENT NAME`)
Copy link
Contributor

Choose a reason for hiding this comment

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

@stlaz what will you see here in practice in a real cluster?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Anandnatraj this is the field I briefly mentioned. It will probably be a constant string like "console" or so, even if the token is used with oc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

6 participants