strongloop / loopback-sdk-angular

Service for auto-generating Angular $resource services for LoopBack
Other
155 stars 82 forks source link

user can't update its own data (401) #125

Open Morriz opened 9 years ago

Morriz commented 9 years ago

The loopback angular model.$save (or Model.update for that matter) defaults to $upsert, which tries a batch update on the server, which is not allowed for just the $owner. The PUT method for a logged in user in the explorer works though.

Difference in calls is (while using same body payload):

explorer PUT: /api/Users/someid (allowed) loopback-services.js PUT: /api/Users?id=someid (not allowed because of reason mentioned)

output after call from loopback-services.js:

loopback:security:role isInRole(): $everyone +0ms
  loopback:security:access-context ---AccessContext--- +1ms
  loopback:security:access-context principals: +0ms
  loopback:security:access-context principal: {"type":"USER","id":1} +0ms
  loopback:security:access-context modelName User +0ms
  loopback:security:access-context modelId 1 +0ms
  loopback:security:access-context property upsert +0ms
  loopback:security:access-context method upsert +0ms
  loopback:security:access-context accessType WRITE +0ms
  loopback:security:access-context accessToken: +0ms
  loopback:security:access-context   id "KCXtsjtfaDpwMh2IExPFHCS3DgIAF1oL56hW7mWCbCvVoFcExbzJAWW6DAkVSQAZ" +0ms
  loopback:security:access-context   ttl 1209600 +0ms
  loopback:security:access-context getUserId() 1 +1ms
  loopback:security:access-context isAuthenticated() true +0ms
  loopback:security:role Custom resolver found for role $everyone +0ms
  loopback:security:acl The following ACLs were searched:  +0ms
  loopback:security:acl ---ACL--- +0ms
  loopback:security:acl model User +0ms
  loopback:security:acl property * +0ms
  loopback:security:acl principalType ROLE +0ms
  loopback:security:acl principalId $everyone +0ms
  loopback:security:acl accessType * +0ms
  loopback:security:acl permission DENY +0ms
  loopback:security:acl with score: +0ms 7495
  loopback:security:acl ---Resolved--- +0ms
  loopback:security:access-context ---AccessRequest--- +0ms
  loopback:security:access-context  model User +1ms
  loopback:security:access-context  property upsert +0ms
  loopback:security:access-context  accessType WRITE +0ms
  loopback:security:access-context  permission DENY +0ms
  loopback:security:access-context  isWildcard() false +0ms
  loopback:security:access-context  isAllowed() false +0ms
joncarlson commented 9 years ago

Also experiencing this (not using Postgres). If I change the URL in $upsert from urlBase + '/people' to urlBase + '/people/:id' updates work correctly.

bajtos commented 9 years ago

IIRC, there is an important difference between "PUT /api/people" and "PUT /api/people/:id": the former replaces all properties with the data in the request and deletes any properties not included in the request, while the latter updates only the properties specified by the request and leaves the rest untouched.

@raymondfeng @ritch you are more familiar with ACLs than I am. Is it expected that updateOrCreate is not allowed for $owner role?

bajtos commented 9 years ago

Apparently PUT /api/people (updateOrCreate) and PUT /api/people/:id (prototype.updateAttributes) is all that loopback provides, in which case we have to use "updateAttributes" to update the current user.

There are two things that can be done.

A) Change your Angular app to call updateAttributes instead of save:

// before: User.$save();
User.prototype$updateAttributes({ id: user.id }, user);

B) Rework the implementation of $save (source) to detect whether the id property is set. If it is, then call Model.prototype$updateAttributes, otherwise call Model.create.

nolandubeau commented 9 years ago

I would vote for B.

On Wednesday, March 4, 2015, Miroslav Bajtoš notifications@github.com wrote:

Apparently PUT /api/people (updateOrCreate) and PUT /api/people/:id (prototype.updateAttributes) is all that loopback provides, in which case we have to use "updateAttributes" to update the current user.

There are two things that can be done.

A) Change your Angular app to call updateAttributes instead of save:

// before: User.$save(); User.prototype$updateAttributes({ id: user.id }, user);

