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

Doesn't work with Drone 0.4 API #30

Closed
johnrengelman opened this issue Dec 29, 2015 · 50 comments
Closed

Doesn't work with Drone 0.4 API #30

johnrengelman opened this issue Dec 29, 2015 · 50 comments

Comments

@johnrengelman
Copy link

This no longer functions with the Drone 0.4 API.

@juanluisbaptiste
Copy link

Yes, I just tested it too and the server crashes after some minutes. On the console I get this error:

drone-wall-1 | 2015-12-30T07:22:55.977253338Z undefined:1 drone-wall-1 | 2015-12-30T07:22:55.977705934Z 404 page not found drone-wall-1 | 2015-12-30T07:22:55.977889451Z ^ drone-wall-1 | 2015-12-30T07:22:55.998233127Z SyntaxError: Unexpected token p drone-wall-1 | 2015-12-30T07:22:55.998285880Z at Object.parse (native) drone-wall-1 | 2015-12-30T07:22:55.998295824Z at IncomingMessage.<anonymous> (/webapp_root/server.js:45:29) drone-wall-1 | 2015-12-30T07:22:55.998306819Z at IncomingMessage.emit (events.js:117:20) drone-wall-1 | 2015-12-30T07:22:55.998315598Z at _stream_readable.js:943:16 drone-wall-1 | 2015-12-30T07:22:55.998324028Z at process._tickCallback (node.js:419:13

@scottferg
Copy link
Contributor

Yup this was expected. Thanks for reporting. We're in the process of migrating to Drone v0.4 internally and will get this patched up once we have an instance to test against. I'm hoping it'll be ready to go within the next few weeks.

@johnrengelman
Copy link
Author

Sounds good. It was more for in case anyone else came looking for answers.

@Jean85
Copy link

Jean85 commented Jan 21, 2016

Any news? 😢

@scottferg
Copy link
Contributor

We're rolling out v4 internally right now. Once we've switched over completely (next week or two) updates to the wall are planned.

@scottferg
Copy link
Contributor

FYI we're working on getting this rolled out this week. I'll be opening a PR on drone/drone soon.

@bradrydzewski
Copy link
Member

@scottferg while you are digging into the code ... there should be changes in place allowing cors (if not I can quickly adjust to enable). It would be awesome if we could make this client only (no backend) and store the url and token in local storage ... I would be happy to host the static site somewhere for people to use ... perhaps at wall.drone.io

@Jean85
Copy link

Jean85 commented Feb 5, 2016

Great @scottferg ! In the meantime a collegue of mine ( @taueres ) forked this repo and made it work with some patches.. We will follow this closely ;)

@scottferg
Copy link
Contributor

@bradrydzewski SGTM

cc: @Tathanen since he's doing most of the legwork on this end

@Tathanen
Copy link
Contributor

Tathanen commented Feb 5, 2016

👍 Yep, the new version is client-only, you'll just pass in the path to your API when you spin it up.

@Tathanen
Copy link
Contributor

Tathanen commented Feb 5, 2016

Question for you tho @bradrydzewski about how the API authorization works. Right now I can get it to respond with JSON in the browser if I've already github-authenticated with drone proper, it registers a cookie on the domain (user_sess) and that gets sent along with the API requests automatically. If I want to reference the API routes from a web client at a different domain, though, what's my avenue for passing along an auth token? Does it accept a straight github oauth token? Should I require a wall-user to oauth separately? I haven't been able to find any auth talk in the API documentation.

@bradrydzewski
Copy link
Member

Yep, the new version is client-only, you'll just pass in the path to your API when you spin it up.

Awesome!

if I want to reference the API routes from a web client at a different domain, though, what's my avenue for passing along an auth token? Does it accept a straight github oauth token? Should I require a wall-user to oauth separately? I haven't been able to find any auth talk in the API documentation.

You can append ?access_token={token} or use an Authorization: Bearer {token} where the token is retrieved from your user profile screen in drone. You can use this library for reference https://github.com/drone/drone-node

So the wall should be able to access the Drone API endpoints without being authenticated (via GitHub oauth) to Drone proper.

@Tathanen
Copy link
Contributor

Tathanen commented Feb 5, 2016

Great! I was able to get the API responding successfully using that token via the Authorization header in postman, but I'm indeed getting CORS errors when trying it from the static app. So if you could get the Access-Control-Allow-Origin header configured that'd be suuuuper.

@bradrydzewski
Copy link
Member

yep, I think we were missing the CORS header on option requests. I just pushed the following change:
harness/harness@e018a34

a new Docker image for Drone should be available in ~5 minutes

@Tathanen
Copy link
Contributor

Tathanen commented Feb 5, 2016

Okay we've got the new image installed, but now I'm gettin these in Firefox/Chrome:

  • Reason: missing token 'content-type' in CORS header 'Access-Control-Allow-Headers' from CORS preflight channel
  • Request header field Content-Type is not allowed by Access-Control-Allow-Headers in preflight response.

I'm just using Angular's $http service, it looks like I have this in my request headers:

access-control-request-headers:accept, authorization, content-type

Maybe you need to allow content-type and accept alongside the already-existing authorization?

@bradrydzewski
Copy link
Member

what if I remove that line all together?
https://github.com/drone/drone/blob/master/router/middleware/header/header.go#L30

does cors require the access-control-request-headers?

@bradrydzewski
Copy link
Member

or perhaps the accepted answer in this stackoverflow? http://stackoverflow.com/questions/13146892/cors-access-control-allow-headers-wildcard-being-ignored

@Tathanen
Copy link
Contributor

Tathanen commented Feb 5, 2016

That stackoverflow answer may do it, as long as there aren't any headers you'd be afraid to allow. I... can't imagine there are any.

@bradrydzewski
Copy link
Member

for now I pushed harness/harness@398db0b which adds the recommended headers: accept, content-type, origin and authorization. If that doesn't work we can follow-up with a more comprehensive approach

new image should be ready in ~5 min

@Tathanen
Copy link
Contributor

Tathanen commented Feb 5, 2016

Great! I'm having trouble sending the Authorization header via the client for some reason (the problem there may be on my end), but I've got it working successfully if I send the token as part of the URL.

@bradrydzewski
Copy link
Member

@Tathanen no worries, the query parameter should work just fine as well. For reference you can see how it is done with the header in this node lib https://github.com/drone/drone-node/blob/master/lib/client.js#L20

@Tathanen
Copy link
Contributor

Hey @bradrydzewski, I've got most things working now, but I'm caught up at the moment because the route I'm using (user/feed) is missing a timestamp field that I used to access on the older drone wall implementation, updated_at. It was basically the last time something pertaining to the build changed, most notably the status flag. Every time I hit the route, I used to compare that timestamp to the time I last hit the route, and only process records that had been updated.

Without it, I basically have to maintain a persistent recorded state of every build I've processed to avoid re-creating builds and PRs that have completed or closed, which is a lot of overhead. Do you suppose updated_at is something you could bring back?

@bradrydzewski
Copy link
Member

is it possible to use a combination of the started_at and finished_at fields? When the build is pending, started_at == 0, when the build starts finished_at == 0 and when the build finishes, the finished_at field is populated. Not sure how the internals work, but wondering if that logic helps?

@taueres
Copy link

taueres commented Feb 12, 2016

@Tathanen @bradrydzewski This: https://github.com/taueres/drone-wall/blob/master/server/api.js#L11 is how I computed updated_at for mapping the newest API to the older one.

@Tathanen
Copy link
Contributor

@taueres that's a pretty good idea actually, I think I'll start with that. @bradrydzewski I may tweak the methodology a bit knowing the exact mapping for each state that you've got there, between those two bits I may be able to get it working. I'll let you know!

@Tathanen
Copy link
Contributor

Alright it's lookin pretty good on my end, we're gonna trial it in-office for the week to make sure it's all workin the way it should, then I'll open a PR on this repo.

@Tathanen
Copy link
Contributor

Hey @bradrydzewski, I was afraid something like this might happen with how your new API is working, but after a while with the wall running, I'm getting a 400 response and this:

GET https://api.github.com/user/orgs?page=1: 403 API rate limit exceeded for Tathanen. []

The wall tries to hit the API every 5 seconds to update the state of any running builds, pull in new ones, etc. Realistically it runs every 10 seconds because each call is taking around 6 seconds or so, and I wait for each request to complete before starting a new one.

Not really sure what the workaround for this is, since my understanding is that you're hitting the GitHub API every time I hit the Drone API now. It'd need to cache all the repo info the first time and only respond with build data on subsequent requests, cutting out the need to hit GitHub as often. That sounds like a pretty fundamental change, though.

I feel like some combination of websockets that report new builds with GitHub data, and updated builds with only build data, would be ideal.

@bradrydzewski
Copy link
Member

the repository list is cached, however, it looks like the /api/user/feed endpoint is not correctly using this cache resulting un-necessary calls to github. No workaround or fundamental changes needed, just need to hook up the cache

@Tathanen
Copy link
Contributor

Oh, lucky! 👍

@Tathanen
Copy link
Contributor

Okay! Last issue here, @bradrydzewski.

In many cases, we want the drone wall to not be publicly accessible. So I'm looking at my options for throwing a login gate on it, be it github oauth, google oauth, something like that. The problem is associating one of those options with the owner of the token that's driving the wall. The closest I can get is a github oauth that then uses the drone token to check the user info of the drone user, and compares the two usernames. But then only the user whose token was hard-coded into the wall can ever access it, which is quite inconvenient when a whole team wants to access it.

The ideal scenario here would be if the drone wall could just read the drone user_sess cookie. There'd be no need to include a token in the build, and no need for a login prompt. If you're already authorized as a user with access to these repos via the actual drone site, the wall will Just Work.

@scottferg mentioned that wall.drone.io was potentially an option, and when hosting a local drone instance at something like drone.whatever.com, we'd also host the wall at something like wall.whatever.com. So the change on your end would be to make the user_sess cookie not subdomain-limited. My only concern there is that user_sess is a pretty generic name that might be used by other sites on the domain for other purposes, so maybe it'd make sense to rename it to something like drone_user_sess.

We've done this before for other projects, sharing a cookie between subdomains to maintain a single user session, and it's been pretty effective. Thoughts?

@Jean85
Copy link

Jean85 commented Feb 25, 2016

I think this approach is nice, but could be problematic. The wall may not be hostable on the same subdomain level; for example, I could host the wall locally, on a Intel NUC hooked to a TV, and have Drone\GitLab hosted somewhere else, or be public.

Maybe it will be enough to provide also a fallback method, something very simple as a basic HTTP auth with fixed user\password, just to cover those "strange" setups..

@Tathanen
Copy link
Contributor

Yeah, my thinking is that if the user-specific setup can't make the cookie sharing work, I'd still support the ability to just pass the token in during the build step. It wouldn't be secured then, but I imagine on local in-house setups, the login prompt isn't super necessary. It's just to protect the wall if it's hosted on a public domain.

@bradrydzewski
Copy link
Member

@Tathanen I was thinking that you would provide the token when you visit the site, and it would be stored in local storage in the browser. This is based on the assumption that the wall is hooked up to a TV which means you would only need to do this step once.

@Tathanen
Copy link
Contributor

@bradrydzewski That's definitely an option, but at least in my experience a lot of people in our office like to pull the wall up on their machines here and there to check on build statuses. I've actually got the wall working a bit more responsively this time around so you can see the build status portion nicely on a phone. Requiring a manual token entry for each of these cases feels like a bit much, being able to read it from an existing cookie streamlines the process considerably.

@bradrydzewski
Copy link
Member

@Tathanen we have an endpoint in drone that let's you swap a GitHub token for a drone token (see https://github.com/drone/drone/blob/master/controller/login.go#L118). What if you give the ability to paste your drone token, or login with GitHub? If you choose that latter, you can use the browser-based oauth2 flow to fetch the GitHub token, exchange for a Drone token, and then store that in localstorage as if the user had pasted it themselves.

@Tathanen
Copy link
Contributor

@bradrydzewski Aha! That's the kind of thing I'm lookin for. It's the solution I was trying to solve for initially, using the GitHub oauth, but I didn't know this endpoint existed. So it's a post to /token it looks like, what does the post body need to be? Or does it just need to post with the github token in the Authorization header?

@bradrydzewski
Copy link
Member

