fix(scopes): Formalize and document scope-handling rules. #551

rfk · 2018-05-02T03:54:27Z

Connects to #575

While working on adding OAuth support for sync, I figured we should have a canonical reference for what sorts of scope we support and how we check for matches. So I started something, and here's the rendered view:

https://github.com/mozilla/fxa-oauth-server/blob/scopes-documentation/docs/scopes.md

I want to add some explicit testcases for this in the code here, but am putting the doc up for early feedback. @vladikoff does this seem sane? I think it differs slightly from the proposal in the original scoped-keys doc, in that it uses the URL fragment for qualifications like "readonly". But I quite like the ability to check scopes by parsing them as URL resource references.

philbooth · 2018-05-02T05:57:12Z

docs/scopes.md

+
+* Be an absolute `https://` URL.
+* Have no query component.
+* Have a fragment component consisting only of alphanumeric ascii characters.


Random drive-by comment, but this wording makes it sound like the fragment part is mandatory because of the MUST before the list. Maybe clarify here with an "if specified..." clause, or split out to a separate MAY list?

(or include MUST / MAY in each bullet rather than aggregated at the top)

Thanks, good call, I will update.

vladikoff · 2018-05-03T02:05:03Z

docs/scopes.md

+introduced during earily development.
+
+**No new short-name scope values should be added.**
+Instead we prefer to use URLs for new scope values,


+No new short-name scope values should be added.
Instead we prefer to use URLs for new scope values,

Ah interesting 👍 . I'm surprised to hear this but i guess this makes sense after looking at Google's list of these scopes

We can always revisit this as time goes on, if we find the URL syntax to be unwieldy.

vladikoff · 2018-05-03T02:05:47Z

docs/scopes.md

+* `https://identity.mozilla.com/apps/oldsync#read` indicates
+   read-only access to the user's data in Firefox Sync.
+* `https://identity.mozilla.com/apps/oldsync/history#write` indicates
+   write-only access to the user's history data in Firefox Sync.


vladikoff

r+, @rfk let's add the link to the fxa-shared implementation that we have in this doc as well (once that lands). I think that should be useful for future readers

rfk · 2018-05-03T04:08:14Z

Thanks @vladikoff; I think I'll leave this open until I have a corresponding PR to use it in some of our other codebases, in case that reveals some opportunities for API cleanup.

shane-tomlinson · 2018-06-04T20:55:55Z

@rfk - is there anything else you want to add to this?

GitCop · 2018-07-04T21:02:05Z

Thanks for contributing! Unfortunately, I'm here to tell you there were the following style issues with your Pull Request:

Commit: 5d198fc
Commits must be in the following format: %{type}(%{scope}): %{description}

Guidelines are available at https://github.com/mozilla/fxa-oauth-server

This message was auto-generated by https://gitcop.com

GitCop · 2018-07-04T21:26:57Z

Thanks for contributing! Unfortunately, I'm here to tell you there were the following style issues with your Pull Request:

Commit: 5d198fc
Commits must be in the following format: %{type}(%{scope}): %{description}

Guidelines are available at https://github.com/mozilla/fxa-oauth-server

This message was auto-generated by https://gitcop.com

GitCop · 2018-07-04T21:42:32Z

Thanks for contributing! Unfortunately, I'm here to tell you there were the following style issues with your Pull Request:

Commit: 5d198fc
Commits must be in the following format: %{type}(%{scope}): %{description}

Guidelines are available at https://github.com/mozilla/fxa-oauth-server

This message was auto-generated by https://gitcop.com

GitCop · 2018-07-04T22:02:32Z

Thanks for contributing! Unfortunately, I'm here to tell you there were the following style issues with your Pull Request:

Commit: 5d198fc
Commits must be in the following format: %{type}(%{scope}): %{description}

Guidelines are available at https://github.com/mozilla/fxa-oauth-server

This message was auto-generated by https://gitcop.com

shane-tomlinson · 2018-07-05T10:48:59Z

@rfk - what's left to do here?

rfk · 2018-07-09T00:28:25Z

I made a combined tracking issue to summarize status: #575

This'll need re-review given the evolution of the shared checking code.

