Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RSA10d #493

Merged
merged 2 commits into from
Oct 3, 2016
Merged

RSA10d #493

merged 2 commits into from
Oct 3, 2016

Conversation

EvgenyKarkan
Copy link
Contributor

No description provided.

@mattheworiordan
Copy link
Member

Not entirely sure I understand the isOnlyForce logic, but this change sin 0.9 anyway so I think we should merge anyway

@EvgenyKarkan
Copy link
Contributor Author

EvgenyKarkan commented Sep 7, 2016 •
edited
Loading

Not entirely sure I understand the isOnlyForce logic

RSA10d spec states:

A special case convenience exists for AuthOption stating that if all its attributes are null apart from force, then when passed to authorise as an argument, the previously configured authentication options will remain intact.

So I had to check that all attributes except force are nil, store result in isOnlyForce private boolean variable and use it inside scope of ARTAuth class.

@mattheworiordan
Copy link
Member

So I had to check that all attributes except force are nil, store result in isOnlyForce boolean variable and use inside scope of ARTAuth class.

Ok, thanks, I do follow now, sorry about that!

@EvgenyKarkan
Copy link
Contributor Author

Please review and merge if everything looks good

tcard
tcard reviewed Sep 20, 2016
View reviewed changes
Source/ARTAuth.m
@@ -25,6 +25,7 @@ @implementation ARTAuth {
ARTTokenParams *_tokenParams;
// Dedicated to Protocol Message
NSString *_protocolClientId;
BOOL _isOnlyForceTrue;
Copy link
Contributor

@tcard tcard Sep 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Why is this an instance property of the ARTAuth class when it's only relevant inside the authorise method? Shouldn't it be declared as a local there?

Source/ARTAuth.m
if (error) {
callback(nil, error);
} else {
[self executeTokenRequest:tokenRequest callback:callback];
}
_isOnlyForceTrue = NO;
Copy link
Contributor

@tcard tcard Sep 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This change doesn't have any effect, does it? The only other place where _isOnlyForceTrue is used is in authorise, and there it's always being set before being read.

@mattheworiordan
Copy link
Member

@EvgenyKarkan I am aware that this change is a 0.9 change that is breaking i.e. we should not be merging this into master as that is currently the 0.8 branch. Before we merge this, I think we need to start a 0.8 branch and maintain that in case any hot fixes are needed on that branch.

It may be worth having a quick catch up today / tomorrow to discuss our 0.8/0.9 strategy

@ricardopereira
Copy link
Contributor

Will change the base to master.

@ricardopereira ricardopereira changed the base branch from RSA10j to master September 29, 2016 09:24
@ricardopereira
Copy link
Contributor

Squashed.

@ricardopereira
Copy link
Contributor

@mattheworiordan @tcard Are you happy with this?

@ricardopereira
Copy link
Contributor

@mattheworiordan Just clarifying, i'm confused. Why are you saying that this is a 0.9 change and should not be merged? From what I saw, the 0.9 removes the force property, so this change is only for 0.8, right?

@mattheworiordan
Copy link
Member

LGTM

tcard
tcard approved these changes Oct 3, 2016
View reviewed changes
Copy link
Contributor

@tcard tcard left a comment

Choose a reason for hiding this comment

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

LGTM

@ricardopereira ricardopereira merged commit 61ccf4b into master Oct 3, 2016
@ricardopereira ricardopereira deleted the RSA10d branch October 3, 2016 15:20
@ricardopereira ricardopereira mentioned this pull request Oct 12, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tcard tcard tcard approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@EvgenyKarkan @mattheworiordan @ricardopereira @tcard