skygear-server icon indicating copy to clipboard operation
skygear-server copied to clipboard

Include session in user create event

Open kiootic opened this issue 5 years ago • 4 comments

Developers may be interested in session properties (e.g. IP, user agent) in user creation web-hook event. We should include the newly created session in user create event, so that:

  • If user is signed up normally: payload.session is the newly created session, context.session is null.
  • If user is created manually: payload.session is null, context.session is session of the request user.

If the session cannot be created immediately (e.g. MFA is required), both payload.session and context.session may be null.

kiootic avatar Dec 17 '19 11:12 kiootic

@kiootic I think this issue is good, but can you also update the feature spec and also guides / API spec altogether? :)

(btw, to help ensure we don't miss things, it would be good if from now on we follow the cycle of "create feature issue => create implementation, docs issues => update implementation and cos => close feature issue" cycle)

chpapa avatar Dec 18 '19 07:12 chpapa

Actually I've spent the morning think about it, and now I think we should drop this issue for now:

  • There would be some sort of dependency between user_create and session_create events: the session cannot be created before user_create event, and user_create event cannot be generated without creating the session.
  • It would be tricky to implement hook handler correctly. Developer may assume payload.session is always non-null (as in common cases), but the assumption would be broken if we implemented some kind of API for user creation by admin.
  • It's near the time for release and it's better to freeze the features we had for now. It's not a breaking change to add it next version.
  • It can be worked around by handling session_create event with session create reason = signup.

@louischan-oursky @carmenlau @chpapa what do you think about it?

By the way, I think it's okay to open these kind of issues here at first, since I'm not sure how the actual design would goes at first. I think the flow goes like:

  1. Developers open a feature/bug issue here.
  2. Maintainers discuss how to solve the feature/bug.
  3. If the change is major (e.g. requiring API/design change), open issue at feature repo.
  4. Cross-reference the feature issue in original issue.

kiootic avatar Dec 18 '19 07:12 kiootic

Will look at it later, I haven't give much thought yet.

Just make sure if we have a change that should update spec and API references / Guides, we don't forgot the feature issue.

chpapa avatar Dec 18 '19 08:12 chpapa

There would be some sort of dependency between user_create and session_create events: the session cannot be created before user_create event, and user_create event cannot be generated without creating the session.

Agree, it may not be good to have such dependency too

It can be worked around by handling session_create event with session create reason = signup.

Looks good to me

Other thought:

I think ip (and user agent) should not necessarily bind to the session, maybe we will want to have it in the context too? Developer may be interested to request ip before user login, e.g. detect user signup or login attempt in somewhere. We may not need to add this now, but it is good to have in future.

carmenlau avatar Dec 19 '19 02:12 carmenlau