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

Box #85

Closed
wants to merge 8 commits into from
Closed

Box #85

wants to merge 8 commits into from

Conversation

dkhamsing
Copy link
Contributor

Hey Caleb, would love for you to take a look

I tested this successfully with the example app although setting up Box to work in an app requires a special redirect URI in the format boxsdk-YOUR_CLIENT_ID://boxsdkoauth2redirect (source)

As you can see, we can get the redirect URI from the CLIENT_ID but I couldn't get this to work so for now, you have to set

    NSString *boxClientId = @"CLIENT_ID";
    SimpleAuth.configuration[@"box-web"] = @{
                                             @"client_id":boxClientId,
                                             @"client_secret":@"CLIENT_SECRET",
                                             @"redirect_uri" : [NSString stringWithFormat:@"boxsdk-%@://boxsdkoauth2redirect", boxClientId],
                                             };

@calebd
Copy link
Owner

calebd commented Mar 26, 2015

Can the provider automatically synthesize the box redirect url for you? That seems like something the consumer should not have to do.

@calebd
Copy link
Owner

calebd commented Mar 26, 2015

Oh wait you're saying you tried that and it didn't work?

@dkhamsing
Copy link
Contributor Author

Yes the provider should do it.. I was trying to update self.options but it is readonly.. should I override the getter for options ?

@calebd
Copy link
Owner

calebd commented Mar 26, 2015

Do you need to store it in options or can you just synthesize it where it is needed?

@dkhamsing
Copy link
Contributor Author

Will need it in the target url check isTargetRedirectURL, what do u recommended?

@calebd
Copy link
Owner

calebd commented Mar 26, 2015

If you only need it in the web view controller, I would make a mutable copy of self.options, synthesize the value, and pass that new dictionary to the view controller initializer. Then the view controller can include it in the initial request as well as the url check.

If you need it in other places, consider making a private, read-only property in the provider that returns the synthesized value that you can use elsewhere.

I would not override the options accessor or try to mutate it. It is immutable for a reason.

@calebd
Copy link
Owner

calebd commented Mar 26, 2015

Actually, you can override initWithOptions:, copy and mutate the dictionary, then call super with the new value there and it will be usable everywhere in the provider.

@dkhamsing
Copy link
Contributor Author

Will try, thanks

@dkhamsing
Copy link
Contributor Author

Hey Caleb, the provider now takes care of redirect URI so the user can just set

 SimpleAuth.configuration[@"box-web"] = @{
  @"client_id":@"CLIENT_ID",
  @"client_secret":@"CLIENT_SECRET",
};

Worked in the example app

@dkhamsing
Copy link
Contributor Author

There's a bit of code duplication for how that redirect URI is generated, let me know if that's ok or how it could be improved (class method on SimpleAuthBoxWebLoginViewController?)

@calebd
Copy link
Owner

calebd commented Mar 27, 2015

I flattened the redirect_uri building by moving it into -[SimpleAuthBoxWebProvider initWithOptions:]. This way the redirect_uri is calculated only once and is available everywhere in the provider and its login view controller.

Would you mind pulling my box branch and testing it out?

The specific change can be seen in 35e9ea5 if you are curious.

@calebd
Copy link
Owner

calebd commented Mar 27, 2015

If it looks good to you I'll merge the branch into master and close the pull request. Thanks for your work on this!

@dkhamsing
Copy link
Contributor Author

Ok will try it out

dkhamsing added 2 commits March 27, 2015 14:00
Conflicts:
	Example/SimpleAuth/SADProviderListViewController.m
@dkhamsing
Copy link
Contributor Author

Tried out the changes 👍
Ok to merge 😄

@calebd calebd closed this Mar 27, 2015
@dkhamsing
Copy link
Contributor Author

Uh..

@calebd
Copy link
Owner

calebd commented Mar 27, 2015

Your commits are now in master. You didn't pull the right branch from my repo into yours so none of the changes I made where in there. Just easier to merge it myself and close.

@calebd
Copy link
Owner

calebd commented Mar 27, 2015

Which now leads me to wonder: did you check out my box branch and the commit I referenced, or did you only pull master? If not the change I made might not be correct. I don't have a box account so I am unable to verify.

@dkhamsing
Copy link
Contributor Author

Caleb I tested master, we are all good

@calebd
Copy link
Owner

calebd commented Mar 28, 2015

Awesome! Thanks again for your work on this!

@dkhamsing
Copy link
Contributor Author

😄

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

2 participants