rfk · 2018-07-23T06:33:45Z

This has been updated to use a tag of fxa-shared rather than a github ref; @mozilla/fxa-devs r?

vbudhram · 2018-08-09T00:48:47Z

docs/scopes.md

+* `profile:write` implies `profile:email:write`.
+* `profile:email:write` implies `profile:email`.
+* `profile profile:email:write` implies `profile:email`.
+* `profile profile:email:write` implies `profile:display_name`.


Did you mean profile profile:display_name:write implies profile:display_name?

Did you mean profile profile:display_name:write implies profile:display_name?

profile implies profile:display_name (both read-only), the profile:email:write is needed to write to the email.

vbudhram

@rfk This looks good and makes sense to me. However, I wouldn't mind extra eyes on this since I am not very familiar with the repo.

shane-tomlinson

Only a couple comments about the docs, logic looks good to me.

shane-tomlinson · 2018-08-09T12:19:59Z

docs/scopes.md

+* `profile:locale`: access the user's locale.
+* `profile:avatar`: access the user's avatar picture.
+* `profile:display_name`: access the user's human-readable display name.
+* `profile:amr`: access information about the user's authentication methods and 2FA status.


Should there be any indication on whether any of these are read vs read/write? uid/locale/amr I imagine are read-only.

It's true that the amr cannot be modified, but "profile:amr:write" is still a well-formed scope with well-defined semantics (that doesn't happen to grant you any addition powers over "profile:amr".

shane-tomlinson · 2018-08-09T12:23:48Z

docs/scopes.md

+   but not to other data types.
+* `https://identity.mozilla.com/apps/oldsync#read` indicates
+   read-only access to the user's data in Firefox Sync.
+* `https://identity.mozilla.com/apps/oldsync/history#write` indicates


Why would one want write-only access to history?

If you're a mobile app that wants to record visits that can show up in the activity-stream on the user's desktop, but we don't want to let you read the sites that the user has been visiting on other devices.

shane-tomlinson · 2018-08-09T12:24:26Z

docs/scopes.md

+
+We say that a scope value A *implies* another scope value B
+if they are exactly equal,
+of if A represents a more general capability than B.


of if=> or if

shane-tomlinson · 2018-08-09T12:27:53Z

docs/scopes.md

+* `profile:write` implies `profile:email:write`.
+* `profile:email:write` implies `profile:email`.
+* `profile profile:email:write` implies `profile:email`.
+* `profile profile:email:write` implies `profile:display_name`.


Did you mean profile profile:display_name:write implies profile:display_name?

profile implies profile:display_name (both read-only), the profile:email:write is needed to write to the email.

shane-tomlinson · 2018-08-09T12:31:21Z

docs/scopes.md

+* `profile profile:email:write` does *not* imply `profile:write`.
+* `https` does *not* imply `https://identity.mozilla.com/apps/oldsync`.
+* `https://identity.mozilla.com/apps/oldsync` does *not* imply `profile`.
+* `https://identity.mozilla.com/apps/oldsync#read` does *not* imply `https://identity.mozila.com/apps/oldsync/bookmarks`.


In the URL after "does not imply" do you want "mozila" or "mozilla"? Applies to this and the following 6 lines.

should be mozilla yeap :)

…e them.

rfk · 2018-08-15T04:46:20Z

Thanks @vbudhram @shane-tomlinson, nits fixed.

ghost assigned rfk May 2, 2018

ghost added the waffle:active label May 2, 2018

rfk force-pushed the scopes-documentation branch from 0640db4 to 4aa3e6c Compare May 2, 2018 03:55

philbooth reviewed May 2, 2018

View reviewed changes

rfk force-pushed the scopes-documentation branch from 4aa3e6c to 79672ba Compare May 2, 2018 09:31

vladikoff self-requested a review May 2, 2018 11:56

rfk mentioned this pull request May 3, 2018

feat(scope): Implement shareable code for checking OAuth scopes. mozilla/fxa-shared#21

Merged

vladikoff reviewed May 3, 2018

View reviewed changes

vladikoff previously approved these changes May 3, 2018

View reviewed changes

vladikoff added the train-113 label May 14, 2018

