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

Get the authenticated user in a remote method #240

Closed
wants to merge 1 commit into from
Closed

Get the authenticated user in a remote method #240

wants to merge 1 commit into from

Conversation

ataft
Copy link

@ataft ataft commented Jan 12, 2017

Get the authenticated user in a remote method (now that currentContext is deprecated). This is from @jankcat example in this issue 569.

Get the authenticated user in a remote method (now that currentContext is deprecated)
@slnode
Copy link
Contributor

slnode commented Jan 12, 2017

Can one of the admins verify this patch?

@crandmck
Copy link
Contributor

http: function(ctx) {
return ctx.req.accessToken;
}
}
Copy link
Member

@bajtos bajtos Jan 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am afraid this is using a different approach than the one described by this documentation page. While this approach is still valid, I think your example will only confuse readers.

Here is a better example:

// common/models/my-model.js
module.exports = function(MyModel) {
  MyModel.log = function(messageId, options) {
    const Message = this.app.models.Message;
    return Message.findById(messageId, options)
      .then(msg => {
        const userId = options && options.accessToken && options.accessToken.userId;
        const user = userId ? 'user#' + userId : '<anonymous>';
        console.log('(%s) %s', user, msg.text));
      });
  };

// common/models/my-model.json
{
  "name": "MyModel",
  // ...
  "methods": {
    "log": {
      "accepts": [
        {"arg": "messageId", "type": "number", "required": true},
        {"arg": "options", "type": "object", "http": "optionsFromRequest"}
      ],
      "http": {"verb": "POST", "path": "/log/:messageId"}
    }
  }
}

I also think that this example should be placed elsewhere in this page, so that it does not break the flow of text/thoughts here. For example, we can place it at the end of this section "Annotate options argument in remoting metadata".

@crandmck thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if it's an alternative approach, it should be described separate from the existing documented approach. Also, nnow that I read through this more closely, it might be worthwhile to add a bit more info to the existing section, e.g. clarify that the JSON block:

{
  "arg": "options",
  "type": "object",
  "http": "optionsFromRequest"
}

Goes in common/models/my-model.json (and perhaps show more of the surrounding JSON).

Also, can we explain why to use one approach vs. the other?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Admittedly, I am a newbie to stuff so just trying to wrap my head around it. I've been reading about various approaches to context and using the access token, and it appears that many people are also in the same boat.

With regards to the access token and making authenticated requests, I eventually landed on putting the access token in the request header as the Authorization parameter rather than in the request body as the "access_token" parameter. Various articles seemed to say this approach is a bit safer.

With the recommended options approach to current context, why do I need to send options in the request when I'm already sending the access token in the request header?

@bajtos
Copy link
Member

bajtos commented Jan 17, 2017

Let's move the discussion from the line-comment level (#240 (review)) to the main thread.

@crandmck

Also, now that I read through this more closely, it might be worthwhile to add a bit more info to the existing section, e.g. clarify that the JSON block goes in common/models/my-model.json (and perhaps show more of the surrounding JSON).

👍

Also, can we explain why to use one approach vs. the other?

I would say the approach proposed in this patch is a bit more manual and explicit - the remote method receives only the accessToken property of the request, there is no option for customisation of this value. Also this kind of remoting metadata cannot be configured via model JSON files, as it requires a function definition. The benefit is that readers of this code know immediately what to expect in this input argument.

An "options" argument constructed via "optionsFromRequest" requires more knowledge of the framework to understand what will be the value provided when the method is invoked via REST. The benefit is that such argument can be described in a model JSON file and "options" value can be easily customised.

@ataft

Admittedly, I am a newbie to stuff so just trying to wrap my head around it. I've been reading about various approaches to context and using the access token, and it appears that many people are also in the same boat.

Now worries 😃 You have provided us valuable feedback, thank you for that!

With regards to the access token and making authenticated requests, I eventually landed on putting the access token in the request header as the Authorization parameter rather than in the request body as the "access_token" parameter. Various articles seemed to say this approach is a bit safer.

+1

Having said that, the access token is extracted from the HTTP request by our token middleware, which is looking in multiple places for this token.

With the recommended options approach to current context, why do I need to send options in the request when I'm already sending the access token in the request header?

With the recommended options approach, the options argument is constructed by the server. There is no need to send the options in the request, in fact such options would be ignored by the server anyways.

@bajtos
Copy link
Member

bajtos commented Jan 17, 2017

I am not sure what's the best way for moving this pull request forward. I think we discovered this documentation page needs more changes than just adding a code snippet. Are you still keen to continue this work @ataft, or would it be better for @crandmck (or me) to take this over from you?

@ataft
Copy link
Author

ataft commented Jan 22, 2017

Please take it over from me, my understanding of the subject isn't good enough to make this what it needs to be. If I can add anything at any point, then I will try to do so. Thanks!

@bajtos
Copy link
Member

bajtos commented Jan 23, 2017

Closing in favour of #256

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.

4 participants