It's the right idea. Syntax note:
EXEC sp_addrolemember 'db_datareader', 'MyDatabase';
Also note that you can grant login permissions based on Windows accounts. eg::
create login [domain\user] from windows
If you're managing a db with lots of users, it's often better to manage at the Windows group level. Same syntax as above but you only have to add the group once, then add users to the group