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

V1.4.1 #161

Closed
wants to merge 3 commits into from
Closed

V1.4.1 #161

wants to merge 3 commits into from

Conversation

manishagele
Copy link
Contributor

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

@gitblit
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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

@gitblit
Copy link
Collaborator

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
Copy link
Contributor Author

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

@gitblit
Copy link
Collaborator

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
Copy link
Collaborator

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants