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

AGDROID-682 Include checkAuthStatus #14

Merged
merged 3 commits into from
Jan 26, 2018

Conversation

aidenkeating
Copy link
Contributor

@aidenkeating aidenkeating commented Jan 23, 2018

Motivation

Allow checking for current auth status. Both is authorized and token isn't expired.

JIRA: https://issues.jboss.org/browse/AGDROID-682

Description

Wraps AuthState some more with OIDCCredentials, mainly wrapping isAuthorized() and getNeedsTokenRefresh().

Check these to determine the current auth state using checkAuthState().

Progress

  • Check if authorized
  • Check if needs refresh (is expired)
  • Unit tests (Everything is a thin wrapper around AppAuth Android)

@aidenkeating
Copy link
Contributor Author

@TommyJ1994 Would you mind taking a look? Still a bit more work to be done, just sanity checking the current work

@tomjackman tomjackman self-requested a review January 23, 2018 13:05
@tomjackman
Copy link
Contributor

Looks good so far 👍

@aidenkeating
Copy link
Contributor Author

@TommyJ1994 Only coming back to this now. I'm not sure the value testing would add here as we're wrapping AppAuth very closely. Is there anything else this check needs to do?

@ziccardi Would you mind taking a look also?

@tomjackman
Copy link
Contributor

@aidenkeating Agreed, its a light wrapper so I think we can do without tests.

@ziccardi
Copy link
Member

The only comment I have about this, is about adding the get/set methods and the isAuthorized method.
The ICredential interface is a generic interface that could be used for other type of authentication as well. I think it would be better to move those methods the OIDCCredential class

@aidenkeating
Copy link
Contributor Author

aidenkeating commented Jan 25, 2018

@ziccardi Just to confirm, you mean changing UserPrincipalImpl to use OIDCCredential instead of the generic ICredential? https://github.com/aidenkeating/aerogear-android-sdk/blob/a63f7db58971b61d6fd55d8a9ad1ae482db9a9da/auth/src/main/java/org/aerogear/auth/impl/UserPrincipalImpl.java#L37 If so, that sounds good to me, will make that change

Edit: Like this b0c8452

@aidenkeating aidenkeating changed the title [Do not merge] AGDROID-682 Include checkAuthStatus AGDROID-682 Include checkAuthStatus Jan 25, 2018
@ziccardi
Copy link
Member

ziccardi commented Jan 25, 2018

@aidenkeating No, I just mean moving those methods to the OIDCCretential object.
UserPrincipalImpl should always store an ICredential object so that different kind of credentials can be stored.
Since OIDCCredentials implements the ICredential object, UserPrincipalImpl is able to store an OIDCCredential object without any change

* @return <code>true</code> if user is authorized and token is not expired.
*/
@Override
public boolean checkValidAuth() {
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to add this method to the credentials object so that the UserPrincipalImpl remains general purpose. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I understand what you mean now, yep that makes sense to me

@aidenkeating
Copy link
Contributor Author

@ziccardi Would you mind taking another look? Moved checkAuthState into the credentials and also generalised the credential method names a bit too.

@aidenkeating
Copy link
Contributor Author

@ziccardi Mind taking another look?

Copy link
Member

@ziccardi ziccardi left a comment

Choose a reason for hiding this comment

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

LGTM

@aidenkeating aidenkeating merged commit cd7d210 into aerogear:master Jan 26, 2018
@aidenkeating aidenkeating deleted the AGDROID-682 branch January 26, 2018 15:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants