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

API to get new refresh token for user #132

Merged
merged 1 commit into from
Feb 24, 2021

Conversation

sm43
Copy link
Member

@sm43 sm43 commented Nov 2, 2020

This adds implementation for API to get a new refresh token for user. This
requires existing user refresh token to be passed to get a new access token.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

See the contribution guide for more details.

@tekton-robot tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 2, 2020
@piyush-garg
Copy link
Contributor

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 9, 2020
@sm43
Copy link
Member Author

sm43 commented Nov 11, 2020

/hold
as discussed we will merge this with UI's for auth flow.

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 11, 2020
@sm43 sm43 force-pushed the refresh-token-api branch from c8ae249 to bf1a008 Compare November 18, 2020 19:16
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 18, 2020
@sm43 sm43 changed the title API to refresh user access token API to get new refresh token for user Nov 18, 2020
@sm43 sm43 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 18, 2020
@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2020
@sm43 sm43 force-pushed the refresh-token-api branch from bf1a008 to cd34518 Compare December 10, 2020 17:38
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 10, 2020
@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 24, 2020
@sm43 sm43 force-pushed the refresh-token-api branch from cd34518 to d69c3de Compare January 25, 2021 05:28
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 25, 2021
@sm43 sm43 force-pushed the refresh-token-api branch from d69c3de to 577d66d Compare January 25, 2021 05:42
@sm43 sm43 force-pushed the refresh-token-api branch 2 times, most recently from b0b8446 to c38c2df Compare February 22, 2021 12:24
@pratap0007
Copy link
Contributor

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 23, 2021
@sm43 sm43 force-pushed the refresh-token-api branch from c38c2df to 1dbb1e7 Compare February 24, 2021 05:35
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 24, 2021
@pratap0007
Copy link
Contributor

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 24, 2021
// Refreshes the refresh token of User
func (s *service) NewRefreshToken(ctx context.Context, p *user.NewRefreshTokenPayload) (*user.NewRefreshTokenResult, error) {

user, err := s.User(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

I think from line 135 to 151 we can create a common function and use this from both access token and refresh token since that part of code is duplicated

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 rest looks good

@sm43 sm43 force-pushed the refresh-token-api branch from 1dbb1e7 to 7da2684 Compare February 24, 2021 08:11
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 24, 2021
This adds an api to get new refresh token for user using existing
refresh token before it expires. Existing refresh token must be
passed in header as Authorization.

Signed-off-by: Shivam Mukhade <smukhade@redhat.com>
@sm43 sm43 force-pushed the refresh-token-api branch from 7da2684 to 8df86e8 Compare February 24, 2021 08:26
@piyush-garg
Copy link
Contributor

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 24, 2021
@PuneetPunamiya
Copy link
Member

/approve
/meow

@tekton-robot
Copy link

@PuneetPunamiya: cat image

In response to this:

/approve
/meow

Instructions 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/test-infra repository.

@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: PuneetPunamiya

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 24, 2021
@tekton-robot tekton-robot merged commit 9de6dbf into tektoncd:master Feb 24, 2021
@sm43 sm43 deleted the refresh-token-api branch April 15, 2021 14:51
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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants