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

Migrate User API's to auth server #325

Merged

Conversation

PuneetPunamiya
Copy link
Member

@PuneetPunamiya PuneetPunamiya commented Sep 28, 2021

  • Removes user design from Hub Api Server

    • This patch removes the user design from Hub Api
      server and the corresponding service logic for
      user endpoints

  • Adds implementation and test for user info in auth server

    • Adds /user/info api which returns user related
      information in new auth server

    • Adds a middleware which implements the authorization
      logic for services

    • Adds test for /user/info api in auth server


  • Adds implementation for User Refresh Access Token in auth server

    • Adds /user/refresh/accesstoken api to get new refresh access
      token for the user

    • Adds test for /user/refresh/accesstoken api in auth server


  • Adds implementation for New Refresh Token for User in auth server

    • Adds /user/refresh/refreshtoken api to get new refresh
      token for the user

    • Adds test for /user/refresh/refreshtoken api in auth server


Signed-off-by: Puneet Punamiya [email protected]

Changes

Submitter Checklist

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

  • Includes tests (if functionality changed/added)
  • Run API Unit Tests, Lint Checks, API Design, Golden Files with make api-check
  • Run UI Unit Tests, Lint Checks with make ui-check
  • Commit messages follow commit message best practices

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 Sep 28, 2021
Where("code = ?", code)

err := q.First(&User).Error
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if err != nil {
if err:=r.db.Model(&model.User{}).Where("code = ?", code).First(&User).Error; err != nil {

@pratap0007
Copy link
Contributor

pratap0007 commented Oct 4, 2021

@PuneetPunamiya In case of expired refresh token user/refresh/refreshtoken api returns status code 400 , should it return 401?

@PuneetPunamiya PuneetPunamiya force-pushed the migrate-user-to-auth-server branch 2 times, most recently from 4959d36 to 665608d Compare October 11, 2021 09:56
return nil, err
}

if user.RefreshTokenChecksum != createChecksum(token) {
Copy link
Contributor

Choose a reason for hiding this comment

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

token contains Header along with token value and needs to split the token value

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean Bearer + token ??

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2021
}

// Get the user Info
func (s *UserService) Info(res http.ResponseWriter, req *http.Request) {
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 validate the access token here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we are using a middleware it verifies and validates the access token whenever a request is made to the endpoint

@tekton-robot tekton-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 18, 2021
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2021
// See the License for the specific language governing permissions and
// limitations under the License.

package base
Copy link
Member

@sm43 sm43 Nov 22, 2021

Choose a reason for hiding this comment

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

package user?

@sm43 sm43 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 6, 2021
@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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

 - This patch removes the user design from Hub Api
    server and the corresponding service logic for
    user endpoints

Signed-off-by: Puneet Punamiya <[email protected]>
  This patch

    - Adds `/user/info` api which returns user related
      information in new auth server

    - Adds a middleware which implements the authorization
      logic for services

    - Adds test for `/user/info` api in auth server

Signed-off-by: Puneet Punamiya <[email protected]>
  This patch

    - Adds `/user/refresh/accesstoken` api to get new refresh access
      token for the user

    - Adds test for `/user/refresh/accesstoken` api in auth server

Signed-off-by: Puneet Punamiya <[email protected]>
  This patch

    - Adds `/user/refresh/refreshtoken` api to get new refresh
      token for the user

    - Adds test for `/user/refresh/refreshtoken` api in auth server

Signed-off-by: Puneet Punamiya <[email protected]>
Copy link
Member

@vinamra28 vinamra28 left a comment

Choose a reason for hiding this comment

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

/lgtm

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

/test pull-tekton-hub-integration-tests

@tekton-robot tekton-robot merged commit f5ca4fd into tektoncd:main Dec 7, 2021
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