B) Rework the implementation of $save (source https://github.com/strongloop/loopback-sdk-angular/blob/fa7668b8e11e04fad8d239558846804e5d6315f8/lib/services.template.ejs#L392-L400) to detect whether the id property is set. If it is, then call Model.prototype$updateAttributes, otherwise call Model.create.

— Reply to this email directly or view it on GitHub https://github.com/strongloop/loopback-sdk-angular/issues/125#issuecomment-77119483 .

Nolan Dubeau VP, Engineering Guardly

Sent from my Apple Newton

bajtos commented 9 years ago

As I mentioned in https://github.com/strongloop/loopback-datasource-juggler/issues/481, another option is to add a new method "prototype.update" that would always update all attributes and which can be used in $save.

pbrain19 commented 8 years ago

this issue does not seem to be resolved. Currently trying to update customer and it does not seem to have the ownership to do so.

rhoderunner9 commented 8 years ago

The documentation online does not seem to be sufficient to cover how to do simple tasks. I have set up the updateAttributes, and even just threw my hands up and gave $owner unlimited access, and still get the Authentication Required error. This tells me that this user is not the owner of his/her data. The documentation is vast, yet seems to leave many of us hanging with inadequate answers to very common issues. I have gone so far as to set the "property": "update" to ALLOW for $everyone, yet I still get Authentication Required. I'm assuming I'm one of thousands of people spending amazing amounts of human hours on these kind of issues, so could someone clarify how to setup the model update ACL effectively?

superkhau commented 8 years ago

@crandmck Maybe you can help out with the docs here? @rhoderunner9 Do you have any suggestions on how to improve it?

Sequoia commented 8 years ago

@superkhau is there a bullet list/nutshell version of what the proper solution is here? I can write documentation with examples etc. once I know what the recommended approach is but it's not clear from this thread what it is.

Can @bajtos or someone else familiar with the topic clarify the best/proper way to:

  1. set up ACLs to work with the angular SDK
  2. call the API via the angular SDK ($save, $update...?)

Alternately, I can dig in & figure out the solution myself but that is likely to be very time consuming & probably wasted/duplicated effort.

superkhau commented 8 years ago

@bajtos Would know more about the design/intent of the Angular SDK here. @raymondfeng Could help with the intent of the ACL parts.

With regards to 1 and 2, https://github.com/strongloop/loopback-getting-started-intermediate has a flushed out example that uses Angular which ACLs to demonstrate the typical use case. The issue is either a (the workaround) or b (the feature request) from @bajtos's response.

bajtos commented 8 years ago

I am afraid I don't have enough bandwidth to look into this issue right now. I'm moving it to #tob so that we can set aside time to work on this as part of our Scrum process.

Sequoia commented 8 years ago

Please re-assign me when this is ready-for-docs! 🌴

crandmck commented 7 years ago

@bajtos bump....?

bajtos commented 7 years ago

@davidcheung do you perhaps have bandwidth to help me with this issue? I think we should be able to reproduce this problem in our test suite (enable authentication, create a user, try to update the user via userInstance.$save()). I can investigate myself what's going on once we have the new test added (e.g. in a feature branch).

davidcheung commented 7 years ago

yeah this is still a problem, will add to my backlog can reproduce via : https://github.com/strongloop/loopback-sdk-angular/compare/user-cannot-save

luncht1me commented 7 years ago

This is still a problem...

upsert() is doing a PATCH to Model?id= instead of Model/id and failing miserably.

I don't like having to modify my lb-services each time we generate it on an update as per @joncarlson's suggestion.... It's so elegant to just do a Model.upsert(instance) and have it update properly... Having to do Model.update({where:{id:instance.id}}, _.omit(instance, 'id')) is a real pain in the arse when upsert worked in previous versions.

Dangerous things happen when using Update, and not validating that id is actually defined. One of our juniors overwrote everything in a collection by doing this w/o a properly verified id....

rhoderunner9 commented 7 years ago

I have run into this issue several times. I think one possible solution is to modify the service generation app that writes the normal interfaces to the same sdk directory, making all the generated classes abstract. A secondary directory (something like "sdk-impl") can be generated if it does not exist, otherwise providing feedback that an interface has changed, and therefore should be verified so that individual modifications are maintained. If the interface to the service hasn't changed, no notification is necessary. If it has changed, then a notification can be specific to the changes. I don't think this would be too difficult. A simple properties file in the pre-existing sdk specifying the previous api could be used to provide information on the changes that need to be addressed in the sdk-impl. I've been maintaining my modifications in a WICHTIG.md file that keeps track of the things that need to be changed each time I generate the sdk, so it'd be nice to rely on my change set.