kredits/hubot-kredits
9
0
Fork 0
Code Issues 8 Pull Requests Releases 9 Activity

Add GitHub signup oracle #41

Merged
bumi merged 13 commits from feature/github-signup into master 2019-08-28 13:56:54 +00:00
Conversation 26 Commits 13 Files Changed 4
+955 -245
bumi commented 2019-07-11 13:47:00 +00:00 (Migrated from github.com)
Copy Link
Copy Source

This adds a GitHub oauth signup oracle that allows users to create a new contributor profile through a oauth verification.

hubot implements an API and the oracle to create the contributor. The frontend is handled by kredits-web.

Flow:
kredits web -> reditects to on hubot: /kredits/signup/github/connect/github -> user gets redirected to github -> redirected to kredits web with access token -> API call to hubot POST /kredits/signup/github to create the contributor.

This adds a GitHub oauth signup oracle that allows users to create a new contributor profile through a oauth verification. hubot implements an API and the oracle to create the contributor. The frontend is handled by kredits-web. Flow: kredits web -> reditects to on hubot: `/kredits/signup/github/connect/github` -> user gets redirected to github -> redirected to kredits web with access token -> API call to hubot POST `/kredits/signup/github` to create the contributor.
galfert commented 2019-07-26 02:44:03 +00:00 (Migrated from github.com)
Copy Link
Copy Source

I finished the implementation. This should all work together with 67P/kredits-web#142 now.

When creating the Github OAuth app, the Authorization callback URL should be the base URL of HAL8000 (e.g. http://localhost:8888 for development).

When starting HAL8000, set the following new ENV vars:

I haven't tested the actual creation of the contributor yet (i.e. this part), because I don't have a full local Kredits contracts setup yet. For testing, I just added the contributorAttr object to the response and logged it in Kredits Web.

I finished the implementation. This should all work together with 67P/kredits-web#142 now. When creating the Github OAuth app, the Authorization callback URL should be the base URL of HAL8000 (e.g. http://localhost:8888 for development). When starting HAL8000, set the following new ENV vars: * `GITHUB_KEY` * `GITHUB_SECRET` * `KREDITS_WEB_URL` (http://localhost:4200 for development) I haven't tested the actual creation of the contributor yet (i.e. [this part](https://github.com/67P/hubot-kredits/pull/41/files#diff-562725e9559c31e5fbdad85240ac316dR250-R258)), because I don't have a full local Kredits contracts setup yet. For testing, I just added the `contributorAttr` object to the response and logged it in Kredits Web.
bumi (Migrated from github.com) reviewed 2019-07-26 09:09:14 +00:00
integrations/github.js
@@ -175,0 +219,4 @@
headers: {
'Accept': 'application/vnd.github.v3+json',
'Authorization': `token ${accessToken}`
}
bumi (Migrated from github.com) commented 2019-07-26 09:09:14 +00:00
Copy Link
Copy Source

why don't you directly reditect to kredits-web? - using the grant callback option?

why don't you directly reditect to kredits-web? - using the grant callback option?
galfert (Migrated from github.com) reviewed 2019-07-26 09:15:04 +00:00
integrations/github.js
@@ -175,0 +219,4 @@
headers: {
'Accept': 'application/vnd.github.v3+json',
'Authorization': `token ${accessToken}`
}
galfert (Migrated from github.com) commented 2019-07-26 09:15:03 +00:00
Copy Link
Copy Source

That first redirect should go to the backend server, because that one only contains a (temporary) code that you can then use together with the client secret to exchange it for the actual access token. But you shouldn't put the client secret in a client-side web app.

That first redirect should go to the backend server, because that one only contains a (temporary) code that you can then use together with the client secret to exchange it for the actual access token. But you shouldn't put the client secret in a client-side web app.
galfert (Migrated from github.com) reviewed 2019-07-26 09:17:23 +00:00
integrations/github.js
@@ -175,0 +219,4 @@
headers: {
'Accept': 'application/vnd.github.v3+json',
'Authorization': `token ${accessToken}`
}
galfert (Migrated from github.com) commented 2019-07-26 09:17:22 +00:00
Copy Link
Copy Source

Also, the endpoint to retrieve the access token using the code and client secret (POST https://github.com/login/oauth/access_token) doesn't have CORS headers, so requests from Web apps wouldn't work.

Also, the endpoint to retrieve the access token using the code and client secret (POST https://github.com/login/oauth/access_token) doesn't have CORS headers, so requests from Web apps wouldn't work.
raucao (Migrated from github.com) requested changes 2019-07-26 10:31:50 +00:00
raucao (Migrated from github.com) left a comment
Copy Link
Copy Source

Noice! Added some code comments and questions.

Noice! Added some code comments and questions.
integrations/github.js
raucao (Migrated from github.com) commented 2019-07-26 09:50:32 +00:00
Copy Link
Copy Source

I think we need to define the signup paths here, if we don't want to apply these things to all hubot routes.

I think we need to define the signup paths here, if we don't want to apply these things to all hubot routes.
raucao (Migrated from github.com) commented 2019-07-26 09:51:36 +00:00
Copy Link
Copy Source

This function could easily go in its own file.

This function could easily go in its own file.
raucao (Migrated from github.com) commented 2019-07-26 09:53:25 +00:00
Copy Link
Copy Source

I think default options should be set at the top of the file instead of directly in a router function.

I think default options should be set at the top of the file instead of directly in a router function.
raucao (Migrated from github.com) commented 2019-07-26 09:54:21 +00:00
Copy Link
Copy Source

Why JSON? Shouldn't sendStatus(200) be enough?

Why JSON? Shouldn't `sendStatus(200)` be enough?
integrations/github.js
@@ -175,0 +246,4 @@
contributorAttr.github_username = user.login;
contributorAttr.github_uid = user.id;
kredits.Contributor.add(contributorAttr, { gasLimit: 350000 })
raucao (Migrated from github.com) commented 2019-07-26 10:30:08 +00:00
Copy Link
Copy Source

Would be useful to know from where and how the contributor was added exactly. We use the [hubot-kredits] prefix in the other files. And could add something like "Contributor added from GitHub signup".

However, creating the contributor needs to be separated from the GitHub hook, because in order to create a more complete profile, we should be able to add more accounts before creating the contributor on chain. This is important, because the bot shouldn't have permission to update hashes afterwards, only to create new contributor records.

Splitting that out can be done in a future PR, of course, but maybe it makes sense to at least split out the functionality from the route already.

Would be useful to know from where and how the contributor was added exactly. We use the `[hubot-kredits]` prefix in the other files. And could add something like "Contributor added from GitHub signup". However, creating the contributor needs to be separated from the GitHub hook, because in order to create a more complete profile, we should be able to add more accounts *before* creating the contributor on chain. This is important, because the bot shouldn't have permission to update hashes afterwards, only to create new contributor records. Splitting that out can be done in a future PR, of course, but maybe it makes sense to at least split out the functionality from the route already.
package.json
@@ -16,0 +16,4 @@
"express": "^4.17.1",
"express-session": "^1.16.2",
"grant-express": "^4.6.1",
"group-array": "^1.0.0",
raucao (Migrated from github.com) commented 2019-07-26 10:31:25 +00:00
Copy Link
Copy Source

Why do we have to import express, when hubot comes with express by default?

Why do we have to import express, when hubot comes with express by default?
galfert (Migrated from github.com) reviewed 2019-07-26 17:16:06 +00:00
integrations/github.js
galfert (Migrated from github.com) commented 2019-07-26 17:16:06 +00:00
Copy Link
Copy Source

I used status(200) without the JSON, but that didn't work. I'll give sendStatus(200) a try.

I used `status(200)` without the JSON, but that didn't work. I'll give `sendStatus(200)` a try.
galfert (Migrated from github.com) reviewed 2019-07-26 17:25:36 +00:00
package.json
@@ -16,0 +16,4 @@
"express": "^4.17.1",
"express-session": "^1.16.2",
"grant-express": "^4.6.1",
"group-array": "^1.0.0",
galfert (Migrated from github.com) commented 2019-07-26 17:25:36 +00:00
Copy Link
Copy Source

Whoops, that's a leftover of something I tried to fix a problem.

Whoops, that's a leftover of something I tried to fix a problem.
raucao (Migrated from github.com) reviewed 2019-07-26 17:32:02 +00:00
integrations/github.js
raucao (Migrated from github.com) commented 2019-07-26 17:32:02 +00:00
Copy Link
Copy Source

Yeah, status only sets the status, but doesn't send a response by itself.

Yeah, `status` only sets the status, but doesn't send a response by itself.
galfert (Migrated from github.com) reviewed 2019-07-26 18:02:52 +00:00
package.json
@@ -16,0 +16,4 @@
"express": "^4.17.1",
"express-session": "^1.16.2",
"grant-express": "^4.6.1",
"group-array": "^1.0.0",
galfert (Migrated from github.com) commented 2019-07-26 18:02:52 +00:00
Copy Link
Copy Source

Correction: when removing that line, I actually get an error when starting HAL8000

Error: Cannot find module 'express'

Correction: when removing that line, I actually get an error when starting HAL8000 > Error: Cannot find module 'express'
galfert (Migrated from github.com) reviewed 2019-07-26 18:56:23 +00:00
integrations/github.js
galfert (Migrated from github.com) commented 2019-07-26 18:56:23 +00:00
Copy Link
Copy Source

Found a better solution using a middleware. With that it was easy to just add the CORS headers to the routes that need it.

Found a better solution using a middleware. With that it was easy to just add the CORS headers to the routes that need it.
galfert (Migrated from github.com) reviewed 2019-07-26 18:57:40 +00:00
integrations/github.js
galfert (Migrated from github.com) commented 2019-07-26 18:57:40 +00:00
Copy Link
Copy Source

Done

Done
galfert (Migrated from github.com) reviewed 2019-07-26 19:03:22 +00:00
integrations/github.js
@@ -175,0 +246,4 @@
contributorAttr.github_username = user.login;
contributorAttr.github_uid = user.id;
kredits.Contributor.add(contributorAttr, { gasLimit: 350000 })
galfert (Migrated from github.com) commented 2019-07-26 19:03:22 +00:00
Copy Link
Copy Source

I changed the log message. But apart from adding the CORS headers I didn't work on the signup route itself. So I don't know exactly how or what to split out. Maybe @bumi can jump in here?

I changed the log message. But apart from adding the CORS headers I didn't work on the signup route itself. So I don't know exactly how or what to split out. Maybe @bumi can jump in here?
galfert commented 2019-08-05 09:42:11 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Did anybody try this out yet?

Did anybody try this out yet?
gregkare commented 2019-08-08 10:31:03 +00:00 (Migrated from github.com)
Copy Link
Copy Source

I have just successfully tried this, until the screen that asks for an ETH address, I stopped there because my local kredits-web couldn't be configured to use my local chain (https://github.com/67P/kredits-web#3-kredits-web)

I had to generate a wallet and copy it to my test hubot's root and set addresses: { Kernel: '0x... } in index.js for hubot to start

I have just successfully tried this, until the screen that asks for an ETH address, I stopped there because my local kredits-web couldn't be configured to use my local chain (https://github.com/67P/kredits-web#3-kredits-web) I had to generate a wallet and copy it to my test hubot's root and set `addresses: { Kernel: '0x... }` in index.js for hubot to start
raucao commented 2019-08-08 12:32:39 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Those docs are wrong or outdated. npm run start:local always uses the local contracts, but you need to always set KREDITS_KERNEL_ADDRESS to the DAO address that you can show in kredits-contracts by running npm run dao:address.

Those docs are wrong or outdated. `npm run start:local` always uses the local contracts, but you need to *always* set `KREDITS_KERNEL_ADDRESS` to the DAO address that you can show in `kredits-contracts` by running `npm run dao:address`.
raucao commented 2019-08-08 12:34:02 +00:00 (Migrated from github.com)
Copy Link
Copy Source

I had to generate a wallet and copy it to my test hubot's root and set addresses: { Kernel: '0x... } in index.js for hubot to start

Yes, hubot-kredits needs a wallet that is allowed to create contributors in your local contracts. That part is missing entirely from the instructions here, unless I missed it.

Also, I think we still have to hardcode the local DAO address in its code, unless I missed that as well.

> I had to generate a wallet and copy it to my test hubot's root and set `addresses: { Kernel: '0x... }` in index.js for hubot to start Yes, hubot-kredits needs a wallet that is allowed to create contributors in your local contracts. That part is missing entirely from the instructions here, unless I missed it. Also, I think we still have to hardcode the local DAO address in its code, unless I missed that as well.
raucao commented 2019-08-08 12:36:07 +00:00 (Migrated from github.com)
Copy Link
Copy Source

I haven't tested the actual creation of the contributor yet (i.e. this part), because I don't have a full local Kredits contracts setup yet.

It would be extremely useful if the authors of this PR had actually tested it from end to end themselves.

> I haven't tested the actual creation of the contributor yet (i.e. [this part](https://github.com/67P/hubot-kredits/pull/41/files#diff-562725e9559c31e5fbdad85240ac316dR250-R258)), because I don't have a full local Kredits contracts setup yet. It would be extremely useful if the authors of this PR had actually tested it from end to end themselves.
raucao commented 2019-08-27 10:40:18 +00:00 (Migrated from github.com)
Copy Link
Copy Source

I merged support for setting the local DAO address via the KREDITS_DAO_ADDRESS env var into this branch, making it much easier to test.

I merged support for setting the local DAO address via the `KREDITS_DAO_ADDRESS` env var into this branch, making it much easier to test.
raucao (Migrated from github.com) reviewed 2019-08-27 10:48:47 +00:00
galfert commented 2019-08-27 11:21:36 +00:00 (Migrated from github.com)
Copy Link
Copy Source

I had to add the gasLimit option to the Contributor.add method, similar to what kredits-web is doing. Otherwise I would get this error when trying to create a new contributor:

{ Error: VM Exception while processing transaction: revert
    at getResult (/Users/galfert/code/active/kosmos/hubot-kredits/node_modules/ethers/providers/json-rpc-provider.js:40:21)
    at exports.XMLHttpRequest.request.onreadystatechange (/Users/galfert/code/active/kosmos/hubot-kredits/node_modules/ethers/utils/web.js:111:30)
    at exports.XMLHttpRequest.dispatchEvent (/Users/galfert/code/active/kosmos/hubot-kredits/node_modules/xmlhttprequest/lib/XMLHttpRequest.js:591:25)
    at setState (/Users/galfert/code/active/kosmos/hubot-kredits/node_modules/xmlhttprequest/lib/XMLHttpRequest.js:610:14)
    at IncomingMessage.<anonymous> (/Users/galfert/code/active/kosmos/hubot-kredits/node_modules/xmlhttprequest/lib/XMLHttpRequest.js:447:13)
    at IncomingMessage.emit (events.js:187:15)
    at endReadableNT (_stream_readable.js:1094:12)
    at process._tickCallback (internal/process/next_tick.js:63:19)

With the gasLimit option it works now.

I had to add the `gasLimit` option to the `Contributor.add` method, similar to what kredits-web is doing. Otherwise I would get this error when trying to create a new contributor: ``` { Error: VM Exception while processing transaction: revert at getResult (/Users/galfert/code/active/kosmos/hubot-kredits/node_modules/ethers/providers/json-rpc-provider.js:40:21) at exports.XMLHttpRequest.request.onreadystatechange (/Users/galfert/code/active/kosmos/hubot-kredits/node_modules/ethers/utils/web.js:111:30) at exports.XMLHttpRequest.dispatchEvent (/Users/galfert/code/active/kosmos/hubot-kredits/node_modules/xmlhttprequest/lib/XMLHttpRequest.js:591:25) at setState (/Users/galfert/code/active/kosmos/hubot-kredits/node_modules/xmlhttprequest/lib/XMLHttpRequest.js:610:14) at IncomingMessage.<anonymous> (/Users/galfert/code/active/kosmos/hubot-kredits/node_modules/xmlhttprequest/lib/XMLHttpRequest.js:447:13) at IncomingMessage.emit (events.js:187:15) at endReadableNT (_stream_readable.js:1094:12) at process._tickCallback (internal/process/next_tick.js:63:19) ``` With the `gasLimit` option it works now.
🎉 1
raucao commented 2019-08-27 13:36:55 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Unfortunately, I get the following error from the callback loading:

hal7000> (node:7741) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'access_token' of undefined
Unfortunately, I get the following error from the callback loading: ``` hal7000> (node:7741) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'access_token' of undefined ```
raucao commented 2019-08-28 13:25:25 +00:00 (Migrated from github.com)
Copy Link
Copy Source

@galfert @bumi Sure this shouldn't be a kredits-3 by now?

(Btw, I don't want kredits for my commits. Was just drive-by fixing stuff in my review and mostly doing the Web part of this.)

@galfert @bumi Sure this shouldn't be a `kredits-3` by now? (Btw, I don't want kredits for my commits. Was just drive-by fixing stuff in my review and mostly doing the Web part of this.)
raucao commented 2019-08-28 13:26:08 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Oh, and I think this is good to merge, right?

Oh, and I think this is good to merge, right?
galfert commented 2019-08-28 13:42:38 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Yes, you're right. I changed the label.

I think we are good to go.

Yes, you're right. I changed the label. I think we are good to go.
bumi commented 2019-08-28 13:53:48 +00:00 (Migrated from github.com)
Copy Link
Copy Source

need to add more documentation, then we merge.

need to add more documentation, then we merge.
raucao commented 2019-08-28 13:56:10 +00:00 (Migrated from github.com)
Copy Link
Copy Source

need to add more documentation, then we merge.

I added an issue for it. We can wait with tagging the release until the new feature is documented properly.

> need to add more documentation, then we merge. I added an issue for it. We can wait with tagging the release until the new feature is documented properly.
raucao added the kredits-3 label 2025-01-23 21:25:06 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
bumi
raucao
galfert
Labels
Clear labels
bug
Something is not working
 dev environment
Config, builds, CI, deployment, etc.
 docs
Documentation
 duplicate
This issue or pull request already exists
 enhancement
Improving existing functionality
 feature
New functionality
 idea
Something to consider
 invalid
Not a bug
 kredits-1
Small contribution
 kredits-2
Medium contribution
 kredits-3
Large contribution
 question
Looking for an answer
release
major
release
minor
release
patch
security
All your base are belong to us
 ui/ux
User interface, process design, etc.
 wontfix
This won't be fixed
No labels kredits-3
Milestone
No items
No Milestone
Assignees
Clear assignees
bkero (bkero) bumi (bumi) fsmanuel (fsmanuel) galfert (galfert) greg (Greg) haythem (haythem) hueso (hueso) raucao (Râu Cao) slvrbckt (Nick Jennings)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kredits/hubot-kredits#41