Rearrange the reg/sreg proxy

RESOLVED FIXED

Status

Cloud Services
Server: Core
--
major
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: tarek, Assigned: tarek)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Assignee)

Description

7 years ago
Following up the discussion with Toby about reg/sreg

- let's introduce a lightweight controller for sreg that provides an access to the auth backend for all write operations
- a mozilla backend, inherited from the ldapsql one, will proxy the write calls when it makes sense
(Assignee)

Updated

7 years ago
Duplicate of this bug: 622698
(Assignee)

Comment 2

7 years ago
A dedicated repository was created at bug 627804.

I am starting to work on this new repo. The final goal is to provide a python26-sync-sreg package that will replace python26-sync-reg on the sreg servers.
Assignee: nobody → tarek
Severity: normal → major
Status: NEW → ASSIGNED
Hardware: x86_64 → All
(Assignee)

Updated

7 years ago
Blocks: 608039
(Assignee)

Comment 3

7 years ago
Toby, since we will probably never run a Python reg against a PHP sreg, I think it's a good opportunity to make a few changes to the sreg APIs to make them simpler. I know these are nit-picking changes, but since I am creating the sreg server now it's the same amount of work and minimal changes on the not-yet-in-production mozilla backend.

Currently, we have these APIs:

- PUT /:username/ => user name
- GET /:username/node/weave  => location 
- GET /:username/reset_code    => array with a 'code' key
- POST /:username/password_reset  => array with a 'success' key
- DELETE /:username/password_reset  => array with a 'success' key
- POST /:username/verify_password_reset => array with a 'success' key
- POST /:username/password   => array with a 'success' key and a 'reason' key in case of a failure

I'd like to have a unified API name for the reset code

Also, some return values can be simplified, since we basically want to know if the operation was successful, and if not get an error code. Last, "success" could be replaced by the error code '0' since it's not used yet at https://wiki.mozilla.org/Labs/Weave/1.0/ResponseCodes. The client would just have to read an int code and act upon.

Proposal (where return code is 0 (success) or any error code):

- PUT /:username/   => user name
- POST /:username/password  
- GET /:username/reset_code    => the reset code
- POST /:username/reset_code   =>  return code
- DELETE /:username/reset_code  => return code
- POST /:username/verify_reset_code => return code

= Open questiona =

Updating the email will change the dn so we could need to proxy this change (or other update needs in the future). so maybe we could add an API to proxy any change to a user:

- POST /:username  (with new values in the body as a json mapping)

The node/weave API could be simplified by

- GET /:username/node   

Since there's nothing specific about weave. But is this API is going o be useful in the future since we're planning to have another server for node attributions ?
(Assignee)

Comment 4

7 years ago
Initial code base done at: http://hg.mozilla.org/users/tziade_mozilla.com/server-sreg/

Will wait for feedback then update it for code review
(In reply to comment #3)


> I'd like to have a unified API name for the reset code
> 
> Also, some return values can be simplified, since we basically want to know if
> the operation was successful, and if not get an error code. Last, "success"
> could be replaced by the error code '0' since it's not used yet at
> https://wiki.mozilla.org/Labs/Weave/1.0/ResponseCodes. The client would just
> have to read an int code and act upon.
> 

That seems fine.


> Proposal (where return code is 0 (success) or any error code):
> 
> - PUT /:username/   => user name
> - POST /:username/password  
> - GET /:username/reset_code    => the reset code
> - POST /:username/reset_code   =>  return code
> - DELETE /:username/reset_code  => return code
> - POST /:username/verify_reset_code => return code
> 

I think there's one too many functions here.

For the reset code, we need 3 functions: create, read (or verify) and delete

Read and verify do the same thing - check the code - the only difference being whether the client or server does the check. My instinct tells me that we should have the client do the check, which means we need:

GET /:username/password_reset_code
DELETE /:username/password_reset_code

In fact, we can really simplify to just these two. GET returns the current value, or generates a new one and returns that if it doesn't exist. When the user first requests a code, we'd issue a DELETE, followed by a GET.

(we should use password_reset_code here, because it's conceivable we might have another reset code in the future)


> = Open questiona =
> 
> Updating the email will change the dn so we could need to proxy this change (or
> other update needs in the future). so maybe we could add an API to proxy any
> change to a user:
> 
> - POST /:username  (with new values in the body as a json mapping)
> 

I think the user is able to change their own dn, so I'm not sure this is needed.


> The node/weave API could be simplified by
> 
> - GET /:username/node   
> 
> Since there's nothing specific about weave. But is this API is going o be
> useful in the future since we're planning to have another server for node
> attributions ?

Odds are pretty good we'll have more servers that need node attributions. If Bespin had become part of services, it would have had a node. Eventually, I suspect we'll have a dedicated API for node assignments, but for now, I'd just leave it.
(Assignee)

Comment 6

7 years ago
(In reply to comment #5)
> 
> For the reset code, we need 3 functions: create, read (or verify) and delete
> 
> Read and verify do the same thing - check the code - the only difference being
> whether the client or server does the check. My instinct tells me that we
> should have the client do the check, which means we need:
> 
> GET /:username/password_reset_code
> DELETE /:username/password_reset_code
> 
> In fact, we can really simplify to just these two. GET returns the current
> value, or generates a new one and returns that if it doesn't exist. When the
> user first requests a code, we'd issue a DELETE, followed by a GET.

Two APIs seem better indeed. But the difference between the POST and the GET on the reset code is that the first one sends the email.

So, currently, sreg is responsible for sending the mails when a reset code is created. I am not sure we want to keep it that way because emails should be sent by the front layer (the reg app or the account manager app) since they might create a custom mail depending on app configurations (like i18n or branding)

So I would keep just two API and delegate the mail sending to the reg and account apps.

Will start a wiki page about the sreg server to describe its APIs.
(Assignee)

Comment 7

7 years ago
I have started https://wiki.mozilla.org/Services/Sync/SRegAPI
(Assignee)

Comment 8

7 years ago
(In reply to comment #6)
> When the user first requests a code, we'd issue a DELETE, followed by a GET.

We can also add a X-Weave-Delete-Previous header in the GET to reset any previous reset code.
(In reply to comment #6)
> (In reply to comment #5)

> So, currently, sreg is responsible for sending the mails when a reset code is
> created. I am not sure we want to keep it that way because emails should be
> sent by the front layer (the reg app or the account manager app) since they
> might create a custom mail depending on app configurations (like i18n or
> branding)
> 

Right. Reg should be sending the emails, and it was just a historical error that sreg ended up doing so.
(Assignee)

Comment 10

7 years ago
Ok. Reworking the code now so it implements https://wiki.mozilla.org/Services/Sync/SRegAPI
(Assignee)

Comment 11

7 years ago
I updated http://hg.mozilla.org/users/tziade_mozilla.com/server-sreg

It's ready for a review at this stage, but we still to discuss CEF logging in sreg.
sreg doesn't generally log into CEF, as any error should be propagated back to reg (and then generates double-logging), so we should be good there.
(Assignee)

Comment 13

7 years ago
(In reply to comment #12)
> sreg doesn't generally log into CEF, as any error should be propagated back to
> reg (and then generates double-logging), so we should be good there.

ok good.

So once you're +r on the clone I will push it to hg.m.o/services
r+ on the clone. However, we can't change the mozilla.py auth to work with it, as it needs to talk to the php version for a while.
(Assignee)

Comment 15

7 years ago
Done in http://hg.mozilla.org/services/server-sreg/
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.