drone / drone-wall

Dashboard for the Drone CI server
260 stars 45 forks source link

Doesn't work with Drone 0.4 API #30

Closed johnrengelman closed 8 years ago

johnrengelman commented 8 years ago

This no longer functions with the Drone 0.4 API.

juanluisbaptiste commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

Jean85 commented 8 years ago

Any news? :cry:

scottferg commented 8 years ago

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 commented 8 years ago

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

bradrydzewski commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

@bradrydzewski SGTM

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

Tathanen commented 8 years ago

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

Tathanen commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

yep, I think we were missing the CORS header on option requests. I just pushed the following change: https://github.com/drone/drone/commit/e018a347118118f267755b872d671f204ee7170a

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

Tathanen commented 8 years ago

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

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 commented 8 years ago

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 commented 8 years ago

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

Tathanen commented 8 years ago

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 commented 8 years ago

for now I pushed https://github.com/drone/drone/commit/398db0bda2a46456994c93af8040fdcd37bee1be 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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

Oh, lucky! :+1:

Tathanen commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

... 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 commented 8 years ago

... 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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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:

Tathanen commented 8 years ago

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 commented 8 years ago

PR finally ready: https://github.com/drone/drone-wall/pull/31

Tathanen commented 8 years ago

Resolved by #31