Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Closes #5913: Add new sessionToken scope #6155

Merged
merged 1 commit into from
Dec 12, 2019

Conversation

vladikoff
Copy link
Contributor

Relanding #5914

@grigoryk r?

@codecov-io
Copy link

codecov-io commented Oct 21, 2019

Codecov Report

Merging #6155 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #6155      +/-   ##
============================================
- Coverage     18.93%   18.93%   -0.01%     
  Complexity      436      436              
============================================
  Files           288      288              
  Lines         11287    11288       +1     
  Branches       1536     1536              
============================================
  Hits           2137     2137              
- Misses         8988     8989       +1     
  Partials        162      162
Impacted Files Coverage Δ Complexity Δ
...org/mozilla/fenix/components/BackgroundServices.kt 46.42% <0%> (-0.42%) 4 <0> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32985af...857a3b2. Read the comment docs.

Copy link
Contributor

@grigoryk grigoryk left a comment

Choose a reason for hiding this comment

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

LGTM, provided both pairing and regular flows work :-)

@grigoryk
Copy link
Contributor

Still seems to crash in @vladikoff 's local testing.

@grigoryk
Copy link
Contributor

grigoryk commented Nov 2, 2019

@vladikoff any updates on this?

@vladikoff
Copy link
Contributor Author

@vladikoff any updates on this?

We should probably wait for Firefox 71.

@rfk do you have any other ideas? I am wondering if we can do something to "handle" crashes that happen if the OAuth code cannot be exchanged.

@grigoryk
Copy link
Contributor

grigoryk commented Nov 5, 2019

I'm fine with waiting for 71 (a month from now).

@rfk
Copy link
Contributor

rfk commented Nov 5, 2019

I'm fine with waiting for 71 (a month from now).

Depends how we feel about the experience on ESR.

The reason this is a problem on 70 but not 71, is that 70 uses BrowserID assertions for authorizing OAuth codes, while 71 directly uses its sessionToken. Since BrowserID assertions don't tell you what sessionToken was used to generate them, we can't track the sessionToken info through the OAuth code grant and use it to generate a cloned session on the other side.

ow that danny has landed phase 2 of the auth/oauth server merger, it's possible that we could do something more clever on the server-side here. For example, I believe BrowserID assertions embed the originating deviceId, we might be able to use that to look up the sessionToken that should be associated with a particular OAuth code.

Alternately, when claiming the code, the server could detect the case where we can't generate a sessionToken and could remove it from the list of granted scopes, rather than failing the whole request. The client would need to be prepared to handle the case where it doesn't get granted all the scopes it requested, but OAuth clients should in theory be prepared for that at any time.

@vladikoff
Copy link
Contributor Author

@grigoryk Rebased!

@ekager ekager added pr:needs-landing PRs that are ready to land [Will be merged by Mergify] and removed pr:do-not-land labels Dec 11, 2019
@NotWoods NotWoods merged commit a3f2f55 into mozilla-mobile:master Dec 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr:needs-landing PRs that are ready to land [Will be merged by Mergify]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants