Closed (fixed)
Project:
Drupal.org CVS applications
Component:
co-maintainer application
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
19 May 2010 at 22:19 UTC
Updated:
6 Nov 2018 at 21:26 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
netw3rker commentedAttached is one the accessory modules I'll be contributing to the siteminder module called siteminder_roles. i do not want this to become its own project since it will just be part of the siteminder package.
Comment #2
avpadernoHello, and thanks for applying for a CVS account.
As you are applying to be a co-maintainer, you should have created a support request in the project queue, offering to become co-maintainer; only once the current maintainer accepted you as co-maintainer, you should have applied for a CVS account.
If you already created such support request, please report the link to the issue. In alternative, I am also happy if the current maintainer would add a comment here saying you are accepted as co-maintainer.
Comment #3
sun1) Should use http://api.drupal.org/api/constant/DRUPAL_AUTHENTICATED_RID/6 instead of integer 2.
2) The value of #value in "header_value" needs to be properly sanitized.
3) User role names used in textarea #title need to be properly sanitized.
4) There is no validation of submitted form values at all here. However, submitted form values are used as-is, without further sanitation in the run-time logic. Sanitation needs to happen in at least one of both places. Ideally validate on input, and additionally sanitize prior to usage.
Comment #4
avpadernoTherefore, this application is for being co-maintainer. In this case, we need more information (see comment #2).
Comment #5
netw3rker commented@sun Thanks for the comments! I've taken your notes into account in the modules code. I'm not sure what you expect next though. do I need to re zip and upload the code so you can verify this before granting me the go-ahead? I'm a little curious how this and future commits work now? Is it always such that modules that get submitted initially go through a review, then after the cvs account is granted people can commit whatever they want w/o review?
@kiamlaluno I asked the maintainer for siteminder to respond here or in the issue queue about being a co-maintainer, but he has fallen off the radar and isnt responding to anything anymore. That being said, i might as well create this as its own module until such time as he wants to incorporate this into his module.
@all if it helps the process, I'm also joining the LDAP project as a co-maintainer. see the post/request here: http://drupal.org/node/806068
Thanks!
-Chris
Comment #6
avpaderno@netw3rker: The status is because there is nothing to review for a co-maintainer application. You didn't report the link to the support request you opened for Siteminder.
Comment #7
netw3rker commentedKiamlaluno,
I'm not sure how to proceed here. I want to go forward with becoming a module maintainer for the module posted above as its own stand alone module. At this point adding it to the siteminder project Isn't going to happen and I'd rather maintain it separately. that means this is no longer a 'co-maintainer request'.
Instead of me guessing what you guys are expecting from me, and you guys guessing at what i'm trying to do. could you please outline what I need to do make the following things happen:
1) I'd like a cvs account
2) I want to maintain a project called siteminder_roles using that account
3) I would like to use my cvs account to co-maintain a couple of projects too (in particular the ldap module approved in the ldap modules call for co-maintainers here: http://drupal.org/node/806068 ) .
Please let me know what i need to do to make these possible.
-Chris
Comment #8
avpadernoIf this is no longer a co-maintainer request, then the code needs to be reviewed. I can do it, but after June 22.
Comment #9
avpadernoAs per previous comment (comment #3) by sun, I am changing the status to .
Comment #10
netw3rker commentedkiamlaluno,
There are 2 parts to this request. one is to become a module maintainer for the siteminder_roles module. the other is to become a co-maintainer for the LDAP module.
Since you wont be able to review the siteminder_roles module till the end of the month, can you grant me a cvs account so that i can be a co-maintainer for the LDAP module? Here is a link to my request to be a co-maintainer: http://drupal.org/node/806068#comment-3051642 and below that is them accepting it.
Thanks
-Chris
Comment #11
sunThe pointed to LDAP project looks and reads like a new joint effort of developers having the same intention, which is very good to see. However, it also means that co-maintenance has been accepted on a hypothetical basis, since the module is not yet in use.
Given that we already have a code review here, which turned out to list a couple of security issues in the proposed siteminder_roles module, it would be preferred to see a revised module as attachment here, in which at least all critical issues have been addressed.
Comment #12
netw3rker commentedHi Sun,
Thanks for the guidance. Attached is the module with the recommended changes made. in note #4 you recommended validating either on input or during runtime-use. Given that these fields are user entries for header values that have no defined naming convention and have values that only have internal purpose-specific conventions, I think it will be impossible to provide meaningful validation for these fields since they can literally be any value. I agree that ensuring that when they are presented to the users they don't have html rendering is a must, so I put in a few check_plain()'s to prevent issues.
let me know if anything else jumps out at you!
Thanks
-Chris
Comment #13
Amazon commentedHi, I am going to go ahead and approve Chris's CVS account. Chris is my colleague so I've asked him to post publicly.
But LDAP integration is critical for Drupal adoption in the enterprise, and since the maintainers have okayed him it doesn't seem to make much sense delaying that effort. If anyone objects to this, I'll gladly accept a reverse in this decision.
But we need more capable maintainers working on critical features that drive Drupal adoption. I trust the siteminder security issues will be resolved.
Kieran
Comment #14
Ian Ward commentedI am late to the thread; Chris (netw3rker) and I have been discussing the siteminder module and the siteminder roles addon module and I had accepted Chris's co-maintainership of the siteminder module. Chris asked me to confirm this on this thread initially but it fell of my radar and I caught it while reviewing my backlog. Even if siteminder_roles is its own module there are patches w/ Siteminder Chris and I discussed and which is why he needed his CVS account. Anyway, looks like he's got it now. Sorry if my late reply caused this thread to drag out.
Ian
Comment #16
avpaderno