ID Token validation by arvindsinghtomar · Pull Request #203 · openid/AppAuth-Android

arvindsinghtomar · 2017-03-22T12:30:51Z

No description provided.

codecov-io · 2017-03-22T12:41:21Z

Codecov Report

Merging #203 into master will decrease coverage by 5.37%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master     #203      +/-   ##
============================================
- Coverage     81.99%   76.62%   -5.38%     
+ Complexity      461      436      -25     
============================================
  Files            41       41              
  Lines          2227     2237      +10     
  Branches        215      216       +1     
============================================
- Hits           1826     1714     -112     
- Misses          318      444     +126     
+ Partials         83       79       -4

Impacted Files	Coverage Δ	Complexity Δ
library/java/net/openid/appauth/Utils.java	`64.7% <0%> (-19.91%)`	`4 <0> (ø)`
.../java/net/openid/appauth/AuthorizationService.java	`56.85% <0%> (-18.65%)`	`11 <0> (-3)`
library/java/net/openid/appauth/TokenResponse.java	`68.29% <0%> (-24.74%)`	`3% <0%> (-4%)`
library/java/net/openid/appauth/TokenRequest.java	`66.66% <0%> (-20.38%)`	`6% <0%> (-2%)`
...rary/java/net/openid/appauth/CodeVerifierUtil.java	`58.62% <0%> (-6.9%)`	`8% <0%> (-2%)`
.../java/net/openid/appauth/RegistrationResponse.java	`86.95% <0%> (-4.22%)`	`8% <0%> (-1%)`
...penid/appauth/AuthorizationManagementActivity.java	`81.92% <0%> (-1.22%)`	`18% <0%> (-3%)`
library/java/net/openid/appauth/AuthState.java	`71.57% <0%> (ø)`	`78% <0%> (ø)`	⬇️
...java/net/openid/appauth/AuthorizationResponse.java	`77.11% <0%> (ø)`	`10% <0%> (ø)`	⬇️
... and 8 more

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 c640288...6aafff5. Read the comment docs.

nynymike · 2017-03-23T14:30:43Z

Hi guys. Please let Arvind know if there is anything we can do to get this right.

WilliamDenniss · 2017-03-24T14:42:37Z

+    public static String decodeBase64urlNoPadding(String strEncoded)
+        throws UnsupportedEncodingException {
+        if (strEncoded != null) {
+            byte[] decodedBytes = Base64.decode(strEncoded, Base64.NO_PADDING);


I'm pretty sure you need URL_SAFE too.

iainmcgin · 2017-03-25T23:10:19Z

Just so you know I'm not ignoring this: I'll try and review thoroughly on Monday.

…validation logic

iainmcgin

Sorry for the very long poll on the review; I've been stretched very thin lately.

iainmcgin · 2017-04-07T19:26:59Z

+                        mClientConfiguration.getConnectionBuilder()
+                        .openConnection(uri);
+                conn.setRequestMethod("GET");
+                //conn.setDoOutput(true);


if these commented out code sections are not needed, just remove them.

iainmcgin · 2017-04-07T19:27:12Z

+                //conn.setDoOutput(true);
+
+                is = new BufferedInputStream(conn.getInputStream());
+                // readStream(in);


Same as above: remove if not needed.

iainmcgin · 2017-04-07T19:28:32Z

+            JSONObject requiredJson = null;
+
+            try {
+                String[] split = mResponse.idToken.split("\\.");


Validate that there are exactly three sections before proceeding.

iainmcgin · 2017-04-07T19:30:58Z

+                    return;
+                }
+
+                Calendar calendar = Calendar.getInstance();


I'm also not convinced that you're gaining much from calendar beyond directly using the exp value. Code can probably just be:

long expirationTime = Long.parseLong(jsonObjectBody.getString("exp") * 1000L; if (expirationTime > mClock.getCurrentTimeMillis()) { ... } else { ... }

arvindsinghtomar · 2017-07-20T06:48:31Z

@iainmcgin I have done changes you suggested in comment

iainmcgin · 2017-07-24T16:38:43Z

Looks like you still have checkstyle warnings - please run ./gradlew checkand ensure that this builds successfully. Also, you could separate your README change for Gluu into a separate pull request, as it is generally unrelated to the rest of this change.

ravibhojwani · 2017-08-16T18:53:33Z

Aeternity are working some magic

# Conflicts: # library/java/net/openid/appauth/AuthorizationService.java

arvindsinghtomar · 2017-09-19T04:29:52Z

@iainmcgin Now I have updated with the latest code, completed ./gradlew check and removed readme-gluu.md for now. Please review it.

arvindsinghtomar · 2017-10-09T03:53:51Z

@iainmcgin please review my PR

iainmcgin · 2017-10-09T23:16:17Z

Apologies, I've not had time to take a look yet.

nynymike · 2017-10-10T14:25:47Z

Thanks Iain! We really want to publish the Gluu README!

iainmcgin · 2018-08-07T23:51:52Z

Closing as #385 provides our preferred approach to ID token structure validation, without the signature verification. We have decided against signature verification within the core library as it isn't required for tokens acquired through an authorization code flow.

nynymike · 2018-08-08T15:10:11Z

Nice! That makes sense, it's easier to see if the TLS certificate cn matches the iss. You can still validate the signature, which might be good for higher assurance use cases.

id_token validation

fa8cac2

WilliamDenniss reviewed Mar 24, 2017

View reviewed changes

arvindsinghtomar added 2 commits March 27, 2017 12:42

Merge remote-tracking branch 'upstream/master'

4ec71f7

added url_safe flag to decode base 64 url and client id fix in token …

b24d549

…validation logic

iainmcgin reviewed Apr 7, 2017

View reviewed changes

arvindsinghtomar and others added 6 commits July 17, 2017 14:37

Create Readme-Gluu.md

05a1373

Update Readme-Gluu.md

3e57f80

Update Readme-Gluu.md

13217f5

Update Readme-Gluu.md

fe2ad2e

Merge branch 'master' into master

eac3d2a

made changes according comments by @iainmcgin on pull request.

68fbe40

arvindsinghtomar and others added 3 commits September 18, 2017 14:12

Merge branch 'master' of https://github.com/openid/AppAuth-Android

6e2f8c3

# Conflicts: # library/java/net/openid/appauth/AuthorizationService.java

Merge branch 'master' of https://github.com/openid/AppAuth-Android

5e24e76

# Conflicts: # library/java/net/openid/appauth/AuthorizationService.java

typos.

6aafff5

tomjackman mentioned this pull request Jan 19, 2018

AGDROID-686 - Add Token Verification [Do No Merge] aerogear/aerogear-android-sdk#12

Closed

4 tasks

WilliamDenniss added the openid-cla: no label Jul 16, 2018

iainmcgin closed this Aug 7, 2018

Uh oh!

Conversation

arvindsinghtomar commented Mar 22, 2017

Uh oh!

codecov-io commented Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nynymike commented Mar 23, 2017

Uh oh!

WilliamDenniss Mar 24, 2017

Choose a reason for hiding this comment

Uh oh!

iainmcgin commented Mar 25, 2017

Uh oh!

iainmcgin left a comment

Choose a reason for hiding this comment

Uh oh!

iainmcgin Apr 7, 2017

Choose a reason for hiding this comment

Uh oh!

iainmcgin Apr 7, 2017

Choose a reason for hiding this comment

Uh oh!

iainmcgin Apr 7, 2017

Choose a reason for hiding this comment

Uh oh!

iainmcgin Apr 7, 2017

Choose a reason for hiding this comment

Uh oh!

arvindsinghtomar commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iainmcgin commented Jul 24, 2017

Uh oh!

ravibhojwani commented Aug 16, 2017

Uh oh!

arvindsinghtomar commented Sep 19, 2017

Uh oh!

arvindsinghtomar commented Oct 9, 2017

Uh oh!

iainmcgin commented Oct 9, 2017

Uh oh!

nynymike commented Oct 10, 2017

Uh oh!

iainmcgin commented Aug 7, 2018

Uh oh!

nynymike commented Aug 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Mar 22, 2017 •

edited

Loading

arvindsinghtomar commented Jul 20, 2017 •

edited

Loading