V1.4.1 #161

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
2 participants
@manishagele
Contributor

manishagele commented Mar 21, 2014

Please commit this to the main branch. This has the modified RPCUtils implementation for gitblit along with the test.

@gitblit

This comment has been minimized.

Show comment
Hide comment
@gitblit

gitblit Mar 21, 2014

Owner

Hi Manisha,

Thanks for proposing this pull request. I'll wait for you to add the remainder of the code (e.g. server changes) and to finish the unit tests before I review.

Thanks,
James

Owner

gitblit commented Mar 21, 2014

Hi Manisha,

Thanks for proposing this pull request. I'll wait for you to add the remainder of the code (e.g. server changes) and to finish the unit tests before I review.

Thanks,
James

@manishagele

This comment has been minimized.

Show comment
Hide comment
@manishagele

manishagele Mar 25, 2014

Contributor

Hi James,

Thanks for pointing out. I have missed adding the server changes to the PR. Added it now.
Also I think I have put everything related to the unit test also.

Please let me know, if anything is needed.
Thanks,
Manisha

Contributor

manishagele commented Mar 25, 2014

Hi James,

Thanks for pointing out. I have missed adding the server changes to the PR. Added it now.
Also I think I have put everything related to the unit test also.

Please let me know, if anything is needed.
Thanks,
Manisha

@gitblit

This comment has been minimized.

Show comment
Hide comment
@gitblit

gitblit Mar 25, 2014

Owner

I still don't like the composite class. I think sending the repository name as a the name parameter would be sufficient. That leaves the UserModel. The way you've designed this, the fork RPC can only be executed by an administrator and the administrator must supply the target user. This feels backwards. I expect that users will want to create their own fork and not have to rely on an administrator to do this for them - that fits in with the permissions model too. Unit tests. I think these must have missed the "git add" step.

Owner

gitblit commented Mar 25, 2014

I still don't like the composite class. I think sending the repository name as a the name parameter would be sufficient. That leaves the UserModel. The way you've designed this, the fork RPC can only be executed by an administrator and the administrator must supply the target user. This feels backwards. I expect that users will want to create their own fork and not have to rely on an administrator to do this for them - that fits in with the permissions model too. Unit tests. I think these must have missed the "git add" step.

@manishagele

This comment has been minimized.

Show comment
Hide comment
@manishagele

manishagele Apr 3, 2014

Contributor

Hi James,

Need a bit of a clarification here.
As per the backend implementation, the gitblitmanager.fork method needs both RepoModel and the UserModel as arguments. So in that case, don't we need to pass both UserModel and RepoModel?
I have tried this and even for the non admin users, could create forks.

Contributor

manishagele commented Apr 3, 2014

Hi James,

Need a bit of a clarification here.
As per the backend implementation, the gitblitmanager.fork method needs both RepoModel and the UserModel as arguments. So in that case, don't we need to pass both UserModel and RepoModel?
I have tried this and even for the non admin users, could create forks.

@gitblit

This comment has been minimized.

Show comment
Hide comment
@gitblit

gitblit Apr 3, 2014

Owner

The fork method UserModel argument should be the UserModel who authenticated for the request. I shouldn't be able to authenticate and create a fork for your account. As for non-admins successfully forking, I'll have to review the security more carefully.

Owner

gitblit commented Apr 3, 2014

The fork method UserModel argument should be the UserModel who authenticated for the request. I shouldn't be able to authenticate and create a fork for your account. As for non-admins successfully forking, I'll have to review the security more carefully.

@manishagele

This comment has been minimized.

Show comment
Hide comment
@manishagele

manishagele May 5, 2014

Contributor

Hi James,
When will this be merged to the trunk?
Has the security being reviewed?
Thanks
Manisha

Contributor

manishagele commented May 5, 2014

Hi James,
When will this be merged to the trunk?
Has the security being reviewed?
Thanks
Manisha

@gitblit

This comment has been minimized.

Show comment
Hide comment
@gitblit

gitblit May 5, 2014

Owner

I'll get some variation of this merged for the next major release. I'm looking at around June 1 for release - so, somewhere in the next 30 days.

Owner

gitblit commented May 5, 2014

I'll get some variation of this merged for the next major release. I'm looking at around June 1 for release - so, somewhere in the next 30 days.

@manishagele

This comment has been minimized.

Show comment
Hide comment
@manishagele

manishagele May 7, 2014

Contributor

Ok. Great.
So I guess what you meant by "a variation" is, a method call without UserModel argument?

Contributor

manishagele commented May 7, 2014

Ok. Great.
So I guess what you meant by "a variation" is, a method call without UserModel argument?

@gitblit

This comment has been minimized.

Show comment
Hide comment
@gitblit

gitblit May 7, 2014

Owner

Yes. And I still think there is an issue with non-admins being able to fork. They should be able to, but I haven't tested that with your code. There is a subtle implied permission in the ordering of the requests enum.

Owner

gitblit commented May 7, 2014

Yes. And I still think there is an issue with non-admins being able to fork. They should be able to, but I haven't tested that with your code. There is a subtle implied permission in the ordering of the requests enum.

@gitblit

This comment has been minimized.

Show comment
Hide comment
@gitblit

gitblit May 8, 2014

Owner

This has been revised and merged into develop.

Owner

gitblit commented May 8, 2014

This has been revised and merged into develop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment