If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Self register new user account API

RESOLVED DUPLICATE of bug 350232

Status

()

Bugzilla
User Accounts
--
enhancement
RESOLVED DUPLICATE of bug 350232
13 years ago
9 years ago

People

(Reporter: Jason Pyeron, Assigned: Jason Pyeron)

Tracking

Details

Attachments

(1 attachment, 1 obsolete attachment)

3.33 KB, patch
Erik Stambaugh
: review+
justdave
: review-
Max Kanat-Alexander
: review-
Details | Diff | Splinter Review
(Assignee)

Description

13 years ago
User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0; .NET CLR 1.0.3705; .NET CLR 1.1.4322)
Build Identifier: 

not all self registered accounts should be done via a HTML type interface, some 
accounts may be created on behalf of their users but via channels other than 
the web.

I.E. IPC, smtp, scripts, etc.

patch to follow

Reproducible: Always
Steps to Reproduce:
(Assignee)

Comment 1

13 years ago
Created attachment 159901 [details] [diff] [review]
SelfRegisterNewAccount API

also modify createaccount.cgi to use it
(Assignee)

Updated

13 years ago
Attachment #159901 - Flags: review?
(Assignee)

Updated

13 years ago
Flags: approval?
Flags: approval?
(Assignee)

Updated

13 years ago
Summary: API self register new user account → PATCH - self register new user account API
Erik, you've been messing with the auth/email stuff most lately, does this fit?

Updated

13 years ago
Summary: PATCH - self register new user account API → Self register new user account API

Comment 3

13 years ago
Comment on attachment 159901 [details] [diff] [review]
SelfRegisterNewAccount API

I've tried to give a high-level review on this, but since I'm not familiar with
this area of the code, I probably failed to do that and Erik will have to fill
the missing dots.

The most high-level reason for which this is not commitable in my opinion is
the performance issue. When refactoring we should try  to avoid regression
regarding CPU load or duplicate tasks. You are duplicating the check of a valid
email syntax:

     CheckEmailSyntax($login);
     $vars->{'login'} = $login;
-    
-    if (!ValidateNewUser($login)) {
+
+    # Create account
+    my ($password, $res)=SelfRegisterNewAccount($login,$realname);

Both CheckEmailSyntax and SelfRegisterNewAccount load
Param('createemailregexp'); and check the current email againest that.

Now, getting towards the low-level things (nits):

-> I don't have a solution for this, but I don't like using -1 and -2 as error
codes. Ideally those could be constants or something similar, if we really must
use this arhitecture.

-> I'd like the identation to be consistent with the way in which the code was
written. For example:

+    my ($login,$realname)=@_;

could be:

+    my ($login, $realname) = @_;

-> I don't like the fact that we must return undef values. For example we could
change the order of the returned params, so instead of having:

+    my $password;
+    if (!ValidateNewUser($login)) { return ($password,-1); }

we could simply do return -1, and in this way we could declare "my $password"
on the same line where we're doing its assignment.

-> Space after comma is inconsistent here too:

+    my ($password, $res)=SelfRegisterNewAccount($login,$realname);

Besides, I think we could use "$result" instead of "$res", it's just 3 chars
longer but it provides a higher level of clarity. Also, spaces should be added
around "==". The developer's guide contains more details about the identing.

-> spellchecker (rutines --> routines)

In the end, if this is what we want from an arhitectural point of view (others
will have to connect the dots where), this could be commitable, as long as we
stay away from code duplication and redundancy.
Attachment #159901 - Flags: review? → review-
(Assignee)

Comment 4

13 years ago
(In reply to comment #3)
> email syntax:
>      CheckEmailSyntax($login);
>      $vars->{'login'} = $login;
> -    
> -    if (!ValidateNewUser($login)) {
> +
> +    # Create account
> +    my ($password, $res)=SelfRegisterNewAccount($login,$realname);
> Both CheckEmailSyntax and SelfRegisterNewAccount load
> Param('createemailregexp'); and check the current email againest that.

sub CheckEmailSyntax {
    my ($addr) = (@_);
    my $match = Param('emailregexp');
    if ($addr !~ /$match/ || $addr =~ /[\\\(\)<>&,;:"\[\] \t\r\n]/) {
        ThrowUserError("illegal_email_address", { addr => $addr });
    }
}


vs 

    my $createexp = Param('createemailregexp');

(Assignee)

Comment 5

13 years ago
Created attachment 160209 [details] [diff] [review]
constants, whitespace, etc...

(In reply to comment #3)
> Now, getting towards the low-level things (nits):
> -> I don't have a solution for this, but I don't like using -1 and -2 as
error
> codes. Ideally those could be constants or something similar, if we really
must
> use this arhitecture.

done. 
use Bugzilla::Constants;

> -> I'd like the identation to be consistent with the way in which the code
was
> written. For example:
> +    my ($login,$realname)=@_;
> could be:
> +    my ($login, $realname) = @_;

done, I think. I am dyslexic, it is very difficult for me to type format.

> -> I don't like the fact that we must return undef values. For example we
could
> change the order of the returned params, so instead of having:
> +    my $password;
> +    if (!ValidateNewUser($login)) { return ($password,-1); }
> we could simply do return -1, and in this way we could declare "my $password"

> on the same line where we're doing its assignment.

done.

> -> Space after comma is inconsistent here too:
> +    my ($password, $res)=SelfRegisterNewAccount($login,$realname);
> Besides, I think we could use "$result" instead of "$res", it's just 3 chars
> longer but it provides a higher level of clarity. Also, spaces should be
added
> around "==". The developer's guide contains more details about the identing.

done.

> -> spellchecker (rutines --> routines)

done.
Attachment #159901 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #160209 - Flags: review?
Attachment #160209 - Flags: review? → review?(erik)

Comment 6

13 years ago
Comment on attachment 160209 [details] [diff] [review]
constants, whitespace, etc...

I definitely have a big interest in this patch.  I feel badly that most of the
good nits have already been taken, though.

I tested this out and it ran fine.  I have one tiny little inconsequential nit:


>Index: createaccount.cgi
[...]
>+    #if ($result != SelfRegisterNewAccount_SUCCESS) error...

I can't tell the purpose of this comment.  Was it a note in the code (like a
XXX comment), or earlier code that was commented out?


That's it.

r=erik, as long as we can make sense of that
Attachment #160209 - Flags: review?(erik) → review+
(Assignee)

Comment 7

13 years ago
it is a lazy error check or assertion, which I commented out BUT left in as 
a 'comment'

just checks that the result is not the OK result vs it is error A or error B
Flags: approval?

Updated

13 years ago
Assignee: myk → jpyeron
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 8

13 years ago
(In reply to comment #7)
> it is a lazy error check or assertion, which I commented out BUT left in as 
> a 'comment'
> 
> just checks that the result is not the OK result vs it is error A or error B

Hmm.  It should probably be pulled, then.

Comment 9

13 years ago
Comment on attachment 160209 [details] [diff] [review]
constants, whitespace, etc...

+    if (!ValidateNewUser($login)) 
+    {
+     return (SelfRegisterNewAccount_FAIL_DUP); 
+    }
+    my $createexp = Param('createemailregexp');
+    if (!($createexp) || ($login !~ /$createexp/)) 
+    { 
+     return (SelfRegisterNewAccount_FAIL_REGX); 
+    }
+    my $password;
+    $password = InsertNewUser($login, $realname);

This needs to have the identation fixed, and probably the last 2 lines need to
be merged in one line.

The performance regression that I outlined above is still present.

Comment 10

13 years ago
If the patch is approved and we don't care much about the regression, I can do
the fixes in comment 8 and comment 9 upon checkin.
(Assignee)

Comment 11

13 years ago
bugger, mid air collision again.

I was addressing comment 8:

I think either the commentg should stay or the assertion be enabled with 
a 'generic bz error' informing the user that an internal error / assertion 
has ... and to file a bug or what not.
(Assignee)

Comment 12

13 years ago
(In reply to comment #9)
> The performance regression that I outlined above is still present.

I thought I addressed each line of your previous comment, please elaborate.

Comment 13

13 years ago
> > The performance regression that I outlined above is still present.
> 
> I thought I addressed each line of your previous comment, please elaborate.

Yeah, sorry, you're right, I haven't seen comment 4.
Status: NEW → ASSIGNED
I like this, it helps move things away from depending on CGI, so it can be done
more easily from some other UI (and even makes it much easier for an Auth method
to create users on the fly if they authenticate from an external source, in fact)

However, I'm a little bit concerned about the naming of the function and the
related constants.  For some reason it just sticks out as too wordy, and I'm
wondering if we can come up with a simpler name to use.

I also think globals.pl is the wrong place for it.  globals.pl as a whole is
deprecated, and we intend to make the whole thing go away as soon as we can find
more suitable homes for the existing functions that are in it.  Perhaps this
should go in Bugzilla/User.pm?
Flags: approval?
Target Milestone: --- → Bugzilla 2.20
Comment on attachment 160209 [details] [diff] [review]
constants, whitespace, etc...

adding review- per comment 14
Attachment #160209 - Flags: review-
Jason: let's hold off on re-spinning the patch pending the naming/placement
discussion, once we settle that you can re-spin the patch.

This looks cool, though, thanks!
myk, gerv, joel: any comments?
As dave says: global.pl is the wrong place, the constants don't match our
STANDARD_CONVENTION, and the function name is too wordy (what does the "self"
mean in it?). Other than that, seems OK.

Gerv
(Assignee)

Comment 19

13 years ago
oops, my update from oct 9th is lost.

I was going to say 
"Bugzilla::User - Object for a Bugzilla user" and 
"Data obtained from here is read-only"
http://www.bugzilla.org/docs/tip/pod/Bugzilla/User.pm


it doesn't /shouldn't go in any of these: 
 Bugzilla.pm 
 Bugzilla::Config.pm
 Bugzilla::User.pm
 Bugzilla::Util.pm


so either it has to have a new file, or globals.pl for now.

new patch for "new file" underway. and new function name will be something like

  public_new_user
  public_add_user
  public_add_new_user
  public_create_user
  public_new_account
  public_create_account

argue quickly please.

Location will be:

 Admin::Account.pm

based on the frame work described below.

 Admin.pm          -- generic admin functions not specific to the below
 Admin::System.pm  -- System wide administration.
 Admin::Local.pm   -- Local config issues, kinda like dealing with localconfig
 Admin::Account.pm -- for dealing with accounts, creation, lockout, etc.
** maybe later, we can argue this next idea **
 Admin::Catgory.pm -- for product, component, custom fields, custom res...
The documentation says:

"This package handles Bugzilla users. Data obtained from here is read-only;
there is currently no way to modify a user from this package."

The key word is "currently".  That doesn't mean it needs to stay that way.

I think User.pm is the correct place for this.
and why do we need the "public_" prefix?  The "selfregister" part of the old one
is what was bothering me before.  Who is self?  and who is public?  Aren't we
just centralizing the create user code?

Comment 22

13 years ago
I spoke to Dave on IRC. The plan should be:

- Bugzilla::User::create()

  [Create a new User]

This follows the OO practice other modules implement. Don't worry about that POD
comment; it is a bit misguided, since we already have a method there
(derive_groups) that writes to the database and therefore "writes to" the user.
We could clean that up, but *not* in this bug.

- USER_CREATE_OK
  USER_CREATE_DUPE
  USER_CREATE_INVALID

These are in accordance to the other constants defined, and actually helps tie
the constants in with the module that is going to use them.

Hopefully that is clear enough.

Comment 23

13 years ago
Comment on attachment 160209 [details] [diff] [review]
constants, whitespace, etc...

Yes, this needs to be Bugzilla::User::create. It's a good idea, though.
Attachment #160209 - Flags: review-

Comment 24

13 years ago
Actually, we already have Bugzilla::User::insert_new_user, which does most of
the work. And for any actual API for external stuff, we're going to be using the
XML-RPC interface in bug 224577. So that bug will probably handle the actual
end-user need for this.
Severity: normal → enhancement
Depends on: 224577
Target Milestone: Bugzilla 2.20 → ---
Version: unspecified → 2.19.1

Updated

11 years ago
QA Contact: mattyt-bugzilla → default-qa

Comment 25

10 years ago
Hm, is this bug depending on bug 350232, or is it a duplicate ?

Comment 26

9 years ago
(In reply to comment #25)
> Hm, is this bug depending on bug 350232, or is it a duplicate ?

It's a dupe.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 350232
You need to log in before you can comment on or make changes to this bug.