@Tathanen it is a POST to /api/user/token where the payload is { "access_token": "" }

I'm not sure this approach would be any better, though. You would need to expose your github client id and secret in your client side javascript, which puts us back to square one.

@Tathanen
Copy link
Contributor

@bradrydzewski hm yeah, the client ID probably isn't a big deal but the secret, don't wanna put that in there. Kinda brings us back to the shared cookie as the cleanest solution. Do you have any concerns with that approach?

@bradrydzewski
Copy link
Member

the problem with the cookie is that the drone cookie expires, which means it will get logged out every 72 hours. This could be problematic for some teams that have the TVs + small computers wall mounted.

@Tathanen
Copy link
Contributor

Hm, yeah we'd need a token exchange route the wall could hit every day or so to get a fresh one.

That said, I've been using my drone token for wall testing for weeks now without it expiring. Not sure what that's about.

@Tathanen
Copy link
Contributor

Oh wait, you said cookie not token. Duh.

Would it be possible to create a second cookie for a defined "wall." subdomain when users authenticate that doesn't expire?

@Tathanen
Copy link
Contributor

... or couldn't I just read the cookie that first time, then store the value in localstorage? I don't need the cookie past that initial authorization really.

@bradrydzewski
Copy link
Member

... or couldn't I just read the cookie that first time, then store the value in localstorage? I don't need the cookie past that initial authorization really.

@Tathanen this wouldn't work because we use CSRF tokens within drone when using cookie-based authentication. This will prevent something like the wall from attempting to create a token, as it would fail CSRF verification.

@Tathanen
Copy link
Contributor

Well the wall doesn't need to create a token, it would just use the token value in the cookie wholesale. If there's no token present in a user_sess cookie it would tell the user to authenticate with drone first, then come back to the wall. The wall would read that cookie and use the token in it to hit the drone API. I just tried using the value in my user_sess cookie in postman and in my local running wall instance, and it worked without issue.

@bradrydzewski
Copy link
Member

sorry if I'm misunderstanding, but the token in user_sess expires every 72 hours and this would require people to re-login. If the wall is installed on a TV and computer is mounted or in a rack in a closet this might be considered onerous.

would it be possible for the default wall configuration to prompt the user for the drone url and token, so that the wall can be client side only (no server-side requirements) with a fallback to read the user_token if present for individuals visiting from their desktop or mobile device?

@Tathanen
Copy link
Contributor

Sorry, when you said the cookie expires I thought you were referring to the cookie lifespan itself, not the token inside.

Could you describe your fallback scenario there a bit more? How is the wall accessing the user_token?

@bradrydzewski
Copy link
Member

@Tathanen yes, the cookie containers a JWT token that has an expiration date of 72 hours encoded into the token itself.

How is the wall accessing the user_token?

The wall is not going to have access to user_token unless it is running on the same domain or subdomain as drone, which means you will probably need to put a reverse proxy in front of drone and drone wall and use subpaths to direct traffic appropriately.

Could you describe your fallback scenario there a bit more?

Here are some options off the top of my head:

  • drone wall ships with optional server backend that enables some sort of oauth process to get a drone token, which it passes to the client. The user can choose to manually enter the drone url and token, or use oauth to get the token.
  • drone wall is deployed on the same domain with a reverse proxy and makes an api call to /api/user to see if the user is authenticated. If not, it prompts the user to enter the drone url and token. The user can do this, or login to drone and go back to the wall and refresh the page.
  • drone wall prompts user to enter the drone url and token. The user goes to drone, gets the token, and copy / pastes into the wall. This will be a one-time setup from the desktop and avoids some of the design complexities with the other options

@Tathanen
Copy link
Contributor

I'd reeeally like to keep this entirely client-side, which leaves us with the third option there. It's basically what I'm doing already, just without a dedicated user input for the fields. I think it's kind of a pain, but at least it's a one-time process.

I'll still allow the token to just be built in from the start for usecases where it'll only be accessed via an internal address, and access-control isn't a concern.

@Tathanen
Copy link
Contributor

Tathanen commented Mar 2, 2016

PR finally ready: #31

@Tathanen
Copy link
Contributor

Tathanen commented Mar 5, 2016

Resolved by #31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants