Using user credentials for review diffs?

[Brian DeGeeter]

Is there anything on the reviewboard roadmap to support passing
through user credentials to determine if source code can be displayed?

We're interested in using reviewboard in our development organization,
but it's currently viewed as a security concern because it cannot
restrict access to viewing source code on a per user basis.

We're using perforce. I'm not sure how access restrictions would vary
for the scmtools support, I wouldn't mind taking a stab at trying to
implement this.

cheers,

-Brian

[Christian Hammond]

Is there a reason you don't just restrict who can have a Review Board account?

Christian


--
Christian Hammond - chi...@chipx86.com
VMware, Inc.

[Brian DeGeeter]

We want to authenticate to an ldap server. The ldap server does not
know who has a perforce account. Within perforce authentication users
are restricted to certain depots. The single account on the
reviewboard server approach would allow users to read code which their
perforce account should restrict.

We'd like user to be able to just use reviewboard for reviews within
the depots they have access to and see a friendly "access restricted"
message when trying to view a review diff from an area they do not
have read access.

-Brian

On Jun 27, 3:05 pm, "Christian Hammond" <chip...@chipx86.com> wrote:
> Is there a reason you don't just restrict who can have a Review Board
> account?
>
> Christian
>
> --

> Christian Hammond - chip...@chipx86.com
> VMware, Inc.

[Christian Hammond]

The desired setup is going to vary depending on a company's install, and this is going to take a lot of work to get right. I'm unsure how best to implement this, but I imagine what we'll want is:

* A flag on a Repository indicating whether it's "public" (anyone can post/view reviews on it).
* A list of users on a Repository who have access to reviews on that repository. (Used only when the public flag is not set).

We wouldn't be able to use the Django permissions because those are more for "Joe can view entries on this model" rather than "this particular instance of the model."

We'd then need some way of automatically adding people to the list based on..... something. That I don't know. Whatever mechanism we chose would need to be generic enough to allow for people to use it with different setups.

Christian

--
Christian Hammond - chi...@chipx86.com
VMware, Inc.

[Brian DeGeeter]

The approach I've been playing around with goes something like this.
(It's very perforce specific)

Extend the authentication to get a perforce ticket for the user when
they log in. Add it to the user's account as some sort of scmtool
authentication token. This will allow p4 commands to be issued
without the users password.

The scm perforce methods would have to be tweaked to try an perforce
command with the user's account instead of the general repository
account. (Maybe the flag on the repository could be used for this?)

Would it be pretty straightforward to use the user's account name
instead of the repository account?

Then tweak the exception handling to display nice messages when a diff
can't be applied to the full code from source control.

I'm just not sure how scm neutral that approach would be. I'm not as
familiar with this sort of access control in CVS or SVN.

-Brian

On Jun 27, 4:19 pm, "Christian Hammond" <chip...@chipx86.com> wrote:
> The desired setup is going to vary depending on a company's install, and
> this is going to take a lot of work to get right. I'm unsure how best to
> implement this, but I imagine what we'll want is:
>
> * A flag on a Repository indicating whether it's "public" (anyone can
> post/view reviews on it).
> * A list of users on a Repository who have access to reviews on that
> repository. (Used only when the public flag is not set).
>
> We wouldn't be able to use the Django permissions because those are more for
> "Joe can view entries on this model" rather than "this particular instance
> of the model."
>
> We'd then need some way of automatically adding people to the list based
> on..... something. That I don't know. Whatever mechanism we chose would need
> to be generic enough to allow for people to use it with different setups.
>
> Christian
>
> --

> Christian Hammond - chip...@chipx86.com
> VMware, Inc.
>

[David Trowbridge]

Using it for all SCM commands wouldn't necessarily be good. Maybe just have an
authentication check for the files in the diff before showing the
review request.

We cache everything for the diffs pretty heavily, so if things are in the cache,
RB shouldn't hit the perforce server at all when showing the reviews.

-David

[Brian DeGeeter]

That's a good point. I'll keep it in mind.

So I spent a bit of time looking for similar enhancement requests in
the wiki. When I posted this topic I was hoping for "Yes, we've been
working on this just wait a week and grab the latest from svn". :)

Sounds like this wouldn't be too hard to implement, I just need to
make a case for doing the work ourselves.

Thanks for the feedback!

-Brian

On Jun 27, 5:20 pm, "David Trowbridge" <trowb...@gmail.com> wrote:
> Using it for all SCM commands wouldn't necessarily be good.  Maybe just have an
> authentication check for the files in the diff before showing the
> review request.
>
> We cache everything for the diffs pretty heavily, so if things are in the cache,
> RB shouldn't hit the perforce server at all when showing the reviews.
>
> -David
>