[go: nahoru, domu]
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

[spaceship] Handle "Apple ID is locked" and other authentication errors better #14387

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[spaceship] Handle "Apple ID is locked" and other authentication errors better #14387

wants to merge 3 commits into from

Conversation

max-ott
Copy link
Contributor
@max-ott max-ott commented Mar 12, 2019 •
edited
Loading

Apple id returning different status and messages when the Apple ID is not "alright". We should better handle the cases, so spaceship does not break.

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Case: wrong password

INFO  [17:52:12]: >> GET https://olympus.itunes.apple.com/v1/session: [undefined body] 
DEBUG [17:52:13]: << GET https://olympus.itunes.apple.com/v1/session: 401 Unauthenticated

Request ID: LTG6ZX6EN55MAMQMQAYAKJN6.0.0

WARN  [17:52:13]: Auth lost
INFO  [17:52:13]: >> POST https://idmsa.apple.com/appleauth/auth/signin: {"accountName":"wrongpassword@acmecorp.com","password":"***","rememberMe":true} 
DEBUG [17:52:14]: << POST https://idmsa.apple.com/appleauth/auth/signin: 401 {"serviceErrors"=>[{"code"=>"-20101", "message"=>"Your Apple ID or password was incorrect."}]}
WARN  [17:52:14]: Auth lost

Case: account locked by Apple

INFO  [17:52:21]: >> POST https://idmsa.apple.com/appleauth/auth/signin: {"accountName":"locked@acmecorp.com","password":"***","rememberMe":true} 
DEBUG [17:52:22]: << POST https://idmsa.apple.com/appleauth/auth/signin: 403 {"serviceErrors"=>[{"code"=>"-20209", "message"=>"This Apple ID has been locked for security reasons. Visit iForgot to reset your account (https://iforgot.apple.com)."}]}

Case: Apple ID messed up by Apple (currently in process with dev support)

INFO  [17:53:58]: >> POST https://idmsa.apple.com/appleauth/auth/signin: {"accountName":"messedup@acmecorp.com","password":"***","rememberMe":true} 
DEBUG [17:53:59]: << POST https://idmsa.apple.com/appleauth/auth/signin: 403 {"serviceErrors"=>[{"code"=>"-20751", "message"=>"Your Apple ID or password was incorrect."}]}

Need help with:

It's probably possible to have the error handling in a nicer way. With my quick fix I get the following messages:

aa=432B7D3AE74EF5684B3E447669F46985; Domain=idmsa.apple.com; Path=/; Secure; HttpOnly, dslang=US-EN; Domain=apple.com; Path=/; Secure; HttpOnly, site=USA; Domain=apple.com; Path=/; Secure; HttpOnly

@max-ott
Handle "Apple ID is locked" status (WIP)
c911255 
Apple id returning different status and messages when the Apple ID is not "alright". We should better handle the cases, so spaceship does not break.
@janpio janpio changed the title Handle "Apple ID is locked" status (WIP) [WIP] Handle "Apple ID is locked" status Mar 14, 2019
@janpio
Copy link
Member
janpio commented Mar 14, 2019

With my quick fix I get the following messages:

What does this refer to? The code you posted looks like part of a cookie!?

@max-ott max-ott changed the title [WIP] Handle "Apple ID is locked" status Handle "Apple ID is locked" and other authentication errors better Mar 15, 2019
getaaron
getaaron requested changes Mar 16, 2019
View reviewed changes
spaceship/lib/spaceship/client.rb Outdated Show resolved Hide resolved
@getaaron
Copy link
Collaborator

Looks like there’s a linter error

@max-ott
Copy link
Contributor Author
max-ott commented Mar 19, 2019

Not sure how to fix this, because it never got touched:


spaceship/lib/spaceship/client.rb:400:5: C: Metrics/PerceivedComplexity: Perceived complexity for send_shared_login_request is too high. [20/18]
    def send_shared_login_request(user, password)
    ^^^

1049 files inspected, 1 offense detected```

janpio
janpio reviewed Mar 19, 2019
View reviewed changes
spaceship/lib/spaceship/client.rb
else
raise InvalidUserCredentialsError.new, "Login worked, but Apple reported that something was wrong anyway. Historically this has happened when an Apple ID got lost on Apple's end. See https://github.com/fastlane/fastlane/issues/14398"
case response.body["serviceErrors"][0]["code"]
when "-20209"
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a comment with the full response body here (and for all other error cases) for reference?
(Or as fixtures for a corresponding test?)

@getaaron
Copy link
Collaborator
getaaron commented Mar 22, 2019 •
edited
Loading

@max-ott Perceived complexity for send_shared_login_request is too high means (roughly) that there's too many conditionals in 1 method, and you can usually fix it by extracting some logic to a separate method

@janpio janpio changed the title Handle "Apple ID is locked" and other authentication errors better [spaceship] Handle "Apple ID is locked" and other authentication errors better Mar 22, 2019
@max-ott
Copy link
Contributor Author
max-ott commented Mar 29, 2019

No idea how to do that. Any help appreciated.

@janpio
Copy link
Member
janpio commented May 7, 2019

This are the docs of the cop: https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Metrics/PerceivedComplexity
As I understand it, moving parts of the method into their own method would probably solve the problem.
Question is: Which parts, why and how.

@rogerluan
Copy link
Member

@max-ott @janpio @getaaron any idea if this PR (and its underlying issue) is still relevant? It seems like its branch got deleted, not sure if we want to keep this PR open and move push it forward 👀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janpio janpio janpio left review comments

@getaaron getaaron getaaron requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
cla: yes status: needs-attention tool: spaceship
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@max-ott @janpio @getaaron @rogerluan @googlebot @fastlane-bot