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

pubsub: implemented iam methods #767

Merged
merged 1 commit into from
Aug 14, 2015
Merged

pubsub: implemented iam methods #767

merged 1 commit into from
Aug 14, 2015

Conversation

callmehiphop
Copy link
Contributor

Closes #758

  • Add support for IAM methods within Topic and Subscription
  • IAM#getPolicy
    • unit tested
    • system tested
    • documented
  • IAM#setPolicy
    • unit tested
    • system tested
    • documented
  • IAM#testPermissions
    • unit tested
    • system tested
    • documented

I'm getting a 503 for my testPermissions system test -- not sure if it's a problem upstream or implementation/user error.

documentation preview

@callmehiphop callmehiphop added api: pubsub Issues related to the Pub/Sub API. don't merge labels Jul 30, 2015
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 30, 2015
* var topic = pubsub.topic('my-topic');
* var iam = topic.iam;
*/
function Iam(pubsub, resource) {

This comment was marked as spam.

This comment was marked as spam.

@callmehiphop
Copy link
Contributor Author

Couple of questions around testPermissions

  1. I can't get that API call to go through, however per the discovery docs it's supposed to return a subset of the permissions you're testing for. Would it be better to return a hash so the user doesn't have to do a bunch of indexOf checks? (assuming that's how they'd use it)

    var tests = [
      'storage.bucket.list',
      'storage.bucket.insert'
    ];
    iam.testPermissions(tests, function(err, permissions) {
      console.log(permissions);
      // => {
      //   "storage.bucket.list": true,
      //   "storage.bucket.insert": false
      // }
    });
  2. If we do want to go with the hash, then should we rename the method to hasPermissions?

  3. If we don't want to go with the hash and the user provides a single permission, should we return a boolean instead of an array?

    iam.testPermissions('storage.bucket.list', function(err, hasPermission) {
      console.log(hasPermission);
      // => true
    });

@callmehiphop
Copy link
Contributor Author

@tmatsuo - any insight you could provide would be greatly appreciated!

@tmatsuo
Copy link
Contributor

tmatsuo commented Aug 3, 2015

1 sounds good.
2 sounds like the method is supposed to return boolean, but actually not. I would keep testPermissions.
3 seems confusing to me.

BTW, the example is using storage permissions. You can use permissions like pubsub.topics.publish and pubsub.subscriptions.consume. Also I would like to see the pubsub specific example, like testing permissions on topics/subscriptions.

@callmehiphop
Copy link
Contributor Author

The pubsub specific way is the same, I decided to treat the iam object in the same fasion as an acl (property and not subclass). A more complete example would look like this

var topic = pubsub.topic('my-topic');
var tests = ['pubsub.topics.publish'];

topic.iam.testPermissions(tests, function(err, permissions, apiResponse) {});

@callmehiphop
Copy link
Contributor Author

Updated post to include a link for a documentation preview.

@callmehiphop
Copy link
Contributor Author

Now that Pub/Sub is out of beta (https://github.com/GoogleCloudPlatform/gcloud-common/issues/15) and IAM has official documentation. I've updated the system tests and documentation/code examples and everything looks good to go!

* @param {?error} callback.err - An error returned while making this request.
* @param {array} callback.permissions - A subset of permissions that the caller
* @param {object} callback.apiResponse - The full API response.
* is allowed

This comment was marked as spam.

@callmehiphop
Copy link
Contributor Author

Maybe we can add some convenience methods

It looks like there are 5 roles currently. Would you want methods for each one?

@stephenplusplus
Copy link
Contributor

Yeah, I think that would be much easier than remembering those values.

Maybe:

topic.iam.subscribers.add('serviceAccount:myotherproject@appspot.gserviceaccount.com', callback);
topic.iam.viewers.add('user:sean@example.com', callback);

// arrays also:
subscription.iam.owners.add(['...', '...'], callback);

Hopefully that example actually makes sense. Not an expert with IAMs yet!

*
* @example
* //-
* // Test a single permission

This comment was marked as spam.

@stephenplusplus
Copy link
Contributor

Throughout the examples, can you have the callbacks receive the apiResponse parameter as well?

@callmehiphop
Copy link
Contributor Author

Made some documentation updates to include upstream resource links.

};

iam.getPolicy(assert.ifError);
});

This comment was marked as spam.

updated examples and system tests per official docs

tweaked documentation and code style

added upstream doc references

added error handling unit tests
stephenplusplus added a commit that referenced this pull request Aug 14, 2015
@stephenplusplus stephenplusplus merged commit 14523d3 into googleapis:master Aug 14, 2015
@stephenplusplus
Copy link
Contributor

Awesome!

This is you:

@callmehiphop
Copy link
Contributor Author

@callmehiphop callmehiphop deleted the pubsub-policy branch August 18, 2015 19:53
sofisl pushed a commit that referenced this pull request Jan 17, 2023
Update npm scripts: add clean, prelint, prefix; make sure that lint and fix are set properly. Use post-process feature of synthtool.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants