Skip to content

Describe Authorization scopes - #338

Merged
bajtos merged 1 commit into
gh-pagesfrom
feature/authorization-scopes
Apr 27, 2017
Merged

Describe Authorization scopes#338
bajtos merged 1 commit into
gh-pagesfrom
feature/authorization-scopes

Conversation

@bajtos

@bajtosbajtos commented Mar 29, 2017

Copy link
Copy Markdown
Member

Documentation for the new feature to be added by strongloop/loopback#3313.

Connect to strongloop/loopback#382

@bajtosbajtos self-assigned this Mar 29, 2017
@bajtos
bajtosforce-pushed the feature/authorization-scopes branch 2 times, most recently from f86fa19 to 9b37503CompareMarch 29, 2017 12:28
@bajtos

Copy link
Copy Markdown
MemberAuthor

(This pull request must not be landed before the change in LoopBack is finished.)

@crandmck

Copy link
Copy Markdown
Contributor

Actually, the doc PR should not be landed until the code is landed AND published to npm.

@bajtosbajtos mentioned this pull request Apr 6, 2017
3 tasks
@bajtos
bajtosforce-pushed the feature/authorization-scopes branch from 9b37503 to e5dbb0fCompareApril 7, 2017 13:27
@bajtos

Copy link
Copy Markdown
MemberAuthor

I have reworked the pull request to match what has been landed by strongloop/loopback#3313.

@ebarault@crandmck could you please review again?

@bajtos
bajtos requested a review from ebaraultApril 7, 2017 13:28
@bajtosbajtos changed the title [DO-NOT-MERGE] Describe Authorization scopesDescribe Authorization scopesApr 7, 2017
@bajtos
bajtosforce-pushed the feature/authorization-scopes branch from e5dbb0f to 430de8aCompareApril 7, 2017 13:31

@crandmckcrandmck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I went ahead and made wording and grammar edits directly in the branch, since that seemed most efficient.

IIUC, we should also update https://loopback.io/doc/en/lb3/Model-definition-JSON-file.html#methods since this feature will now be implemented. Is that right?
If so, please add to this PR--or if you prefer I can take a stab at it next week.
Again, if I'm understanding, this reference info should largely duplicate what's in https://loopback.io/doc/en/lb3/Remote-methods.html#options. If so, then instead of
actually duplicating these tables, we can move them into the _includes/content and
single-source them so we only have to maintain them there.

The scopes are defined together with other remoting metadata via the new
setting `accessScopes`. The user can invoke the remote method as long as their access token
is granted at least one of the scopes listed in `accessScopes` array.
The new model property `accessScopes` defines scopes as part of remoting metadata.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@crandmck
accessScopes is not a model property, it is a new property in remote method metadata. My proposal:

- The new model property `accessScopes` defines scopes as part of remoting metadata.+ The new setting `accessScopes` defines scopes as part of remoting metadata.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IIUC, it's a value set in the model JSON file, which is what I meant by a "model property". Apparently that is confusing, but "setting" doesn't really convey any specific meaning.

Perhaps it would be clearer to say

The methods.accessScopes property in the model JSON file defines...

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Done in ac92730

@bajtos

Copy link
Copy Markdown
MemberAuthor

@crandmck thank you for the edits.

https://loopback.io/doc/en/lb3/Remote-methods.html#options

Good catch, accessScopes should be added to that table. I'll do that ASAP.

IIUC, we should also update https://loopback.io/doc/en/lb3/Model-definition-JSON-file.html#methods since this feature will now be implemented. Is that right?

I am confused. The current doc pages says that remote methods cannot be configured via JSON files yet, but that hasn't been true for months. So yes, we should fix that section and I think that fix is independent of the changes I am making here. I am proposing to leave that out of scope of this pull request.

Again, if I'm understanding, this reference info should largely duplicate what's in https://loopback.io/doc/en/lb3/Remote-methods.html#options. If so, then instead of
actually duplicating these tables, we can move them into the _includes/content and
single-source them so we only have to maintain them there.

Ditto, I am proposing to leave _includes/content out of scope of this patch.

@bajtos

Copy link
Copy Markdown
MemberAuthor

Updated, PTAL.

@crandmck

crandmck commented Apr 11, 2017

Copy link
Copy Markdown
Contributor

The current doc pages says that remote methods cannot be configured via JSON files yet, but that hasn't been true for months. So yes, we should fix that section and I think that fix is independent of the changes I am making here. I am proposing to leave that out of scope of this pull request.

No one told me, created an issue, or opened a doc PR, so apparently it was not documented (unless I'm missing something). It's closely related to this PR, but we can do it separately if you prefer.
Added #352.

With one small edit noted above I think this is ready to land, WHEN the change is published to npm. We shouldn't land this until that time, though.

@crandmckcrandmck changed the title Describe Authorization scopesDescribe Authorization scopes [WAITING FOR PUBLISH TO npm]Apr 12, 2017
@crandmck

Copy link
Copy Markdown
Contributor

Note: c305994 will require this to be rebased, because I added the doc for remote method config via JSON per #352.

@bajtos
bajtosforce-pushed the feature/authorization-scopes branch from b3b812c to ac92730CompareApril 18, 2017 13:24
@bajtos

Copy link
Copy Markdown
MemberAuthor

Note: c305994 will require this to be rebased, because I added the doc for remote method config via JSON per #352.

Rebased, I moved the newly-added remote method option "accessScopes" to the new place you created.

I think this is ready to land, WHEN the change is published to npm. We shouldn't land this until that time, though.

Sounds good. I'd like to land strongloop/loopback#3350 before making a new loopback 3.x release.

@bajtos

Copy link
Copy Markdown
MemberAuthor

All required changes were landed and released in loopback@3.7.0. I'm going to rebase and land this patch.

@bajtos
bajtosforce-pushed the feature/authorization-scopes branch from ac92730 to 65eea1fCompareApril 27, 2017 11:21
@bajtos
bajtosforce-pushed the feature/authorization-scopes branch from 65eea1f to 96c0fd8CompareApril 27, 2017 11:25
@bajtos
bajtos merged commit 3207ee9 into gh-pagesApr 27, 2017
@bajtos
bajtos deleted the feature/authorization-scopes branch April 27, 2017 11:50
@bajtosbajtos removed the review label Apr 27, 2017
@bajtosbajtos changed the title Describe Authorization scopes [WAITING FOR PUBLISH TO npm]Describe Authorization scopesApr 27, 2017
Sign up for freeto 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

@bajtos@crandmck@superkhau@siddhipai