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

Authentication callback #12

Merged
merged 7 commits into from
Sep 11, 2023
Merged

Authentication callback #12

merged 7 commits into from
Sep 11, 2023

Conversation

VallariAg
Copy link
Member

@VallariAg VallariAg commented Aug 14, 2023 •
edited
Loading

  1. Redirect response to PULPITO
  2. Sends user data as cookie "GH_USER" (github username)

Pulpito PR: ceph/pulpito-ng#52

@VallariAg VallariAg mentioned this pull request Aug 14, 2023
Merged
3 tasks
@VallariAg VallariAg requested review from zmc and kamoltat August 16, 2023 16:45
@VallariAg VallariAg force-pushed the auth-callback branch 5 times, most recently from af5275d to 939fb97 Compare August 17, 2023 12:36
@zmc
Copy link
Member

zmc commented Aug 17, 2023

With these environment variables:

      PADDLES_URL: http://smithi085.front.sepia.ceph.com:8080/
      PULPITO_URL: http://smithi085.front.sepia.ceph.com:8081/
      GH_CLIENT_ID:
      GH_CLIENT_SECRET:
      GH_AUTHORIZATION_BASE_URL: 'https://github.com/login/oauth/authorize'
      GH_TOKEN_URL: 'https://github.com/login/oauth/access_token'
      GH_FETCH_MEMBERSHIP_URL: 'https://api.github.com/user/memberships/orgs/ceph'

I'm getting redirected to https://github.com/login/oauth/authorize?client_id=None&scope=read:org
I realize I'll need a GH_CLIENT_ID and GH_CLIENT_SECRET; should I create those myself?

I do think that some error handling would be good here, and also some documentation for how to configure this.

@VallariAg
Copy link
Member Author

I realize I'll need a GH_CLIENT_ID and GH_CLIENT_SECRET; should I create those myself?

I think you might have to create them. I remember that Junior shared secrets of his oauth app setup with us over an email, but the callback URL there would be localhost.
You can follow these steps and create a new github oauth app, I'll add proper instructions in the README.

I do think that some error handling would be good here, and also some documentation for how to configure this.

Sounds right, I'll add error handling and documentation for this!

@VallariAg
Copy link
Member Author

@kamoltat please check the changes in README and .env.dev in case I missed any setup instruction for github oauth

@VallariAg VallariAg force-pushed the auth-callback branch 3 times, most recently from 5b80d1f to c33bcc0 Compare August 21, 2023 10:39
kamoltat
kamoltat requested changes Aug 21, 2023
View reviewed changes
Copy link
Member

@kamoltat kamoltat left a comment

Choose a reason for hiding this comment

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

Just minor comment, other than that should be good, thank you for making this change!

.env.dev Outdated Show resolved Hide resolved
kamoltat
kamoltat approved these changes Aug 23, 2023
View reviewed changes
Copy link
Member

@kamoltat kamoltat left a comment

Choose a reason for hiding this comment

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

Okay this is good now!

@kamoltat
Copy link
Member

signed commit issue ...

@VallariAg
Copy link
Member Author

Need to merge #14 and then I'll rebase this

@kamoltat
Copy link
Member

@VallariAg Just waiting for revert on #11

@VallariAg
Copy link
Member Author

@kamoltat I clicked on revert and it opened this PR: #15
Looks like github creates a new branch revert-<PR-number>-<branch-name> with commits that revert the original PR's commits but since there was already that branch existing with your commit, it just asked me to open PR against that.

@kamoltat kamoltat closed this Aug 30, 2023
@kamoltat kamoltat reopened this Aug 30, 2023
kamoltat
kamoltat approved these changes Aug 30, 2023
View reviewed changes
Copy link
Member

@kamoltat kamoltat left a comment

Choose a reason for hiding this comment

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

LGTM

@VallariAg
Login callback: redirect to pulpito with session cookies
79c2cdf 
Signed-off-by: Vallari <[email protected]>
@VallariAg
main: change 'allow_origins' from * to URLs
c22ea9e 
To send cross site requests from pulpito-ng with `withCredentials`,
the server cannot have `*` as allowed origins.

Signed-off-by: Vallari <[email protected]>
@VallariAg
fix cookie_data.items() error
50ff7d0 
Signed-off-by: Vallari <[email protected]>
@VallariAg
add .env.dev
b1e4ea7 
Signed-off-by: Vallari <[email protected]>
@kamoltat kamoltat added enhancement Improving existing feature or logic authentication Authentication related PRs labels Sep 11, 2023
@kamoltat
Copy link
Member

Tested this locally and things went smoothly

@kamoltat kamoltat merged commit 8ca5514 into main Sep 11, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kamoltat kamoltat kamoltat approved these changes

@zmc zmc Awaiting requested review from zmc

Assignees

@VallariAg VallariAg

Labels
authentication Authentication related PRs enhancement Improving existing feature or logic
Projects
Pulpito Next Gen + Friendly Teuthology
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@VallariAg @zmc @kamoltat