FBXMPP OAuth

[Peter Hosey]

On Feb 28, 2011, at 17:55:36, Colin Barrett^[c2bFrank Dowsett wrote:
> <xmppFB.diff>

Return of the JSON framework!

> +- (void)doIT

Surely we can think of a better method name than that.

> +		token = @"176717249009197|2.SBY9kCilxdgHv6dRMAPZvQ__.86400.1297969200-899650296|AA0KSXp7kShNmKEC5PzAHdo6O50";

What is this? What is its origin/how was it made?

This sort of thing should be documented in a variable name and/or comment.

> +	NSString *token = [adium.accountController passwordForAccount:account];

> +	NSString *urlstring = [NSString stringWithFormat:@"https://graph.facebook.com/me?access_token=%@", token];

So, if the user sets a password, it will be passed to Facebook in clear text as a GET parameter? Is this necessary, or can we do better?

> +	NSDictionary *resp = [[[NSString alloc] initWithData:conn encoding:NSUTF8StringEncoding] JSONValue];

Leak.

> +        [[NSNotificationCenter defaultCenter] postNotificationName:FACEBOOK_OAUTH_FINISHED

> +		object:[self parseURLParams:[[request URL] fragment]]];

> +	NSString *token = [[note object] objectForKey:@"access_token"];

Are you sure you want fragment and not queryString?

I think an example of the sort of URL we're parsing here, stored in a comment in the redirect method, is warranted.

Also, I think @"access_token" should be a macro. Literal strings in code bodies worry me; literal strings that appear more than once, doubly so.

> +//Save controls
> +- (void)saveConfiguration
> +{
> +	
>  }

It would be a good idea to document why this is implemented as empty. Are we suppressing some superclass behavior? If so, what, and why?

> +	NSMutableDictionary *params = [[[NSMutableDictionary alloc] init] autorelease];

You can use dictionary here instead of alloc, init, and autorelease.

> +		NSArray *kv = [pair componentsSeparatedByString:@"="];
> +		NSString *val =
> +		[[kv objectAtIndex:1]
> +		 stringByReplacingPercentEscapesUsingEncoding:NSUTF8StringEncoding];
> +		
> +		[params setObject:val forKey:[kv objectAtIndex:0]];

This is mostly just a style niggle, but for things like this, I prefer faking Python's unpacking behavior using NSEnumerator:

	NSEnumerator *kvEnum = [kv objectEnumerator];
	NSString *key = [kvEnum nextObject];
	NSString *val = [kvEnum nextObject];
	val = [val stringByReplacingPercentEscapesUsingEncoding:NSUTF8StringEncoding];
	[params setObject:val forKey:key];

As a general rule, whenever you find yourself typing “objectAtIndex:”, it is worth stopping to think of how else you might do it.

Mainly, I dislike indexes because it's easy to get them wrong. Copying and pasting is one way: Forgetting to change something after pasting is a very common cause of bugs. In the NSEnumerator version, exactly the same message is sent both times, and it successfully gets both components.

> - at interface AIFacebookXMPPService : AIService {
> + at interface AIFacebookXMPPService : PurpleService {


Should this be a subclass of ESJabberService?

> +- (NSCharacterSet *)allowedCharacters{
> +	return [NSCharacterSet characterSetWithCharactersInString:@"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_ "];
> +}
> +
> +- (NSCharacterSet *)allowedCharactersForUIDs{
> +	return [NSCharacterSet characterSetWithCharactersInString:@"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_"];
>  }

Do we really only want the ASCII letters and numbers, or would it be better to use the alphanumeric character set and add the ‘_’ and space to it?

Also, is that space supposed to be there? Do we only want the space, or should we allow all whitespace? What about line breaks?

If we make this a subclass of ESJabberService, should we inherit its -allowedCharacters and -allowedCharactersForUIDs methods?

> +GCC_VERSION = com.apple.compilers.llvm.clang.1_0

Is this meant to be included in this patch?