rfk added waffle:next train-114 and removed train-113 waffle:active labels May 22, 2018

shane-tomlinson added train-115 and removed train-114 labels Jun 18, 2018

rfk force-pushed the scopes-documentation branch from 79672ba to 889abd4 Compare July 4, 2018 07:20

ghost added waffle:active and removed waffle:next labels Jul 4, 2018

rfk force-pushed the scopes-documentation branch from 889abd4 to 50ef151 Compare July 4, 2018 08:05

rfk mentioned this pull request Jul 4, 2018

Formalize, document, and implement shared code for oauth scope-checking rules #575

Closed

rfk force-pushed the scopes-documentation branch from 50ef151 to 8005f03 Compare July 4, 2018 21:02

rfk force-pushed the scopes-documentation branch from 8005f03 to a78d8d0 Compare July 4, 2018 21:26

rfk force-pushed the scopes-documentation branch from ab963b3 to 58cd363 Compare July 4, 2018 21:42

rfk force-pushed the scopes-documentation branch from 58cd363 to 765966c Compare July 4, 2018 22:02

rfk force-pushed the scopes-documentation branch 6 times, most recently from c01f8f5 to 83f8875 Compare July 9, 2018 23:01

rfk force-pushed the scopes-documentation branch 3 times, most recently from 8713663 to 6c5a8e4 Compare July 23, 2018 06:26

rfk added train-117 and removed train-116 labels Jul 23, 2018

vladikoff added the shipit label Aug 8, 2018

vbudhram reviewed Aug 9, 2018

View reviewed changes

vbudhram approved these changes Aug 9, 2018

View reviewed changes

shane-tomlinson approved these changes Aug 9, 2018

View reviewed changes

fix(scopes): Document scope-handling rules, use shared code to enforc…

d7ea1fc

…e them.

rfk force-pushed the scopes-documentation branch from 6c5a8e4 to d7ea1fc Compare August 15, 2018 04:46

rfk merged commit 237886d into master Aug 15, 2018

ghost removed train-117 waffle:active labels Aug 15, 2018

shane-tomlinson added this to the FxA-0: quality milestone Sep 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(scopes): Formalize and document scope-handling rules. #551

fix(scopes): Formalize and document scope-handling rules. #551

rfk commented May 2, 2018 •

edited

Loading

philbooth May 2, 2018

philbooth May 2, 2018

rfk May 2, 2018

vladikoff May 3, 2018

rfk May 3, 2018

vladikoff May 3, 2018

vladikoff left a comment

rfk commented May 3, 2018

shane-tomlinson commented Jun 4, 2018

GitCop commented Jul 4, 2018

GitCop commented Jul 4, 2018

GitCop commented Jul 4, 2018

GitCop commented Jul 4, 2018

shane-tomlinson commented Jul 5, 2018

rfk commented Jul 9, 2018

rfk commented Jul 23, 2018

vbudhram Aug 9, 2018

shane-tomlinson Aug 9, 2018

vbudhram left a comment

shane-tomlinson left a comment

shane-tomlinson Aug 9, 2018

rfk Aug 15, 2018

shane-tomlinson Aug 9, 2018

rfk Aug 15, 2018

shane-tomlinson Aug 9, 2018

shane-tomlinson Aug 9, 2018

shane-tomlinson Aug 9, 2018

vladikoff Aug 9, 2018

rfk commented Aug 15, 2018

fix(scopes): Formalize and document scope-handling rules. #551

fix(scopes): Formalize and document scope-handling rules. #551

Conversation

rfk commented May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladikoff left a comment

Choose a reason for hiding this comment

rfk commented May 3, 2018

shane-tomlinson commented Jun 4, 2018

GitCop commented Jul 4, 2018

GitCop commented Jul 4, 2018

GitCop commented Jul 4, 2018

GitCop commented Jul 4, 2018

shane-tomlinson commented Jul 5, 2018

rfk commented Jul 9, 2018

rfk commented Jul 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbudhram left a comment

Choose a reason for hiding this comment

shane-tomlinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk commented Aug 15, 2018

rfk commented May 2, 2018 •

edited

Loading